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.

Sign up for GitHub

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

Add if the trace was sampled to the ctxTags #184

Merged
merged 5 commits into from
Jan 18, 2019
Merged

Add if the trace was sampled to the ctxTags #184

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a6882b7
we now will not add the traceId / spanId to ctxtags if it is not sampled
domgreen Jan 4, 2019
438b3b1
update to add sampled to the tags
domgreen Jan 4, 2019
b8c75ad
code review
domgreen Jan 10, 2019
a913ddf
OT explicitly check value is true or false
domgreen Jan 17, 2019
71cd4a0
OT not add tag if unknown value
domgreen Jan 17, 2019
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
45 changes: 29 additions & 16 deletions tracing/opentracing/id_extract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
// Copyright 2017 Michal Witkowski. All Rights Reserved.
// See LICENSE for licensing terms.

package grpc_opentracing

import (
Expand All @@ -12,39 +9,55 @@ import (
)

const (
TagTraceId = "trace.traceid"
TagSpanId = "trace.spanid"
TagTraceId = "trace.traceid"
TagSpanId = "trace.spanid"
TagSampled = "trace.sampled"
JaegerNotSampledFlag = "0"
domgreen marked this conversation as resolved.
Show resolved Hide resolved
)

// hackyInjectOpentracingIdsToTags writes the given context to the ctxtags.
// injectOpentracingIdsToTags writes the given context to the ctxtags if the trace.
domgreen marked this conversation as resolved.
Show resolved Hide resolved
// This is done in an incredibly hacky way, because the public-facing interface of opentracing doesn't give access to
// the TraceId and SpanId of the SpanContext. Only the Tracer's Inject/Extract methods know what these are.
// Most tracers have them encoded as keys with 'traceid' and 'spanid':
// https://github.com/openzipkin/zipkin-go-opentracing/blob/594640b9ef7e5c994e8d9499359d693c032d738c/propagation_ot.go#L29
// https://github.com/opentracing/basictracer-go/blob/1b32af207119a14b1b231d451df3ed04a72efebf/propagation_ot.go#L26
// Jaeger from Uber use one-key schema with next format '{trace-id}:{span-id}:{parent-span-id}:{flags}'
// https://www.jaegertracing.io/docs/client-libraries/#trace-span-identity
func hackyInjectOpentracingIdsToTags(span opentracing.Span, tags grpc_ctxtags.Tags) {
if err := span.Tracer().Inject(span.Context(), opentracing.HTTPHeaders, &hackyTagsCarrier{tags}); err != nil {
grpclog.Printf("grpc_opentracing: failed extracting trace info into ctx %v", err)
func injectOpentracingIdsToTags(span opentracing.Span, tags grpc_ctxtags.Tags) {
if err := span.Tracer().Inject(span.Context(), opentracing.HTTPHeaders, &tagsCarrier{tags}); err != nil {
grpclog.Infof("grpc_opentracing: failed extracting trace info into ctx %v", err)
}
}

// hackyTagsCarrier is a really hacky way of
type hackyTagsCarrier struct {
// tagsCarrier is a really hacky way of
type tagsCarrier struct {
grpc_ctxtags.Tags
}

func (t *hackyTagsCarrier) Set(key, val string) {
if strings.Contains(key, "traceid") || strings.Contains(strings.ToLower(key), "traceid") {
func (t *tagsCarrier) Set(key, val string) {
if strings.Contains(strings.ToLower(key), "traceid") {
domgreen marked this conversation as resolved.
Show resolved Hide resolved
t.Tags.Set(TagTraceId, val) // this will most likely be base-16 (hex) encoded
} else if (strings.Contains(key, "spanid") && !strings.Contains(key, "parent")) || (strings.Contains(strings.ToLower(key), "spanid") && !strings.Contains(strings.ToLower(key), "parent")) {
}

if strings.Contains(strings.ToLower(key), "spanid") && !strings.Contains(strings.ToLower(key), "parent") {
t.Tags.Set(TagSpanId, val) // this will most likely be base-16 (hex) encoded
} else if key == "uber-trace-id" {
}

if strings.Contains(strings.ToLower(key), "sampled") {
t.Tags.Set(TagSampled, val)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about enforcing that the val is either true or false, and ignoring it if it isn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do that however both OpenZipkin and OpenTracing do strconv.FormatBool(sc.Sampled) to set the value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I would prefer to pass thru what is in the sampled field from the OT implementation if it is not true/false it would be valuable to have this visualised.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@devnev is this change blocking a merge?

}

if key == "uber-trace-id" {
parts := strings.Split(val, ":")
if len(parts) >= 2 {
if len(parts) == 4 {
t.Tags.Set(TagTraceId, parts[0])
t.Tags.Set(TagSpanId, parts[1])

if parts[3] != JaegerNotSampledFlag {
t.Tags.Set(TagSampled, "true")
} else {
t.Tags.Set(TagSampled, "false")
}
}
}
}
59 changes: 32 additions & 27 deletions tracing/opentracing/interceptors_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
package grpc_opentracing_test

import (
"encoding/json"
"errors"
"strconv"
"strings"
Expand All @@ -17,10 +16,10 @@ import (

"github.com/grpc-ecosystem/go-grpc-middleware"
"github.com/grpc-ecosystem/go-grpc-middleware/tags"
grpc_testing "github.com/grpc-ecosystem/go-grpc-middleware/testing"
"github.com/grpc-ecosystem/go-grpc-middleware/testing"
pb_testproto "github.com/grpc-ecosystem/go-grpc-middleware/testing/testproto"
grpc_opentracing "github.com/grpc-ecosystem/go-grpc-middleware/tracing/opentracing"
opentracing "github.com/opentracing/opentracing-go"
"github.com/grpc-ecosystem/go-grpc-middleware/tracing/opentracing"
"github.com/opentracing/opentracing-go"
"github.com/opentracing/opentracing-go/mocktracer"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand All @@ -36,20 +35,6 @@ var (
fakeInboundSpanId = 999
)

func tagsToJson(value map[string]interface{}) string {
str, _ := json.Marshal(value)
return string(str)
}

func tagsFromJson(t *testing.T, jstring string) map[string]interface{} {
var msgMapTemplate interface{}
err := json.Unmarshal([]byte(jstring), &msgMapTemplate)
if err != nil {
t.Fatalf("failed unmarshaling tags from response %v", err)
}
return msgMapTemplate.(map[string]interface{})
}

type tracingAssertService struct {
pb_testproto.TestServiceServer
T *testing.T
Expand All @@ -59,7 +44,9 @@ func (s *tracingAssertService) Ping(ctx context.Context, ping *pb_testproto.Ping
assert.NotNil(s.T, opentracing.SpanFromContext(ctx), "handlers must have the spancontext in their context, otherwise propagation will fail")
tags := grpc_ctxtags.Extract(ctx)
assert.True(s.T, tags.Has(grpc_opentracing.TagTraceId), "tags must contain traceid")
assert.True(s.T, tags.Has(grpc_opentracing.TagSpanId), "tags must contain traceid")
assert.True(s.T, tags.Has(grpc_opentracing.TagSpanId), "tags must contain spanid")
assert.True(s.T, tags.Has(grpc_opentracing.TagSampled), "tags must contain sampled")
assert.Equal(s.T, tags.Values()[grpc_opentracing.TagSampled], "true", "sampled must be set to true")
return s.TestServiceServer.Ping(ctx, ping)
}

Expand All @@ -72,12 +59,19 @@ func (s *tracingAssertService) PingList(ping *pb_testproto.PingRequest, stream p
assert.NotNil(s.T, opentracing.SpanFromContext(stream.Context()), "handlers must have the spancontext in their context, otherwise propagation will fail")
tags := grpc_ctxtags.Extract(stream.Context())
assert.True(s.T, tags.Has(grpc_opentracing.TagTraceId), "tags must contain traceid")
assert.True(s.T, tags.Has(grpc_opentracing.TagSpanId), "tags must contain traceid")
assert.True(s.T, tags.Has(grpc_opentracing.TagSpanId), "tags must contain spanid")
assert.True(s.T, tags.Has(grpc_opentracing.TagSampled), "tags must contain sampled")
assert.Equal(s.T, tags.Values()[grpc_opentracing.TagSampled], "true", "sampled must be set to true")
return s.TestServiceServer.PingList(ping, stream)
}

func (s *tracingAssertService) PingEmpty(ctx context.Context, empty *pb_testproto.Empty) (*pb_testproto.PingResponse, error) {
assert.NotNil(s.T, opentracing.SpanFromContext(ctx), "handlers must have the spancontext in their context, otherwise propagation will fail")
tags := grpc_ctxtags.Extract(ctx)
assert.True(s.T, tags.Has(grpc_opentracing.TagTraceId), "tags must contain traceid")
assert.True(s.T, tags.Has(grpc_opentracing.TagSpanId), "tags must contain spanid")
assert.True(s.T, tags.Has(grpc_opentracing.TagSampled), "tags must contain sampled")
assert.Equal(s.T, tags.Values()[grpc_opentracing.TagSampled], "false", "sampled must be set to false")
return s.TestServiceServer.PingEmpty(ctx, empty)
}

Expand Down Expand Up @@ -135,12 +129,17 @@ func (s *OpentracingSuite) SetupTest() {
s.mockTracer.Reset()
}

func (s *OpentracingSuite) createContextFromFakeHttpRequestParent(ctx context.Context) context.Context {
func (s *OpentracingSuite) createContextFromFakeHttpRequestParent(ctx context.Context, sampled bool) context.Context {
jFlag := 0
if sampled {
jFlag = 1
}

hdr := http.Header{}
hdr.Set("uber-trace-id", fmt.Sprintf("%d:%d:%d:1", fakeInboundTraceId, fakeInboundSpanId, fakeInboundSpanId))
hdr.Set("uber-trace-id", fmt.Sprintf("%d:%d:%d:%d", fakeInboundTraceId, fakeInboundSpanId, fakeInboundSpanId, jFlag))
hdr.Set("mockpfx-ids-traceid", fmt.Sprint(fakeInboundTraceId))
hdr.Set("mockpfx-ids-spanid", fmt.Sprint(fakeInboundSpanId))
hdr.Set("mockpfx-ids-sampled", fmt.Sprint(true))
hdr.Set("mockpfx-ids-sampled", fmt.Sprint(sampled))

parentSpanContext, err := s.mockTracer.Extract(opentracing.HTTPHeaders, opentracing.HTTPHeadersCarrier(hdr))
require.NoError(s.T(), err, "parsing a fake HTTP request headers shouldn't fail, ever")
Expand Down Expand Up @@ -179,7 +178,7 @@ func (s *OpentracingSuite) assertTracesCreated(methodName string) (clientSpan *m
}

func (s *OpentracingSuite) TestPing_PropagatesTraces() {
ctx := s.createContextFromFakeHttpRequestParent(s.SimpleCtx())
ctx := s.createContextFromFakeHttpRequestParent(s.SimpleCtx(), true)
_, err := s.Client.Ping(ctx, goodPing)
require.NoError(s.T(), err, "there must be not be an on a successful call")
s.assertTracesCreated("/mwitkow.testproto.TestService/Ping")
Expand All @@ -188,7 +187,7 @@ func (s *OpentracingSuite) TestPing_PropagatesTraces() {
func (s *OpentracingSuite) TestPing_ClientContextTags() {
const name = "opentracing.custom"
ctx := grpc_opentracing.ClientAddContextTags(
s.createContextFromFakeHttpRequestParent(s.SimpleCtx()),
s.createContextFromFakeHttpRequestParent(s.SimpleCtx(), true),
opentracing.Tags{name: ""},
)

Expand All @@ -206,7 +205,7 @@ func (s *OpentracingSuite) TestPing_ClientContextTags() {
}

func (s *OpentracingSuite) TestPingList_PropagatesTraces() {
ctx := s.createContextFromFakeHttpRequestParent(s.SimpleCtx())
ctx := s.createContextFromFakeHttpRequestParent(s.SimpleCtx(), true)
stream, err := s.Client.PingList(ctx, goodPing)
require.NoError(s.T(), err, "should not fail on establishing the stream")
for {
Expand All @@ -220,7 +219,7 @@ func (s *OpentracingSuite) TestPingList_PropagatesTraces() {
}

func (s *OpentracingSuite) TestPingError_PropagatesTraces() {
ctx := s.createContextFromFakeHttpRequestParent(s.SimpleCtx())
ctx := s.createContextFromFakeHttpRequestParent(s.SimpleCtx(), true)
erroringPing := &pb_testproto.PingRequest{Value: "something", ErrorCodeReturned: uint32(codes.OutOfRange)}
_, err := s.Client.PingError(ctx, erroringPing)
require.Error(s.T(), err, "there must be an error returned here")
Expand All @@ -229,6 +228,12 @@ func (s *OpentracingSuite) TestPingError_PropagatesTraces() {
assert.Equal(s.T(), true, serverSpan.Tag("error"), "server span needs to be marked as an error")
}

func (s *OpentracingSuite) TestPingEmpty_NotSampleTraces() {
ctx := s.createContextFromFakeHttpRequestParent(s.SimpleCtx(), false)
_, err := s.Client.PingEmpty(ctx, &pb_testproto.Empty{})
require.NoError(s.T(), err, "there must be not be an on a successful call")
}

type jaegerFormatInjector struct{}

func (jaegerFormatInjector) Inject(ctx mocktracer.MockSpanContext, carrier interface{}) error {
Expand Down
3 changes: 2 additions & 1 deletion tracing/opentracing/server_interceptors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ func newServerSpanFromInbound(ctx context.Context, tracer opentracing.Tracer, fu
ext.RPCServerOption(parentSpanContext),
grpcTag,
)
hackyInjectOpentracingIdsToTags(serverSpan, grpc_ctxtags.Extract(ctx))

injectOpentracingIdsToTags(serverSpan, grpc_ctxtags.Extract(ctx))
return opentracing.ContextWithSpan(ctx, serverSpan), serverSpan
}

Expand Down