Skip to content

tracing: add ExtractTraceSpanID #550

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

Merged
merged 6 commits into from
Jul 17, 2024
Merged
Show file tree
Hide file tree
Changes from 4 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@
* [ENHANCEMENT] SpanProfiler: do less work on unsampled traces. #528
* [ENHANCEMENT] Log Middleware: if the trace is not sampled, log its ID as `trace_id_unsampled` instead of `trace_id`. #529
* [EHNANCEMENT] httpgrpc: httpgrpc Server can now use error message from special HTTP header when converting HTTP response to an error. This is useful when HTTP response body contains binary data that doesn't form valid utf-8 string, otherwise grpc would fail to marshal returned error. #531
* [ENHANCEMENT] tracing: add ExtractTraceSpanID function.
* [CHANGE] Backoff: added `Backoff.ErrCause()` which is like `Backoff.Err()` but returns the context cause if backoff is terminated because the context has been canceled. #538
* [BUGFIX] spanlogger: Support multiple tenant IDs. #59
* [BUGFIX] Memberlist: fixed corrupted packets when sending compound messages with more than 255 messages or messages bigger than 64KB. #85
Expand Down
22 changes: 18 additions & 4 deletions tracing/tracing.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,30 @@ func NewFromEnv(serviceName string, options ...jaegercfg.Option) (io.Closer, err

// ExtractTraceID extracts the trace id, if any from the context.
func ExtractTraceID(ctx context.Context) (string, bool) {
if sctx := extractJaegerContext(ctx); sctx != nil {
return sctx.TraceID().String(), true
}
return "", false
}

// ExtractTraceSpanID extracts the trace id, span id if any from the context.
func ExtractTraceSpanID(ctx context.Context) (string, string, bool) {
if sctx := extractJaegerContext(ctx); sctx != nil {
return sctx.TraceID().String(), sctx.SpanID().String(), true
}
return "", "", false
}

func extractJaegerContext(ctx context.Context) *jaeger.SpanContext {
sp := opentracing.SpanFromContext(ctx)
if sp == nil {
return "", false
return nil
}
sctx, ok := sp.Context().(jaeger.SpanContext)
if !ok {
return "", false
return nil
}

return sctx.TraceID().String(), true
return &sctx
Copy link
Contributor

Choose a reason for hiding this comment

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

By returning &sctx, jaeger.SpanContext needs to be allocated on a heap first, and then pointer to it returned, right?

Perhaps we could return just traceID and spanID directly? Or jaeger.SpanContext without the pointer.

Copy link
Contributor Author

@krajorama krajorama Jul 17, 2024

Choose a reason for hiding this comment

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

I thought about that , but that (direct return) would be duplicated code. I can definitely do a copy of the jeager.SpanContext thought, if that's preferred?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way , I don't know if the compiler is smart enough to avoid allocation on the heap

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep ExtractTraceID, and call ExtractTraceSpanID inside, just discard spanID when return result. In that case no need to refectory this extractJaegerContext but leave all logics in ExtractTraceSpanID

Copy link
Contributor Author

@krajorama krajorama Jul 17, 2024

Choose a reason for hiding this comment

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

facepalm, of course
although then your doing an extra string formatting call for the spanid

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is a fair comment, current code LGTM, approved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it did inspire me to go ahead and look at the assembly :) Got the final result by deferring the formatting to the public function , but only returning the two relevant fields from the helper function. It's the least extra instructions.

}

// ExtractSampledTraceID works like ExtractTraceID but the returned bool is only
Expand Down
47 changes: 47 additions & 0 deletions tracing/tracing_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// SPDX-License-Identifier: Apache-2.0

package tracing

import (
"context"
"testing"

"github.com/opentracing/opentracing-go"
"github.com/stretchr/testify/require"
jaeger "github.com/uber/jaeger-client-go"
)

func TestExtractTraceSpanID(t *testing.T) {
spanCtx := jaeger.NewSpanContext(jaeger.TraceID{High: 1, Low: 2}, jaeger.SpanID(3), 0, true, nil)
tracer, closer := jaeger.NewTracer("test", jaeger.NewConstSampler(true), jaeger.NewNullReporter())
defer closer.Close()
span := tracer.StartSpan("test", opentracing.ChildOf(spanCtx))

testCases := map[string]struct {
ctx context.Context
expectedOk bool
expectedTraceID string
expectedSpanID string
}{
"no trace": {
ctx: context.Background(),
expectedOk: false,
expectedTraceID: "",
expectedSpanID: "",
},
"trace": {
ctx: opentracing.ContextWithSpan(context.Background(), span),
expectedOk: true,
expectedTraceID: "00000000000000010000000000000002",
expectedSpanID: span.Context().(jaeger.SpanContext).SpanID().String(),
},
}
for name, tc := range testCases {
t.Run(name, func(t *testing.T) {
traceID, spanID, ok := ExtractTraceSpanID(tc.ctx)
require.Equal(t, tc.expectedOk, ok)
require.Equal(t, tc.expectedTraceID, traceID)
require.Equal(t, tc.expectedSpanID, spanID)
})
}
}