Skip to content

Commit

Permalink
Merge #132741
Browse files Browse the repository at this point in the history
132741: kvprober: reuse TracingEnabled config variable r=kvoli,joshimhoff a=VishalJaishankar

There was a panic caused due to null reference from variable `finishAndGetRecording` 
see #131672

This was happening because we check the TracingEnabled config twice and in a corner case the flag was false on the first call and enabled when the second call was made causing the null reference.

This PR checks the flag once and reuses it.

Fixes: #131672

Epic: none
Release note: None

Co-authored-by: Vishal Jaishankar <[email protected]>
  • Loading branch information
craig[bot] and VishalJaishankar committed Oct 24, 2024
2 parents 9c1d89e + 2444856 commit 21f306c
Showing 1 changed file with 6 additions and 4 deletions.
10 changes: 6 additions & 4 deletions pkg/kv/kvprober/kvprober.go
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,8 @@ func (p *Prober) readProbeImpl(ctx context.Context, ops proberOpsI, txns proberT
var finishAndGetRecording func() tracingpb.Recording
var probeCtx = ctx

if tracingEnabled.Get(&p.settings.SV) {
isTracingEnabled := tracingEnabled.Get(&p.settings.SV)
if isTracingEnabled {
probeCtx, finishAndGetRecording = tracing.ContextWithRecordingSpan(ctx, p.tracer, "read probe")
}

Expand Down Expand Up @@ -425,7 +426,7 @@ func (p *Prober) readProbeImpl(ctx context.Context, ops proberOpsI, txns proberT

d := timeutil.Since(start)
// Extract leaseholder information from the trace recording if enabled.
if tracingEnabled.Get(&p.settings.SV) {
if isTracingEnabled {
leaseholder := p.returnLeaseholderInfo(finishAndGetRecording())
ctx = logtags.AddTag(ctx, "leaseholder", leaseholder)
log.Health.Infof(ctx, "kv.Get(%s), r=%v having likely leaseholder=%s returned success in %v", step.Key, step.RangeID, leaseholder, d)
Expand All @@ -451,7 +452,8 @@ func (p *Prober) writeProbeImpl(ctx context.Context, ops proberOpsI, txns prober
var finishAndGetRecording func() tracingpb.Recording
var probeCtx = ctx

if tracingEnabled.Get(&p.settings.SV) {
isTracingEnabled := tracingEnabled.Get(&p.settings.SV)
if isTracingEnabled {
probeCtx, finishAndGetRecording = tracing.ContextWithRecordingSpan(ctx, p.tracer, "write probe")
}

Expand Down Expand Up @@ -506,7 +508,7 @@ func (p *Prober) writeProbeImpl(ctx context.Context, ops proberOpsI, txns prober

d := timeutil.Since(start)
// Extract leaseholder information from the trace recording if enabled.
if tracingEnabled.Get(&p.settings.SV) {
if isTracingEnabled {
leaseholder := p.returnLeaseholderInfo(finishAndGetRecording())
ctx = logtags.AddTag(ctx, "leaseholder", leaseholder)
log.Health.Infof(ctx, "kv.Txn(Put(%s); Del(-)), r=%v having likely leaseholder=%s returned success in %v", step.Key, step.RangeID, leaseholder, d)
Expand Down

0 comments on commit 21f306c

Please sign in to comment.