*: support logging session connect attrs to slow query log#66617
*: support logging session connect attrs to slow query log#66617ti-chi-bot[bot] merged 11 commits intopingcap:masterfrom
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-tests-checked
release-note-none
|
Review Complete Findings: 2 issues |
ti-chi-bot
Bot
added
the
size/XL
|
|
|
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCapture, parse, store, and expose client Session_connect_attrs: handshake decoding with limits/truncation/metrics, new sysvar and status entries, slow-query emission and parsing of Session_connect_attrs, and tests across server, executor, and session packages. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant HandshakeParser as Handshake Parser
participant Validator as Validation & Truncation
participant SessionCtx as Session Context
participant SlowLog as Slow Log Emitter
participant InfoSchema as Slow Query Store
Client->>HandshakeParser: send handshake with connect attrs
HandshakeParser->>Validator: pass raw attrs bytes
rect rgba(220, 180, 120, 0.5)
alt total size > 1 MiB
Validator-->>HandshakeParser: return error (hard limit)
HandshakeParser-->>Client: reject connection
else within hard limit
alt exceeds configured ConnectAttrsSize
Validator->>Validator: truncate accepted attrs, set "_truncated"
Validator->>SessionCtx: record accepted attrs
Validator->>SessionCtx: ConnectAttrsLost += truncated_bytes
else accept all attrs
Validator->>SessionCtx: store attrs
end
alt totalSize < 64 KiB
Validator->>Validator: update ConnectAttrsLongestSeen
end
HandshakeParser-->>Client: handshake OK
end
end
Client->>SlowLog: execute query
SlowLog->>SessionCtx: read SessionConnectAttrs
SlowLog->>SlowLog: encode attrs as JSON
SlowLog->>InfoSchema: write slow log row with Session_connect_attrs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR enhances TiDB’s slow query logging and slow-log system tables by capturing and exposing MySQL client SESSION_CONNECT_ATTRS (connection attributes) to better identify client/application sources in dynamic environments (e.g., Kubernetes).
Changes:
- Add
Session_connect_attrsto the slow log output (JSON) and expose it viaSLOW_QUERY/CLUSTER_SLOW_QUERYas a JSON column. - Implement a functional global system variable
performance_schema_session_connect_attrs_sizeplus global status counters to track truncation and largest seen payload. - Enforce handshake parsing limits (1 MiB hard cap + configurable soft cap) and add tests for truncation/rejection and slow-log parsing/formatting.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/sessionctx/variable/tests/session_test.go | Adds test assertions for slow log serialization and placement of Session_connect_attrs. |
| pkg/sessionctx/variable/sysvar.go | Implements performance_schema_session_connect_attrs_size as a real GLOBAL sysvar backed by an atomic. |
| pkg/sessionctx/variable/statusvar.go | Adds GLOBAL status variables and wires them to atomic counters. |
| pkg/sessionctx/variable/slow_log.go | Adds Session_connect_attrs slow log field and JSON serialization in slow log formatter. |
| pkg/sessionctx/variable/noop.go | Removes the previous noop definition of performance_schema_session_connect_attrs_size. |
| pkg/sessionctx/vardef/tidb_vars.go | Adds defaults and atomic counters for connect-attrs size/metrics. |
| pkg/sessionctx/vardef/sysvar.go | Adds vardef constant for performance_schema_session_connect_attrs_size. |
| pkg/server/internal/parse/parse.go | Adds hard/soft limits, truncation accounting, and warning emission during handshake attrs parsing. |
| pkg/server/conn_test.go | Adds handshake parsing tests for truncation, hard-limit rejection, and longest-seen behavior. |
| pkg/infoschema/tables.go | Adds JSON column for Session_connect_attrs to slow query system tables. |
| pkg/executor/slow_query_test.go | Updates expected record strings and adds a parser test for Session_connect_attrs JSON. |
| pkg/executor/slow_query.go | Parses Session_connect_attrs field and maps it to a JSON datum for system table output. |
| pkg/executor/adapter_slow_log.go | Plumbs SessionConnectAttrs from SessionVars.ConnectionInfo.Attributes into slow log items. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/server/internal/parse/parse.go (1)
136-141: Add an explicit bounds check before slicing the attrs payload.Line 140 can still hit panic/recover for malformed packets where declared
numis within 1 MiB but exceeds remaining bytes. Prefer returningmysql.ErrMalformPacketdirectly.♻️ Proposed hardening
if num, null, intOff := util2.ParseLengthEncodedInt(data[offset:]); !null { if num > 1<<20 { // 1 MiB hard limit return errors.New("connection refused: session connection attributes exceed the 1 MiB hard limit") } offset += intOff // Length of variable length encoded integer itself in bytes + if int(num) > len(data)-offset { + return mysql.ErrMalformPacket + } row := data[offset : offset+int(num)] attrs, warning, err := parseAttrs(row)🤖 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 136 - 141, The slice operation at row := data[offset : offset+int(num)] can panic if num exceeds remaining bytes even when under the 1 MiB limit; before slicing, check that offset+int(num) <= len(data) and if not return mysql.ErrMalformPacket (or the package's equivalent) instead of letting a panic occur, preserving existing behavior for parseAttrs and using the same local symbols (num, offset, intOff, parseAttrs) to locate and harden the bounds check.
🤖 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/executor/slow_query_test.go`:
- Around line 287-341: Add an integration test in slow_query_sql_test.go (e.g.
TestSlowQuerySessionConnectAttrs) that exercises SESSION_CONNECT_ATTRS via SQL:
create a temp slow log file containing the same slowLogStr used in
TestParseSlowLogSessionConnectAttrs, initialize/reload the slow-query retriever
(reuse newSlowQueryRetriever/parseLog or the service initialization path used by
other slow_query_sql tests) so the slow log row is visible to
INFORMATION_SCHEMA.SLOW_QUERY, then run a TestKit query like SELECT
SESSION_CONNECT_ATTRS FROM INFORMATION_SCHEMA.SLOW_QUERY WHERE Digest=... (or
other identifying filter) and assert the returned JSON contains
"_client_name","Go-MySQL-Driver","_os","linux","app_name","test_app"; ensure
cleanup of the temp file and any retriever state.
In `@pkg/server/internal/parse/parse.go`:
- Around line 213-221: Reject or strip any client-supplied "_truncated" connect
attribute when building attrs and only set attrs["_truncated"] internally when
truncated == true; specifically, during attribute parsing/ingestion ensure you
ignore keys equal to "_truncated" (or rename the internal marker to a clearly
reserved constant and remove client-provided values) so that the only time
attrs["_truncated"] appears is when you assign truncatedBytes = totalSize -
acceptedSize and set attrs["_truncated"] = strconv.FormatInt(truncatedBytes, 10)
from the truncation branch (the symbols to locate are attrs, "_truncated",
truncated, truncatedBytes, totalSize, acceptedSize).
In `@pkg/sessionctx/variable/tests/session_test.go`:
- Around line 359-363: The test currently checks that "Session_connect_attrs"
(attrsIdx) appears before the SQL but does not ensure it appears after
"Storage_from_mpp"; update the assertions to locate storageIdx :=
strings.Index(logString, "Storage_from_mpp") and then require that attrsIdx >
storageIdx and attrsIdx < sqlIdx (using attrsIdx, storageIdx, sqlIdx and the
existing logString/sql variables) so the ordering "Storage_from_mpp" ->
"Session_connect_attrs" -> SQL is enforced.
---
Nitpick comments:
In `@pkg/server/internal/parse/parse.go`:
- Around line 136-141: The slice operation at row := data[offset :
offset+int(num)] can panic if num exceeds remaining bytes even when under the 1
MiB limit; before slicing, check that offset+int(num) <= len(data) and if not
return mysql.ErrMalformPacket (or the package's equivalent) instead of letting a
panic occur, preserving existing behavior for parseAttrs and using the same
local symbols (num, offset, intOff, parseAttrs) to locate and harden the bounds
check.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
pkg/executor/adapter_slow_log.gopkg/executor/slow_query.gopkg/executor/slow_query_test.gopkg/infoschema/tables.gopkg/server/conn_test.gopkg/server/internal/parse/parse.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/session_test.go
💤 Files with no reviewable changes (1)
- pkg/sessionctx/variable/noop.go
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #66617 +/- ##
================================================
+ Coverage 77.7862% 79.4421% +1.6558%
================================================
Files 2022 1968 -54
Lines 554688 542118 -12570
================================================
- Hits 431471 430670 -801
+ Misses 121473 109997 -11476
+ Partials 1744 1451 -293
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
/retest |
|
@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. |
jiong-nba
force-pushed
the
feature/slow-log-session-attrs
branch
from
1217fe1 to
67a951b
Compare
ti-chi-bot
Bot
added
size/XXL
/retest |
|
@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. |
jiong-nba
force-pushed
the
feature/slow-log-session-attrs
branch
from
67a951b to
4e68a10
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/executor/slow_query_sql_test.go (1)
582-588: Avoid direct type assertion for JSON cell extraction in test assertions.
rows[0][0].(string)can panic if result encoding changes. A safer conversion keeps failures assertion-driven.♻️ Suggested test hardening
- attrsStr := rows[0][0].(string) + attrsStr := fmt.Sprint(rows[0][0])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/slow_query_sql_test.go` around lines 582 - 588, The test currently does a direct type assertion rows[0][0].(string) which can panic if the DB driver returns []byte or another type; change to a safe conversion such as using fmt.Sprint(rows[0][0]) (or convert []byte with string(...) after checking type) and assign that to attrsStr before doing require.Contains checks, and add a require.NotNil(rows[0][0]) at the top to keep failures assertion-driven; update imports to include fmt if needed and modify the assertions in the test that reference attrsStr accordingly.pkg/server/conn_test.go (1)
2439-2447: Strengthen the large-payload assertion to prove truncation actually happened.This subtest currently validates only
ConnectAttrsLongestSeen, which can still pass if attribute parsing falls into the warning/ignore path. Add a direct truncation assertion (_truncatedmarker orConnectAttrsLostincrement) to pin behavior.Suggested assertion hardening
err = parse.HandshakeResponseBody(context.Background(), &p, data, offset) require.NoError(t, err) + _, truncated := p.Attrs["_truncated"] + require.True(t, truncated, "expected truncation marker for oversized attrs") + require.Equal(t, int64(1), vardef.ConnectAttrsLost.Load()) + // Attrs are truncated (totalSize=70001 > effectiveLimit=65536, because // limit=-1 maps to effectiveLimit=65536 internally), but LongestSeen // should NOT be updated because totalSize >= 64KB.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/conn_test.go` around lines 2439 - 2447, The test only checks vardef.ConnectAttrsLongestSeen but not that attributes were actually truncated; after calling parse.HandshakeResponseBody(context.Background(), &p, data, offset) add an assertion that proves truncation occurred by checking the explicit truncation signal: either assert the parsed payload's truncation marker (p.Attrs._truncated or equivalent field on the parsed attrs) is true or assert vardef.ConnectAttrsLost was incremented as expected (e.g., compared to its prior value), so the subtest verifies both LongestSeen unchanged and that truncation actually happened.
🤖 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/sessionctx/variable/sysvar.go`:
- Around line 684-693: The sysvar vardef.PerfSchemaSessionConnectAttrsSize lacks
boundary unit tests and SQL integration tests; add unit tests that exercise
SetGlobal/Set logic for values 0 and 65536 (in addition to -1 and small values)
by invoking SetGlobal (which uses TidbOptInt64 and
vardef.ConnectAttrsSize.Store/Load) and asserting the stored value and
truncation/parse behavior, and add integration tests that run SQL like SET
performance_schema_session_connect_attrs_size = <value> and then verify
session-visible behavior (e.g., actual connect attrs size via SELECT or session
variables) to ensure the sysvar plumbing (GetGlobal/SetGlobal) works end-to-end.
---
Nitpick comments:
In `@pkg/executor/slow_query_sql_test.go`:
- Around line 582-588: The test currently does a direct type assertion
rows[0][0].(string) which can panic if the DB driver returns []byte or another
type; change to a safe conversion such as using fmt.Sprint(rows[0][0]) (or
convert []byte with string(...) after checking type) and assign that to attrsStr
before doing require.Contains checks, and add a require.NotNil(rows[0][0]) at
the top to keep failures assertion-driven; update imports to include fmt if
needed and modify the assertions in the test that reference attrsStr
accordingly.
In `@pkg/server/conn_test.go`:
- Around line 2439-2447: The test only checks vardef.ConnectAttrsLongestSeen but
not that attributes were actually truncated; after calling
parse.HandshakeResponseBody(context.Background(), &p, data, offset) add an
assertion that proves truncation occurred by checking the explicit truncation
signal: either assert the parsed payload's truncation marker (p.Attrs._truncated
or equivalent field on the parsed attrs) is true or assert
vardef.ConnectAttrsLost was incremented as expected (e.g., compared to its prior
value), so the subtest verifies both LongestSeen unchanged and that truncation
actually happened.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
pkg/executor/adapter_slow_log.gopkg/executor/slow_query.gopkg/executor/slow_query_sql_test.gopkg/executor/slow_query_test.gopkg/infoschema/tables.gopkg/server/conn_test.gopkg/server/internal/parse/parse.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/session_test.go
💤 Files with no reviewable changes (1)
- pkg/sessionctx/variable/noop.go
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/executor/adapter_slow_log.go
- pkg/executor/slow_query.go
- pkg/sessionctx/variable/tests/session_test.go
- pkg/sessionctx/vardef/tidb_vars.go
- pkg/executor/slow_query_test.go
- pkg/infoschema/tables.go
- pkg/sessionctx/variable/statusvar.go
- pkg/sessionctx/variable/slow_log.go
|
/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. |
|
/retest |
|
@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. |
|
/retest |
|
/retest |
…parsing and tests
jiong-nba
force-pushed
the
feature/slow-log-session-attrs
branch
from
c5f34d9 to
8907a87
Compare
|
/needs-cherry-pick-release-8.5 |
|
/cherry-pick release-8.5 |
ti-chi-bot
Bot
added
the
needs-cherry-pick-release-8.5
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
Bot
removed
the
needs-cherry-pick-release-8.5
What problem does this PR solve?
Issue Number: close #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.What changed and how does it work?
This PR introduces the feature to inject and log
SESSION_CONNECT_ATTRSinto the slow query log and exposes them through system tables: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.Check List
Tests
Environment and connection settings
unistore127.0.0.1:4000127.0.0.1:10080/tmp/tidb-slow-attrs-demo.logmysql-connector-python 9.6.0127.0.0.14000rootconn_attrs(normal case):{"app_name":"slowlog_demo","client_case":"attrs_normal"}conn_attrs(truncation case):{"app_name":"slowlog_demo","client_case":"attrs_trunc","extra":"x"*256}Variables used in test
Test SQL
Virtual table query SQL
Observed output: information_schema.slow_query
####Observed output: information_schema.cluster_slow_query
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Tests