Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tracing: fix WithEventListeners option with verbose parent #98896

Merged
merged 1 commit into from
Mar 21, 2023
Merged
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
22 changes: 13 additions & 9 deletions pkg/util/tracing/span_options.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,15 @@ type spanOptions struct {

// recordingTypeExplicit is set if the WithRecording() option was used. In
// that case, spanOptions.recordingType() returns recordingTypeOpt below. If
// not set, recordingType() looks at the parent.
// not set, recordingType() looks at the parent, subject to
// minRecordingTypeOpt.
recordingTypeExplicit bool
recordingTypeOpt tracingpb.RecordingType
// minRecordingTypeOpt, if set, indicates the "minimum" recording type of
// this span (if it doesn't contradict recordingTypeOpt). If the parent has
// a more "verbose" recording type, than that type is used by
// recordingType().
minRecordingTypeOpt tracingpb.RecordingType
}

func (opts *spanOptions) parentTraceID() tracingpb.TraceID {
Expand All @@ -107,12 +113,15 @@ func (opts *spanOptions) recordingType() tracingpb.RecordingType {
return opts.recordingTypeOpt
}

recordingType := tracingpb.RecordingOff
var recordingType tracingpb.RecordingType
if !opts.Parent.empty() && !opts.Parent.IsNoop() {
recordingType = opts.Parent.i.crdb.recordingType()
} else if !opts.RemoteParent.Empty() {
recordingType = opts.RemoteParent.recordingType
}
if recordingType < opts.minRecordingTypeOpt {
recordingType = opts.minRecordingTypeOpt
}
return recordingType
}

Expand Down Expand Up @@ -472,13 +481,8 @@ var _ SpanOption = eventListenersOption{}

func (ev eventListenersOption) apply(opts spanOptions) spanOptions {
// Applying an EventListener span option implies the span has at least
// `RecordingStructured` recording type. If the span explicitly specifies a
// `RecordingVerbose` recording type via the `WithRecording(...)` option, that
// will be respected instead.
if !opts.recordingTypeExplicit {
opts.recordingTypeExplicit = true
opts.recordingTypeOpt = tracingpb.RecordingStructured
}
// `RecordingStructured` recording type.
opts.minRecordingTypeOpt = tracingpb.RecordingStructured
eventListeners := ([]EventListener)(ev)
opts.EventListeners = eventListeners
return opts
Expand Down
19 changes: 19 additions & 0 deletions pkg/util/tracing/span_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1510,3 +1510,22 @@ func TestFinishGetRecordingRace(t *testing.T) {
root.Finish()
}
}

// TestWithEventListenersAndVerboseParent verifies that if the parent has
// verbose recording and the child uses WithEventListeners option but without
// explicitly specifying the recording type, the child still has verbose
// recording.
func TestWithEventListenersAndVerboseParent(t *testing.T) {
tr := NewTracer()
parent := tr.StartSpan("parent", WithRecording(tracingpb.RecordingVerbose))
defer parent.Finish()
_, child := EnsureChildSpan(context.Background(), tr, "child", WithParent(parent), WithEventListeners())
defer child.Finish()
child.Record("foo")
require.NoError(t, CheckRecording(parent.GetConfiguredRecording(), `
=== operation:parent _unfinished:1 _verbose:1
[child]
=== operation:child _unfinished:1 _verbose:1
event:foo
`))
}