*: refactor traceevent to make some concept clear#65562
Conversation
ti-chi-bot
Bot
added
the
release-note-none
ti-chi-bot
Bot
added
the
size/XL
|
Hi @tiancaiamao. 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. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the traceevent package to clarify naming and relationships between trace event concepts. The refactoring addresses inconsistencies that were introduced in PR #65317 where two different IsEnabled implementations existed.
Changes:
- Renamed
Tracestruct toTraceBufto better reflect its purpose as a buffer for trace events - Replaced
tracing.WithFlightRecorderwithtraceevent.WithTraceBuffor cleaner API semantics - Consolidated
IsEnabledimplementations by makingtracing.IsEnableddelegate totraceevent.IsEnabled - Moved TraceID storage directly into TraceBuf instead of context and removed intermediary functions
- Removed the
rand32field from TraceBuf, simplifying the trace ID generation logic
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/tracing/util.go | Renamed Sink to TraceBuf interface, delegated IsEnabled to traceevent package, removed ExtractTraceID function |
| pkg/util/traceevent/traceevent.go | Removed TraceIDFromContext and ContextWithTraceID functions, updated TraceEvent to use traceBuf.TraceID, simplified GenerateTraceID |
| pkg/util/traceevent/flightrecorder.go | Renamed Trace to TraceBuf, added TraceID field, removed rand32 field, updated all methods accordingly |
| pkg/util/traceevent/adapter.go | Updated to use getTraceBuf helper instead of tracing.GetSink |
| pkg/util/traceevent/test/integration_test.go | Updated test calls from NewTrace/WithFlightRecorder to NewTraceBuf/WithTraceBuf |
| pkg/util/traceevent/adapter_test.go | Updated all test cases to use NewTraceBuf and WithTraceBuf |
| pkg/session/session.go | Updated ExecuteInternal to use GetTraceBuf, NewTraceBuf, and WithTraceBuf |
| pkg/server/conn.go | Updated connection handling to use NewTraceBuf and WithTraceBuf |
| pkg/dxf/framework/taskexecutor/task_executor.go | Updated to use NewTraceBuf and WithTraceBuf |
| pkg/dxf/framework/taskexecutor/manager.go | Updated Manager to use TraceBuf type and WithTraceBuf |
| pkg/dxf/framework/scheduler/scheduler_manager.go | Updated scheduler loops to use NewTraceBuf and WithTraceBuf |
| pkg/dxf/framework/scheduler/nodes.go | Updated node refresh loop to use NewTraceBuf and WithTraceBuf |
| pkg/ddl/notifier/subscribe.go | Updated DDL notifier to use NewTraceBuf and WithTraceBuf |
| pkg/ddl/job_scheduler.go | Updated to use TraceBuf type and set TraceID directly instead of via context |
| pkg/ddl/executor.go | Updated to extract TraceID directly from TraceBuf instead of using TraceIDFromContext |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #65562 +/- ##
================================================
- Coverage 77.7488% 77.2305% -0.5184%
================================================
Files 1959 1942 -17
Lines 543393 543407 +14
================================================
- Hits 422482 419676 -2806
- Misses 120070 123727 +3657
+ Partials 841 4 -837
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
|
@tiancaiamao: 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. |
| zap.Stringer("category", categories), | ||
| zap.Any("mapping", compiled.nameMapping), | ||
| zap.Uint64s("truthTable", ret.truthTable)) | ||
|
|
There was a problem hiding this comment.
This blank line appears to be an unintentional addition that creates inconsistent spacing in the code. The line immediately following is a function call that logically belongs together with the preceding log statement without extra spacing.
|
/retest |
|
@tiancaiamao: 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 |
|
@tiancaiamao: 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. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| traceBuf := tmp.(TraceBuf) | ||
| event := Event{ | ||
| Category: General, | ||
| Name: regionType, |
There was a problem hiding this comment.
The Event created in StartRegion is missing the TraceID field. Unlike TraceEvent in traceevent.go which sets TraceID from traceBuf.GetTraceID(), this event has an uninitialized TraceID field. This creates inconsistency in event data - some events will have TraceID and others won't. Consider adding the TraceID field by getting it from the TraceBuf interface (if GetTraceID method is exposed) or passing it as a parameter.
| Name: regionType, | |
| Name: regionType, | |
| TraceID: traceBuf.GetTraceID(), |
| // Record to flight recorder if enabled (base or full mode). | ||
| if recorderEnabled.Load() { | ||
| // TODO: clean up here | ||
| if recorder := FlightRecorder(); recorder != nil { |
There was a problem hiding this comment.
The TODO comment says "clean up here" but doesn't provide context about what needs to be cleaned up. Given that this PR is about refactoring to make concepts clearer, consider either removing this TODO if it's no longer relevant, or expanding it to be more specific about what cleanup is needed (e.g., "TODO: consolidate duplicate recording to traceBuf and FlightRecorder" or "TODO: remove global FlightRecorder after TraceBuf consolidation is complete").
| if tmp := GetSink(ctx); tmp != nil { | ||
| sink := tmp.(Sink) | ||
| if tmp := GetTraceBuf(ctx); tmp != nil { | ||
| traceBuf := tmp.(TraceBuf) |
There was a problem hiding this comment.
The type assertion tmp.(TraceBuf) will panic if it fails. Consider using the comma-ok idiom for safer type assertion: traceBuf, ok := tmp.(TraceBuf); if !ok { return ret }. This pattern is already used in other places in the codebase (e.g., pkg/ddl/executor.go:6809).
| traceBuf := tmp.(TraceBuf) | |
| traceBuf, ok := tmp.(TraceBuf) | |
| if !ok { | |
| return ret | |
| } |
|
/retest |
|
@tiancaiamao: 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. |
| if recorderEnabled.Load() { | ||
| // TODO: clean up here | ||
| if recorder := FlightRecorder(); recorder != nil { | ||
| recorder.Record(ctx, event) | ||
| } |
There was a problem hiding this comment.
The PR is already big enough and we'd better clean up step by step
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ekexium 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 |
[LGTM Timeline notifier]Timeline:
|
|
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 (3)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughMigrates tracing from a Flight Recorder-based system to a TraceBuf-based approach. Introduces Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Session
participant TraceBuf as TraceBuf (ctx)
participant DDL
participant JobScheduler
participant TiKV as TiKV client-go
Client->>Session: Execute statement (ctx)
Session->>TraceBuf: GetTraceBuf(ctx) / NewTraceBuf()
Session->>TraceBuf: SetTraceID(...)
Session->>DDL: schedule DDL job (ctx with TraceBuf)
DDL->>JobScheduler: deliver job (attach TraceBuf)
JobScheduler->>TraceBuf: trace.SetTraceID(...)
JobScheduler->>TiKV: clientgotrace.ContextWithTraceID(ctx) -> TiKV request
JobScheduler->>TraceBuf: DiscardOrFlush(ctx)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Command failed 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/session/txnmanager.go (1)
157-253:⚠️ Potential issue | 🟡 MinorPlease add/confirm targeted tests for txn lifecycle trace-context handoff.
This change alters session txn lifecycle behavior (
EnterNewTxn/OnStmtStart/OnTxnEnd) and should be covered with targeted package tests and SQL integration scenarios.# Suggested local verification commands go test ./pkg/session -run 'Test.*Txn.*|Test.*TxnManager.*' -tags=intest,deadlock # plus your repo's SQL integration suite for BEGIN/COMMIT/ROLLBACK and implicit txn pathsAs per coding guidelines,
pkg/session/**: "For session variables or protocol behavior changes, run targeted package tests plus SQL integration tests for user-visible behavior".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/session/txnmanager.go` around lines 157 - 253, Add targeted tests that verify trace-context handoff across the txn lifecycle implemented in txnManager: specifically test EnterNewTxn, OnStmtStart, and OnTxnEnd to ensure traceCtx set in EnterNewTxn is propagated to traceevent.TraceEvent calls and that OnStmtStart/OnTxnEnd behave correctly when ctxProvider is nil or present; create unit tests in pkg/session that instantiate a txnManager (or use exported helpers) and assert traceevent.TraceEvent is invoked with the expected context/fields and that errors are returned when ctxProvider is missing, and add integration SQL tests exercising BEGIN/COMMIT/ROLLBACK and implicit txn paths to cover real session behavior (refer to txnManager.traceCtx, txnManager.ctxProvider, EnterNewTxn, OnStmtStart, OnTxnEnd when writing assertions).
🤖 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/ddl/executor.go`:
- Around line 6767-6778: The code stores traceBuf.GetTraceID() directly into
job.TraceInfo.TraceID causing aliasing; change the assignment in the block that
builds job := jobW.Job so you copy/clone the returned byte slice from
traceBuf.GetTraceID() (traceBuf, GetTraceID, TraceBuf, SetTraceID, Reset,
job.TraceInfo.TraceID) into a new slice before assigning it to
job.TraceInfo.TraceID to avoid later mutations or resets of the TraceBuf
invalidating the stored value.
In `@pkg/ddl/job_scheduler.go`:
- Around line 569-575: The getJobRunCtx function currently dereferences
traceInfo.TraceID unconditionally; update getJobRunCtx to guard access by first
checking that traceInfo != nil before reading TraceID (e.g., change the
condition to if traceInfo != nil && len(traceInfo.TraceID) > 0) and only then
call trace.SetTraceID and clientgotrace.ContextWithTraceID to build newCtx; this
prevents panics when jobW.TraceInfo is nil (deliveryJob may pass nil) while
preserving the existing behavior of jobContext and trace handling.
In `@pkg/session/session.go`:
- Around line 1835-1838: The current guard uses tracing.GetTraceBuf(ctx) to
decide whether to create a fallback trace buffer, but that can be fooled by a
mismatched context value; change the check to ensure the value is the typed
*traceevent.TraceBuf. Specifically, replace the nil check with a typed
lookup/assertion (e.g. tb, ok :=
traceevent.GetTraceBuf(ctx).(*traceevent.TraceBuf); if !ok || tb == nil { ... })
so that NewTraceBuf/WithTraceBuf/DiscardOrFlush only run when no real
*traceevent.TraceBuf is present.
In `@pkg/util/traceevent/flightrecorder.go`:
- Around line 39-52: GetTraceID and SetTraceID currently expose and store the
same backing slice (t.traceID) allowing callers to mutate the slice across API
boundaries; update GetTraceID to return a copy of t.traceID and update
SetTraceID to store a copy of the incoming slice (while still using t.mu for
RLock/Lock around reads/writes). Locate the methods TraceBuf.GetTraceID and
TraceBuf.SetTraceID and ensure both create new slices and copy contents when
reading or assigning t.traceID to prevent external mutation and cross-goroutine
races.
In `@pkg/util/traceevent/traceevent.go`:
- Around line 237-253: The current instant event path still obtains a TraceBuf
and calls traceBuf.Record even when tracing is disabled (ModeOff), causing
unnecessary overhead; update the logic in the function that builds Event (using
GetTraceBuf, Event, copyFieldsWithCapacity) to check the global/ctx tracing mode
(ModeOff) before allocating/copying fields and before calling traceBuf.Record,
and short-circuit/return early when tracing is off so no Event is constructed or
recorded.
- Around line 537-540: The log currently prints malformed trace IDs as raw
string bytes using string(event.TraceID) (see the branch that calls
logutil.BgLogger().Info and the zap.String("trace_id", ...)); change it to log
the hex representation by using hex.EncodeToString(event.TraceID) instead so
non-printable bytes are rendered consistently, and add the encoding/hex import
if missing.
---
Outside diff comments:
In `@pkg/session/txnmanager.go`:
- Around line 157-253: Add targeted tests that verify trace-context handoff
across the txn lifecycle implemented in txnManager: specifically test
EnterNewTxn, OnStmtStart, and OnTxnEnd to ensure traceCtx set in EnterNewTxn is
propagated to traceevent.TraceEvent calls and that OnStmtStart/OnTxnEnd behave
correctly when ctxProvider is nil or present; create unit tests in pkg/session
that instantiate a txnManager (or use exported helpers) and assert
traceevent.TraceEvent is invoked with the expected context/fields and that
errors are returned when ctxProvider is missing, and add integration SQL tests
exercising BEGIN/COMMIT/ROLLBACK and implicit txn paths to cover real session
behavior (refer to txnManager.traceCtx, txnManager.ctxProvider, EnterNewTxn,
OnStmtStart, OnTxnEnd when writing assertions).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
pkg/ddl/BUILD.bazelpkg/ddl/executor.gopkg/ddl/job_scheduler.gopkg/ddl/notifier/subscribe.gopkg/dxf/framework/scheduler/nodes.gopkg/dxf/framework/scheduler/scheduler_manager.gopkg/dxf/framework/taskexecutor/manager.gopkg/dxf/framework/taskexecutor/task_executor.gopkg/server/conn.gopkg/session/session.gopkg/session/txnmanager.gopkg/util/traceevent/adapter.gopkg/util/traceevent/adapter_test.gopkg/util/traceevent/flightrecorder.gopkg/util/traceevent/test/integration_test.gopkg/util/traceevent/traceevent.gopkg/util/traceevent/traceevent_test.gopkg/util/tracing/BUILD.bazelpkg/util/tracing/util.gopkg/util/tracing/util_test.go
💤 Files with no reviewable changes (1)
- pkg/util/tracing/BUILD.bazel
| if sink := tracing.GetTraceBuf(ctx); sink == nil { | ||
| trace := traceevent.NewTraceBuf() | ||
| ctx = traceevent.WithTraceBuf(ctx, trace) | ||
| defer trace.DiscardOrFlush(ctx) |
There was a problem hiding this comment.
Use typed TraceBuf lookup for the fallback guard.
Line 1835 checks raw context presence, not typed *traceevent.TraceBuf. If a mismatched value is present, fallback creation is skipped and events can be dropped.
🔧 Proposed fix
func (s *session) ExecuteInternal(ctx context.Context, sql string, args ...any) (rs sqlexec.RecordSet, err error) {
- if sink := tracing.GetTraceBuf(ctx); sink == nil {
+ if traceevent.GetTraceBuf(ctx) == nil {
trace := traceevent.NewTraceBuf()
ctx = traceevent.WithTraceBuf(ctx, trace)
defer trace.DiscardOrFlush(ctx)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if sink := tracing.GetTraceBuf(ctx); sink == nil { | |
| trace := traceevent.NewTraceBuf() | |
| ctx = traceevent.WithTraceBuf(ctx, trace) | |
| defer trace.DiscardOrFlush(ctx) | |
| if traceevent.GetTraceBuf(ctx) == nil { | |
| trace := traceevent.NewTraceBuf() | |
| ctx = traceevent.WithTraceBuf(ctx, trace) | |
| defer trace.DiscardOrFlush(ctx) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/session/session.go` around lines 1835 - 1838, The current guard uses
tracing.GetTraceBuf(ctx) to decide whether to create a fallback trace buffer,
but that can be fooled by a mismatched context value; change the check to ensure
the value is the typed *traceevent.TraceBuf. Specifically, replace the nil check
with a typed lookup/assertion (e.g. tb, ok :=
traceevent.GetTraceBuf(ctx).(*traceevent.TraceBuf); if !ok || tb == nil { ... })
so that NewTraceBuf/WithTraceBuf/DiscardOrFlush only run when no real
*traceevent.TraceBuf is present.
| // GetTraceID gets the traceID field. | ||
| func (t *TraceBuf) GetTraceID() []byte { | ||
| t.mu.RLock() | ||
| traceID := t.traceID | ||
| t.mu.RUnlock() | ||
| return traceID | ||
| } | ||
|
|
||
| // SetTraceID sets the traceID field. | ||
| func (t *TraceBuf) SetTraceID(traceID []byte) { | ||
| t.mu.Lock() | ||
| t.traceID = traceID | ||
| t.mu.Unlock() | ||
| } |
There was a problem hiding this comment.
Avoid sharing mutable traceID slices across API boundaries.
Lines 42 and 50 expose/store the same backing slice. External mutation can corrupt buffered trace IDs and create cross-goroutine races.
🛡️ Proposed fix
func (t *TraceBuf) GetTraceID() []byte {
t.mu.RLock()
- traceID := t.traceID
+ traceID := slices.Clone(t.traceID)
t.mu.RUnlock()
return traceID
}
// SetTraceID sets the traceID field.
func (t *TraceBuf) SetTraceID(traceID []byte) {
t.mu.Lock()
- t.traceID = traceID
+ t.traceID = slices.Clone(traceID)
t.mu.Unlock()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/util/traceevent/flightrecorder.go` around lines 39 - 52, GetTraceID and
SetTraceID currently expose and store the same backing slice (t.traceID)
allowing callers to mutate the slice across API boundaries; update GetTraceID to
return a copy of t.traceID and update SetTraceID to store a copy of the incoming
slice (while still using t.mu for RLock/Lock around reads/writes). Locate the
methods TraceBuf.GetTraceID and TraceBuf.SetTraceID and ensure both create new
slices and copy contents when reading or assigning t.traceID to prevent external
mutation and cross-goroutine races.
| traceBuf := GetTraceBuf(ctx) | ||
| if traceBuf == nil { | ||
| return | ||
| } | ||
|
|
||
| // Defensive copy prevents corruption from caller's buffer reuse. | ||
| // Reserve extra capacity for LogSink to append metadata without allocation. | ||
| event := Event{ | ||
| Category: category, | ||
| Name: name, | ||
| Phase: tracing.PhaseInstant, | ||
| Timestamp: time.Now(), | ||
| TraceID: TraceIDFromContext(ctx), | ||
| TraceID: traceBuf.GetTraceID(), | ||
| Fields: copyFieldsWithCapacity(fields, 3), | ||
| } | ||
| traceBuf.Record(ctx, event) | ||
|
|
There was a problem hiding this comment.
ModeOff can still buffer events into TraceBuf.
This path records into TraceBuf even when both recorder/logging are disabled, which conflicts with the ModeOff contract and still incurs tracing overhead.
Suggested fix
func TraceEvent(ctx context.Context, category TraceCategory, name string, fields ...zap.Field) {
if !IsEnabled(category) {
return
}
+ if !recorderEnabled.Load() && !loggingEnabled.Load() {
+ return
+ }
traceBuf := GetTraceBuf(ctx)
if traceBuf == nil {
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| traceBuf := GetTraceBuf(ctx) | |
| if traceBuf == nil { | |
| return | |
| } | |
| // Defensive copy prevents corruption from caller's buffer reuse. | |
| // Reserve extra capacity for LogSink to append metadata without allocation. | |
| event := Event{ | |
| Category: category, | |
| Name: name, | |
| Phase: tracing.PhaseInstant, | |
| Timestamp: time.Now(), | |
| TraceID: TraceIDFromContext(ctx), | |
| TraceID: traceBuf.GetTraceID(), | |
| Fields: copyFieldsWithCapacity(fields, 3), | |
| } | |
| traceBuf.Record(ctx, event) | |
| func TraceEvent(ctx context.Context, category TraceCategory, name string, fields ...zap.Field) { | |
| if !IsEnabled(category) { | |
| return | |
| } | |
| if !recorderEnabled.Load() && !loggingEnabled.Load() { | |
| return | |
| } | |
| traceBuf := GetTraceBuf(ctx) | |
| if traceBuf == nil { | |
| return | |
| } | |
| // Defensive copy prevents corruption from caller's buffer reuse. | |
| // Reserve extra capacity for LogSink to append metadata without allocation. | |
| event := Event{ | |
| Category: category, | |
| Name: name, | |
| Phase: tracing.PhaseInstant, | |
| Timestamp: time.Now(), | |
| TraceID: traceBuf.GetTraceID(), | |
| Fields: copyFieldsWithCapacity(fields, 3), | |
| } | |
| traceBuf.Record(ctx, event) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/util/traceevent/traceevent.go` around lines 237 - 253, The current
instant event path still obtains a TraceBuf and calls traceBuf.Record even when
tracing is disabled (ModeOff), causing unnecessary overhead; update the logic in
the function that builds Event (using GetTraceBuf, Event,
copyFieldsWithCapacity) to check the global/ctx tracing mode (ModeOff) before
allocating/copying fields and before calling traceBuf.Record, and
short-circuit/return early when tracing is off so no Event is constructed or
recorded.
| } else if len(event.TraceID) > 0 { | ||
| logutil.BgLogger().Info("wrong traceid format", | ||
| zap.String("trace_id", string(event.TraceID))) | ||
| } |
There was a problem hiding this comment.
Log malformed trace IDs in hex, not raw string bytes.
string(event.TraceID) can emit non-printable bytes and make diagnostics noisy. Use hex.EncodeToString for consistency.
Suggested fix
} else if len(event.TraceID) > 0 {
logutil.BgLogger().Info("wrong traceid format",
- zap.String("trace_id", string(event.TraceID)))
+ zap.String("trace_id", hex.EncodeToString(event.TraceID)))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/util/traceevent/traceevent.go` around lines 537 - 540, The log currently
prints malformed trace IDs as raw string bytes using string(event.TraceID) (see
the branch that calls logutil.BgLogger().Info and the zap.String("trace_id",
...)); change it to log the hex representation by using
hex.EncodeToString(event.TraceID) instead so non-printable bytes are rendered
consistently, and add the encoding/hex import if missing.
|
/retest |
|
@tiancaiamao: 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. |
- Clone TraceID bytes before storing in executor.go to prevent aliasing - Add nil check for traceInfo in job_scheduler.go to prevent panic - Use typed TraceBuf lookup in session.go for proper type checking - Make GetTraceID/SetTraceID clone slices to prevent data races - Check mode before building Event to avoid unnecessary overhead - Fix trace_id logging to use hex.EncodeToString for consistency - Add global enabledCategories variable and SetEnabledCategories function - Make GenerateTraceID a pure function by moving rand call out - Remove unused math/rand/v2 import from traceevent.go Also includes improvements from master merge to TraceBuf interface: - Add GetTraceID() []byte to TraceBuf interface - Remove intermediate traceIDProvider interface Co-Authored-By: Claude <noreply@anthropic.com>
|
@tiancaiamao: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
What problem does this PR solve?
Issue Number: ref #64008
Problem Summary:
The code in pkg/util/traceevent are quite messy now. I promise the refactor and here it is.
What changed and how does it work?
In the past, there are a lot concepts like
Sink,Trace,FlightRecorder...This refactor try to make it clearer.
The value binded with context is a
TraceBuf, all the trace events are temporary stored inside it.Whether to flush or discard it is decided by FlightRecorder's configuration, in
CheckFlightRecorderDumpTriggerCheck List
Tests
Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit