Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion internal/locate/region_request.go
Original file line number Diff line number Diff line change
Expand Up @@ -1015,6 +1015,17 @@ func (s *sendReqState) next() (done bool) {
}
}

if len(req.Context.TraceId) == 0 {
if trace.IsCategoryEnabled(trace.CategoryDevDebug) {
trace.TraceEvent(bo.GetCtx(), trace.CategoryDevDebug, "send request with trace_id missing",
zap.Stack("stack"))

// If trace ID is not set, it may indicate the caller is not using client-go properly.
// Print the stack trace to capture all those cases.
trace.CheckFlightRecorderDumpTrigger(bo.GetCtx(), "dump_trigger.suspicious_event.dev_debug", "send_request_trace_id_missing")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a dependency of TiDB, which we should try our best to avoid I think.

One possible way I can think of to improve it is to make it something like NullTraceIdHook and let the adapter in TiDB translate it to CheckFlightRecorderDumpTrigger("dump_trigger.suspicious_event.dev_debug", "send_request_trace_id_missing").

If there will be more dump triggers, a more flexible design is needed. Just like how we have client-go categories and its translation to tidb categories.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a dependency of TiDB, it is a hook just as you described. @ekexium
If TiDB set this function, then the hook is called, otherwise it noop function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a code dependency but a knowledge dependency. If we don't go to TiDB code we cannot understand what "dump_trigger.suspicious_event.dev_debug" is.

}
}

if _, err := util.EvalFailpoint("beforeSendReqToRegion"); err == nil {
if hook := bo.GetCtx().Value("sendReqToRegionHook"); hook != nil {
h := hook.(func(*tikvrpc.Request))
Expand Down Expand Up @@ -1165,7 +1176,6 @@ func (s *sendReqState) send() (canceled bool) {
fields := []zap.Field{
zap.Stringer("cmd", req.Type),
zap.Duration("latency", rpcDuration),
zap.Bool("success", s.vars.err == nil && s.vars.resp != nil),
}
if s.vars.err != nil {
fields = append(fields, zap.Error(s.vars.err))
Expand Down
32 changes: 29 additions & 3 deletions trace/trace.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ const (
CategoryKVRequest
// CategoryRegionCache traces region cache operations and PD lookups.
CategoryRegionCache
// CategoryDevDebug is used for development and debugging purposes.
CategoryDevDebug
)

// TraceEventFunc is the function signature for recording trace events.
Expand All @@ -55,21 +57,45 @@ func noopTraceControlExtractor(context.Context) TraceControlFlags {
return FlagTiKVCategoryRequest
}

// CheckFlightRecorderDumpTriggerFunc is the function signature for checking whether flight recorder should trigger a dump.
type CheckFlightRecorderDumpTriggerFunc func(ctx context.Context, triggerCanonicalName string, val any)

func noopCheckFlightRecorderDumpTrigger(ctx context.Context, triggerCanonicalName string, val any) {}

// Global function pointers stored independently
var (
globalTraceEventFunc atomic.Pointer[TraceEventFunc]
globalIsCategoryEnabledFunc atomic.Pointer[IsCategoryEnabledFunc]
globalTraceControlExtractorFunc atomic.Pointer[TraceControlExtractorFunc]
globalTraceEventFunc atomic.Pointer[TraceEventFunc]
globalIsCategoryEnabledFunc atomic.Pointer[IsCategoryEnabledFunc]
globalTraceControlExtractorFunc atomic.Pointer[TraceControlExtractorFunc]
globalCheckFlightRecorderDumpTriggerFunc atomic.Pointer[CheckFlightRecorderDumpTriggerFunc]
)

func init() {
// Set default no-op implementations
defaultTraceEvent := TraceEventFunc(noopTraceEvent)
defaultIsCategoryEnabled := IsCategoryEnabledFunc(noopIsCategoryEnabled)
defaultTraceControlExtractor := TraceControlExtractorFunc(noopTraceControlExtractor)
defaultCheckFlightRecorderDumpTrigger := CheckFlightRecorderDumpTriggerFunc(noopCheckFlightRecorderDumpTrigger)
globalTraceEventFunc.Store(&defaultTraceEvent)
globalIsCategoryEnabledFunc.Store(&defaultIsCategoryEnabled)
globalTraceControlExtractorFunc.Store(&defaultTraceControlExtractor)
globalCheckFlightRecorderDumpTriggerFunc.Store(&defaultCheckFlightRecorderDumpTrigger)
}

// CheckFlightRecorderDumpTrigger checks whether flight recorder should trigger a dump.
func CheckFlightRecorderDumpTrigger(ctx context.Context, triggerCanonicalName string, val any) {
fn := *globalCheckFlightRecorderDumpTriggerFunc.Load()
fn(ctx, triggerCanonicalName, val)
}

// SetCheckFlightRecorderDumpTriggerFunc sets the flight recorder dump trigger function.
// This is typically called once during application initialization (e.g., by TiDB).
// Passing nil will use a no-op implementation.
func SetCheckFlightRecorderDumpTriggerFunc(fn CheckFlightRecorderDumpTriggerFunc) {
if fn == nil {
fn = noopCheckFlightRecorderDumpTrigger
}
globalCheckFlightRecorderDumpTriggerFunc.Store(&fn)
}

// SetTraceEventFunc registers the trace event handler function.
Expand Down
22 changes: 22 additions & 0 deletions trace/trace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,3 +142,25 @@ func TestTraceIDContext(t *testing.T) {
extractedID2 := TraceIDFromContext(ctxWithTrace2)
require.Equal(t, traceID2, extractedID2)
}

func TestCheckFlightRecorderDumpTrigger(t *testing.T) {
original := globalCheckFlightRecorderDumpTriggerFunc.Load()
defer func() {
globalCheckFlightRecorderDumpTriggerFunc.Store(original)
}()

// Test default behavior (returns false)
ctx := context.Background()
CheckFlightRecorderDumpTrigger(ctx, "just cover some code", 42)

// Test custom behavior
var covered bool
mock := func(ctx context.Context, triggerName string, val any) {
covered = true
}
SetCheckFlightRecorderDumpTriggerFunc(CheckFlightRecorderDumpTriggerFunc(mock))

require.False(t, covered)
CheckFlightRecorderDumpTrigger(ctx, "custom trigger", 123)
require.True(t, covered)
}
Loading