*: support logging session connect attrs to slow query log (test split experiment)#67115
*: support logging session connect attrs to slow query log (test split experiment)#67115jiong-nba wants to merge 10 commits intopingcap:masterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds end-to-end session connection attributes: new vardef/sysvar state and metrics, handshake parsing with truncation/limits and warnings, slow-log serialization and slow-query ingestion of Session_connect_attrs, tests across parsing, slow-query, cluster table, server, and initstats, plus test/build adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server(parse.go)
participant Vardef as vardef (atomics)
participant Logger as Logger
Client->>Server: send HandshakeResponse (length-encoded attrs)
Server->>Server: read length, check bounds (malformed -> ErrMalformPacket)
Server->>Vardef: Load ConnectAttrsSize
alt ConnectAttrsSize == 0
Server->>Vardef: return empty attrs, no warnings
else
Server->>Server: decode k/v pairs, accumulate total size
alt total size > effective limit
Server->>Vardef: increment ConnectAttrsLost (first truncation)
Server->>Server: set `_truncated` metadata with discarded bytes
Server->>Vardef: maybe update ConnectAttrsLongestSeen
Server->>Logger: emit truncation warning
end
alt contains leading-underscore custom keys not allowed
Server->>Logger: emit deprecation warning
end
Server->>Server: return attrs, warningsText
end
Server->>Logger: log warningsText at debug (if non-empty)
alt declared length > 1MiB
Server->>Client: refuse connection (error)
else
Server->>Client: accept connection with packet.Attrs set
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-chi-bot
Bot
added
the
size/M
|
Hi @jiong-nba. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
There was a problem hiding this comment.
Pull request overview
This PR is a comment-only, no-op experiment to reproduce CI behavior by touching the same file set as #66617, helping isolate whether failures (e.g. fast_test_tiprow / TestInitStatsSessionBlockGC) correlate with the touched targets rather than code logic.
Changes:
- Added a trailing no-op comment line to the same Go source and test files touched by #66617.
- Added a trailing no-op comment line to the same Bazel
BUILD.bazeltargets touched by #66617.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/testkit/testutil/require.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/sessionctx/variable/tests/variable_test.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/sessionctx/variable/tests/session_test.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/sessionctx/variable/tests/BUILD.bazel | Add trailing no-op comment to touch Bazel target for CI investigation. |
| pkg/sessionctx/variable/sysvar.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/sessionctx/variable/statusvar.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/sessionctx/variable/slow_log.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/sessionctx/variable/noop.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/sessionctx/vardef/tidb_vars.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/sessionctx/vardef/sysvar.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/server/internal/parse/parse_test.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/server/internal/parse/parse.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/server/internal/parse/BUILD.bazel | Add trailing no-op comment to touch Bazel target for CI investigation. |
| pkg/server/conn_test.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/infoschema/test/clustertablestest/tables_test.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/infoschema/test/clustertablestest/cluster_tables_test.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/infoschema/test/clustertablestest/BUILD.bazel | Add trailing no-op comment to touch Bazel target for CI investigation. |
| pkg/infoschema/tables.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/executor/slow_query_test.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/executor/slow_query_sql_test.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/executor/slow_query.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/executor/cluster_table_test.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/executor/adapter_slow_log.go | Add trailing no-op comment to touch file for CI investigation. |
| pkg/executor/BUILD.bazel | Add trailing no-op comment to touch Bazel target for CI investigation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/test fast_test_tiprow |
|
@jiong-nba: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@jiong-nba: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test all |
|
@jiong-nba: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
|
/test fast_test_tiprow |
|
@jiong-nba: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test all |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67115 +/- ##
================================================
+ Coverage 77.7126% 79.3992% +1.6866%
================================================
Files 2016 1962 -54
Lines 552501 539534 -12967
================================================
- Hits 429363 428386 -977
+ Misses 121396 109697 -11699
+ Partials 1742 1451 -291
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
jiong-nba
changed the title
jiong-nba
force-pushed
the
jiong-nba/ci-noop-same-files-66617
branch
from
ece611a to
78ea5b3
Compare
ti-chi-bot
Bot
added
size/XXL
|
/retest |
…parsing and tests
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/server/tests/initstats/initstats_test.go (1)
86-92: Context is already canceled beforeInitStatsLiteis called.After
h.InitStats(ctx, dom.InfoSchema())returns due to cancellation (line 91), thectxis already in a canceled state. The subsequenth.InitStatsLite(ctx)call (line 92) will immediately seectx.Err() == context.Canceledrather than experiencing an active cancellation while blocked.If the intent is to verify that
InitStatsLitecan be actively canceled mid-execution (matching the test name), consider using a fresh context:♻️ Suggested fix to test active cancellation for both functions
- ctx, cancel := context.WithCancel(context.Background()) - go func() { - time.Sleep(time.Second) - cancel() - }() - require.ErrorIs(t, h.InitStats(ctx, dom.InfoSchema()), context.Canceled) - require.ErrorIs(t, h.InitStatsLite(ctx), context.Canceled) + ctx1, cancel1 := context.WithCancel(context.Background()) + go func() { + time.Sleep(time.Second) + cancel1() + }() + require.ErrorIs(t, h.InitStats(ctx1, dom.InfoSchema()), context.Canceled) + + ctx2, cancel2 := context.WithCancel(context.Background()) + go func() { + time.Sleep(time.Second) + cancel2() + }() + require.ErrorIs(t, h.InitStatsLite(ctx2), context.Canceled)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/tests/initstats/initstats_test.go` around lines 86 - 92, The test uses the same canceled ctx for both calls so InitStatsLite sees ctx already canceled; create a fresh cancellable context (e.g., ctx2, cancel2 := context.WithCancel(context.Background())) and a separate goroutine to cancel it after a delay before calling h.InitStatsLite(ctx2) so that InitStats and InitStatsLite each experience an active mid-execution cancellation; ensure you call the matching cancel functions or defer cancels to avoid leaks and assert ErrorIs(..., context.Canceled) for both InitStats and InitStatsLite.pkg/sessionctx/vardef/tidb_vars.go (1)
1775-1777: Align these exported comments with the actual normalization rules.
pkg/server/internal/parse/parse.gonormalizes-1to a 64 KiB cap, andConnectAttrsLongestSeendeliberately ignores payloads>= 64 KiB. The new comments read a bit broader than that, so spelling those constraints out here would keep the vardef contract accurate for future readers.As per coding guidelines, comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs.
Also applies to: 1914-1918
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sessionctx/vardef/tidb_vars.go` around lines 1775 - 1777, Update the exported comment for DefConnectAttrsSize (and the similar comment around lines 1914-1918) to precisely state the normalization and caps: note that a configured value of -1 is normalized to a 64 KiB cap by pkg/server/internal/parse/parse.go and that ConnectAttrsLongestSeen intentionally ignores payloads >= 64 KiB, so the effective maximum tracked/used size is 64 KiB; mention these constraints and the SQL/compatibility semantics in the comment so readers know the invariant and where normalization occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/server/internal/parse/parse.go`:
- Around line 195-205: The helper parseAttrs should preserve a nil map to
represent “no attrs”: change the early-return when
vardef.ConnectAttrsSize.Load() == 0 to return nil, "", nil instead of an
allocated map, and likewise update the other similar helper later in the file
(the helper in the 241-282 region that currently returns map[string]string{} for
no-attrs cases) to return nil, "", nil; ensure any call sites that copy
resp.Attrs (e.g., conn.go logic referenced in the review) still handle a nil map
as “absent” and keep other error-return paths unchanged.
---
Nitpick comments:
In `@pkg/server/tests/initstats/initstats_test.go`:
- Around line 86-92: The test uses the same canceled ctx for both calls so
InitStatsLite sees ctx already canceled; create a fresh cancellable context
(e.g., ctx2, cancel2 := context.WithCancel(context.Background())) and a separate
goroutine to cancel it after a delay before calling h.InitStatsLite(ctx2) so
that InitStats and InitStatsLite each experience an active mid-execution
cancellation; ensure you call the matching cancel functions or defer cancels to
avoid leaks and assert ErrorIs(..., context.Canceled) for both InitStats and
InitStatsLite.
In `@pkg/sessionctx/vardef/tidb_vars.go`:
- Around line 1775-1777: Update the exported comment for DefConnectAttrsSize
(and the similar comment around lines 1914-1918) to precisely state the
normalization and caps: note that a configured value of -1 is normalized to a 64
KiB cap by pkg/server/internal/parse/parse.go and that ConnectAttrsLongestSeen
intentionally ignores payloads >= 64 KiB, so the effective maximum tracked/used
size is 64 KiB; mention these constraints and the SQL/compatibility semantics in
the comment so readers know the invariant and where normalization occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 06f2618e-6d2e-4d3e-86fe-7fa5004f69a4
📒 Files selected for processing (29)
pkg/executor/BUILD.bazelpkg/executor/adapter_slow_log.gopkg/executor/cluster_table_test.gopkg/executor/slow_query.gopkg/executor/slow_query_sql_test.gopkg/executor/slow_query_test.gopkg/infoschema/tables.gopkg/infoschema/test/clustertablestest/BUILD.bazelpkg/infoschema/test/clustertablestest/cluster_tables_test.gopkg/infoschema/test/clustertablestest/tables_test.gopkg/server/BUILD.bazelpkg/server/conn_test.gopkg/server/internal/parse/BUILD.bazelpkg/server/internal/parse/parse.gopkg/server/internal/parse/parse_test.gopkg/server/stat_test.gopkg/server/tests/initstats/BUILD.bazelpkg/server/tests/initstats/initstats_test.gopkg/server/tests/initstats/main_test.gopkg/sessionctx/vardef/sysvar.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/noop.gopkg/sessionctx/variable/slow_log.gopkg/sessionctx/variable/statusvar.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/tests/BUILD.bazelpkg/sessionctx/variable/tests/session_test.gopkg/sessionctx/variable/tests/variable_test.gopkg/testkit/testutil/require.go
💤 Files with no reviewable changes (3)
- pkg/server/BUILD.bazel
- pkg/server/stat_test.go
- pkg/sessionctx/variable/noop.go
✅ Files skipped from review due to trivial changes (5)
- pkg/sessionctx/variable/tests/BUILD.bazel
- pkg/infoschema/test/clustertablestest/BUILD.bazel
- pkg/executor/BUILD.bazel
- pkg/server/tests/initstats/BUILD.bazel
- pkg/server/internal/parse/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (12)
- pkg/executor/adapter_slow_log.go
- pkg/sessionctx/vardef/sysvar.go
- pkg/sessionctx/variable/statusvar.go
- pkg/executor/slow_query.go
- pkg/infoschema/test/clustertablestest/tables_test.go
- pkg/server/internal/parse/parse_test.go
- pkg/infoschema/tables.go
- pkg/testkit/testutil/require.go
- pkg/sessionctx/variable/slow_log.go
- pkg/sessionctx/variable/sysvar.go
- pkg/server/conn_test.go
- pkg/sessionctx/variable/tests/variable_test.go
| func parseAttrs(data []byte) (map[string]string, string, error) { | ||
| if vardef.ConnectAttrsSize.Load() == 0 { | ||
| return map[string]string{}, "", nil | ||
| } | ||
|
|
||
| decoded, err := decodeConnAttrs(data) | ||
| if err != nil { | ||
| return map[string]string{}, "", err | ||
| } | ||
| attrs, warningsText := applyConnAttrsPolicyAndMetrics(decoded, vardef.ConnectAttrsSize.Load()) | ||
| return attrs, warningsText, nil |
There was a problem hiding this comment.
Preserve nil for the “no attrs” cases.
These helpers always return an allocated map. pkg/server/conn.go:642-646 copies resp.Attrs straight into cc.attrs, so performance_schema_session_connect_attrs_size = 0 stops meaning “no attrs” downstream and starts meaning “present but empty”. That loses the missing-vs-empty distinction and can surface as {} instead of omitted/null in later JSON consumers.
💡 Suggested fix
func parseAttrs(data []byte) (map[string]string, string, error) {
if vardef.ConnectAttrsSize.Load() == 0 {
- return map[string]string{}, "", nil
+ return nil, "", nil
}
decoded, err := decodeConnAttrs(data)
if err != nil {
- return map[string]string{}, "", err
+ return nil, "", err
}
attrs, warningsText := applyConnAttrsPolicyAndMetrics(decoded, vardef.ConnectAttrsSize.Load())
return attrs, warningsText, nil
}
func applyConnAttrsPolicyAndMetrics(decoded decodedConnAttrs, limit int64) (map[string]string, string) {
- attrs := make(map[string]string)
+ var attrs map[string]string
effectiveLimit := normalizeConnectAttrsLimit(limit)
var totalSize int64
var acceptedSize int64
truncated := false
@@
if totalSize > effectiveLimit {
if !truncated {
truncated = true
vardef.ConnectAttrsLost.Add(1)
}
continue
}
if !truncated {
+ if attrs == nil {
+ attrs = make(map[string]string)
+ }
attrs[item.key] = item.value
acceptedSize += kvSize
}
}
@@
if truncated {
truncatedBytes := decoded.totalSize - acceptedSize
+ if attrs == nil {
+ attrs = make(map[string]string)
+ }
attrs[reservedConnAttrTruncated] = strconv.FormatInt(truncatedBytes, 10)
warnings = append(warnings, fmt.Sprintf(
"session connection attributes truncated: total size %d bytes exceeds "+
"performance_schema_session_connect_attrs_size (%d), %d bytes were discarded",
decoded.totalSize, effectiveLimit, truncatedBytes))
}Also applies to: 241-282
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/server/internal/parse/parse.go` around lines 195 - 205, The helper
parseAttrs should preserve a nil map to represent “no attrs”: change the
early-return when vardef.ConnectAttrsSize.Load() == 0 to return nil, "", nil
instead of an allocated map, and likewise update the other similar helper later
in the file (the helper in the 241-282 region that currently returns
map[string]string{} for no-attrs cases) to return nil, "", nil; ensure any call
sites that copy resp.Attrs (e.g., conn.go logic referenced in the review) still
handle a nil map as “absent” and keep other error-return paths unchanged.
78ea5b3 to
0b8fc58
Compare
ti-chi-bot
Bot
added
the
needs-cherry-pick-release-8.5
ti-chi-bot
Bot
added
needs-cherry-pick-release-8.5
What problem does this PR solve?
Issue Number: ref #66616
Problem Summary:
In a Kubernetes deployment environment, container IPs change frequently. The current slow query log records only IP information, which is insufficient to trace the real source or application of SQL statements. Recording
SESSION_CONNECT_ATTRSin the slow log provides critical client metadata (such asapp_name,_os,_client_name, etc.) injected during the connection handshake.This PR is used as a CI isolation experiment for #66617. Other code changes are kept aligned with #66617. The only intentional difference is that
TestInitStatsSessionBlockGCandTestInitStatsSessionBlockGCCanBeCanceledare moved out ofpkg/serverinto an isolated test packagepkg/server/tests/initstats.What changed and how does it work?
This PR introduces the same feature changes as #66617:
Slow Query Log Enhancement:
Session_connect_attrsfield in JSON format to the slow query file (pkg/sessionctx/variable/slow_log.go).SESSION_CONNECT_ATTRSJSON column to both theSLOW_QUERYandCLUSTER_SLOW_QUERYsystem tables (pkg/infoschema/tables.go).pkg/executor/slow_query.go).System Variables:
performance_schema_session_connect_attrs_sizefrom anoopvariable to a functionalGLOBALvariable (pkg/sessionctx/variable/sysvar.go), controlling the maximum total byte size of attributes per connection (default: 4096).Status Variables & Truncation Logic:
performance_schema_session_connect_attrs_size(pkg/server/internal/parse/parse.go).GLOBALstatus variables:Performance_schema_session_connect_attrs_longest_seenandPerformance_schema_session_connect_attrs_lostto monitor attribute size and truncation stats._truncatedto record discarded bytes.Test Isolation Difference From *: support logging session connect attrs to slow query log #66617:
TestInitStatsSessionBlockGCandTestInitStatsSessionBlockGCCanBeCanceledfrompkg/server/stat_test.goto a dedicated packagepkg/server/tests/initstats.fast_test_tiprowinstability is related to same-binary test interaction insidepkg/server.Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
New Features
Tests