-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add propagator interface and W3C propagator #85
Changes from 1 commit
b4cf5f9
067cb3f
fdaf0f0
7137e7b
d9c6157
1e2f4ea
8215b36
ae02d90
ddeb734
ba6cda4
0c84304
034c2fc
2b279af
8d3cced
a55726d
8fe1085
db8f60b
9c3ecb6
e5e1a93
ecd6ba7
2a5fc9e
c8a05ec
d759fec
418b0f4
f271dfc
4ca80c3
064af5e
ed41e82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- also remove PassThroughTracer
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,64 +21,66 @@ import ( | |
|
||
"go.opentelemetry.io/api/core" | ||
"go.opentelemetry.io/api/tag" | ||
apitrace "go.opentelemetry.io/api/trace" | ||
) | ||
|
||
type PassThroughSpan struct { | ||
sc core.SpanContext | ||
type MockSpan struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing doc string. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added. |
||
sc core.SpanContext | ||
tracer apitrace.Tracer | ||
} | ||
|
||
var _ Span = (*PassThroughSpan)(nil) | ||
var _ apitrace.Span = (*MockSpan)(nil) | ||
|
||
// SpancContext returns an invalid span context. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment is false. Also There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed. |
||
func (ds *PassThroughSpan) SpanContext() core.SpanContext { | ||
if ds == nil { | ||
func (ms *MockSpan) SpanContext() core.SpanContext { | ||
if ms == nil { | ||
core.EmptySpanContext() | ||
} | ||
return ds.sc | ||
return ms.sc | ||
} | ||
|
||
// IsRecordingEvents always returns false for PassThroughSpan. | ||
func (ds *PassThroughSpan) IsRecordingEvents() bool { | ||
// IsRecordingEvents always returns false for MockSpan. | ||
func (ms *MockSpan) IsRecordingEvents() bool { | ||
return false | ||
} | ||
|
||
// SetStatus does nothing. | ||
func (ds *PassThroughSpan) SetStatus(status codes.Code) { | ||
func (ms *MockSpan) SetStatus(status codes.Code) { | ||
} | ||
|
||
// SetError does nothing. | ||
func (ds *PassThroughSpan) SetError(v bool) { | ||
func (ms *MockSpan) SetError(v bool) { | ||
} | ||
|
||
// SetAttribute does nothing. | ||
func (ds *PassThroughSpan) SetAttribute(attribute core.KeyValue) { | ||
func (ms *MockSpan) SetAttribute(attribute core.KeyValue) { | ||
} | ||
|
||
// SetAttributes does nothing. | ||
func (ds *PassThroughSpan) SetAttributes(attributes ...core.KeyValue) { | ||
func (ms *MockSpan) SetAttributes(attributes ...core.KeyValue) { | ||
} | ||
|
||
// ModifyAttribute does nothing. | ||
func (ds *PassThroughSpan) ModifyAttribute(mutator tag.Mutator) { | ||
func (ms *MockSpan) ModifyAttribute(mutator tag.Mutator) { | ||
} | ||
|
||
// ModifyAttributes does nothing. | ||
func (ds *PassThroughSpan) ModifyAttributes(mutators ...tag.Mutator) { | ||
func (ms *MockSpan) ModifyAttributes(mutators ...tag.Mutator) { | ||
} | ||
|
||
// Finish does nothing. | ||
func (ds *PassThroughSpan) Finish(options ...FinishOption) { | ||
func (ms *MockSpan) Finish(options ...apitrace.FinishOption) { | ||
} | ||
|
||
// SetName does nothing. | ||
func (ds *PassThroughSpan) SetName(name string) { | ||
func (ms *MockSpan) SetName(name string) { | ||
} | ||
|
||
// Tracer returns noop implementation of Tracer. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That should say that it returns a mock tracer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed. |
||
func (ds *PassThroughSpan) Tracer() Tracer { | ||
return PassThroughTracer{} | ||
func (ms *MockSpan) Tracer() apitrace.Tracer { | ||
return ms.tracer | ||
} | ||
|
||
// AddEvent does nothing. | ||
func (ds *PassThroughSpan) AddEvent(ctx context.Context, msg string, attrs ...core.KeyValue) { | ||
func (ms *MockSpan) AddEvent(ctx context.Context, msg string, attrs ...core.KeyValue) { | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
// Copyright 2019, OpenTelemetry Authors | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package trace | ||
|
||
import ( | ||
"context" | ||
"math/rand" | ||
"sync/atomic" | ||
|
||
"go.opentelemetry.io/api/core" | ||
apitrace "go.opentelemetry.io/api/trace" | ||
) | ||
|
||
// MockTracer is a simple tracer used for testing purpose only. | ||
// It only supports ChildOf option. SpanId is atomically increased every time a | ||
// new span is created. | ||
type MockTracer struct { | ||
// Sampled specifies if the new span should be sampled or not. | ||
Sampled bool | ||
|
||
// StartSpanId is used to initialize spanId. It is incremented by one | ||
// every time a new span is created. | ||
StartSpanId *uint64 | ||
} | ||
|
||
var _ apitrace.Tracer = (*MockTracer)(nil) | ||
|
||
// WithResources does nothing and returns noop implementation of Tracer. | ||
func (mt *MockTracer) WithResources(attributes ...core.KeyValue) apitrace.Tracer { | ||
return mt | ||
} | ||
|
||
// WithComponent does nothing and returns noop implementation of Tracer. | ||
func (mt *MockTracer) WithComponent(name string) apitrace.Tracer { | ||
return mt | ||
} | ||
|
||
// WithService does nothing and returns noop implementation of Tracer. | ||
func (mt *MockTracer) WithService(name string) apitrace.Tracer { | ||
return mt | ||
} | ||
|
||
// WithSpan wraps around execution of func with noop span. | ||
func (mt *MockTracer) WithSpan(ctx context.Context, name string, body func(context.Context) error) error { | ||
return body(ctx) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't that do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If needed for testing then yes. Otherwise it should be a noop. |
||
} | ||
|
||
// Start starts a MockSpan. It creates a new Span based on Reference SpanContext option. | ||
// TracdID is used from Reference Span Context and SpanID is assigned. | ||
// If Reference SpanContext option is not specified then random TraceID is used. | ||
// No other options are supported. | ||
func (mt *MockTracer) Start(ctx context.Context, name string, o ...apitrace.SpanOption) (context.Context, apitrace.Span) { | ||
var opts apitrace.SpanOptions | ||
for _, op := range o { | ||
op(&opts) | ||
} | ||
var span *MockSpan | ||
var sc core.SpanContext | ||
if !opts.Reference.SpanContext.IsValid() { | ||
sc = core.SpanContext{ | ||
TraceID: core.TraceID{ | ||
High: rand.Uint64(), | ||
Low: rand.Uint64(), | ||
}, | ||
} | ||
if mt.Sampled { | ||
sc.TraceOptions = core.TraceOptionSampled | ||
} | ||
} else { | ||
sc = opts.Reference.SpanContext | ||
} | ||
sc.SpanID = atomic.AddUint64(mt.StartSpanId, 1) | ||
span = &MockSpan{ | ||
sc: sc, | ||
tracer: mt, | ||
} | ||
|
||
return apitrace.SetCurrentSpan(ctx, span), span | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,72 +50,71 @@ func (hp httpTraceContextPropagator) Inject(ctx context.Context, supplier apipro | |
} | ||
} | ||
|
||
func (hp httpTraceContextPropagator) Extract(ctx context.Context, supplier apipropagation.Supplier) context.Context { | ||
func (hp httpTraceContextPropagator) Extract(ctx context.Context, supplier apipropagation.Supplier) core.SpanContext { | ||
h := supplier.Get(traceparentHeader) | ||
if h == "" { | ||
return ctx | ||
return core.EmptySpanContext() | ||
} | ||
|
||
sections := strings.Split(h, "-") | ||
if len(sections) < 4 { | ||
return ctx | ||
return core.EmptySpanContext() | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we really check if all the characters in at least first four sections are either digits or lowercase letters from a to f? That's what the W3C trace context spec says in several places, like
or
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added the check. |
||
if len(sections[0]) != 2 { | ||
return ctx | ||
return core.EmptySpanContext() | ||
} | ||
ver, err := hex.DecodeString(sections[0]) | ||
if err != nil { | ||
return ctx | ||
return core.EmptySpanContext() | ||
} | ||
version := int(ver[0]) | ||
if version > maxVersion { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this check be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the expectation here? will version 1 be compatible with version 0? If so then it should just check for invalid value of the version. If not then it should check for specific version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I went through the spec and noticed this in 3.2.4 Versioning of traceparent:
So this needs to stay as is. |
||
return ctx | ||
return core.EmptySpanContext() | ||
} | ||
|
||
if version == 0 && len(sections) != 4 { | ||
return ctx | ||
return core.EmptySpanContext() | ||
} | ||
|
||
if len(sections[1]) != 32 { | ||
return ctx | ||
return core.EmptySpanContext() | ||
} | ||
|
||
result, err := strconv.ParseUint(sections[1][0:16], 16, 64) | ||
if err != nil { | ||
return ctx | ||
return core.EmptySpanContext() | ||
} | ||
var sc core.SpanContext | ||
|
||
sc.TraceID.High = result | ||
|
||
result, err = strconv.ParseUint(sections[1][16:32], 16, 64) | ||
if err != nil { | ||
return ctx | ||
return core.EmptySpanContext() | ||
} | ||
sc.TraceID.Low = result | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add a check
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Never mind that - I see that we basically check it at the end of the function. |
||
if len(sections[2]) != 16 { | ||
return ctx | ||
return core.EmptySpanContext() | ||
} | ||
result, err = strconv.ParseUint(sections[2][0:], 16, 64) | ||
if err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Never mind that, I see that we check it at the end of the function. |
||
return ctx | ||
return core.EmptySpanContext() | ||
} | ||
sc.SpanID = result | ||
|
||
opts, err := hex.DecodeString(sections[3]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should check here if
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, I wonder if we should have a check like
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed. |
||
if err != nil || len(opts) < 1 { | ||
return ctx | ||
return core.EmptySpanContext() | ||
} | ||
sc.TraceOptions = opts[0] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed. |
||
|
||
if !sc.IsValid() { | ||
return ctx | ||
return core.EmptySpanContext() | ||
} | ||
|
||
ctx, _ = trace.GlobalTracer().Start(ctx, "remote", trace.CopyOfRemote(sc)) | ||
return ctx | ||
return sc | ||
} | ||
|
||
func (hp httpTraceContextPropagator) GetAllKeys() []string { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ import ( | |
"github.com/google/go-cmp/cmp" | ||
|
||
"go.opentelemetry.io/api/core" | ||
mocktrace "go.opentelemetry.io/internal/trace" | ||
"go.opentelemetry.io/propagation" | ||
) | ||
|
||
|
@@ -33,7 +34,7 @@ var ( | |
) | ||
|
||
func TestExtractTraceContextFromHTTPReq(t *testing.T) { | ||
trace.SetGlobalTracer(trace.PassThroughTracer{}) | ||
trace.SetGlobalTracer(&mocktrace.MockTracer{}) | ||
propagator := propagation.HttpTraceContextPropagator() | ||
tests := []struct { | ||
name string | ||
|
@@ -81,9 +82,7 @@ func TestExtractTraceContextFromHTTPReq(t *testing.T) { | |
req.Header.Set("traceparent", tt.header) | ||
|
||
ctx := context.Background() | ||
ctx = propagator.Extract(ctx, req.Header) | ||
span := trace.CurrentSpan(ctx) | ||
gotSc := span.SpanContext() | ||
gotSc := propagator.Extract(ctx, req.Header) | ||
if diff := cmp.Diff(gotSc, tt.wantSc); diff != "" { | ||
t.Errorf("Extract Tracecontext: %s: -got +want %s", tt.name, diff) | ||
} | ||
|
@@ -92,7 +91,11 @@ func TestExtractTraceContextFromHTTPReq(t *testing.T) { | |
} | ||
|
||
func TestInjectTraceContextToHTTPReq(t *testing.T) { | ||
trace.SetGlobalTracer(trace.PassThroughTracer{}) | ||
var id uint64 | ||
trace.SetGlobalTracer(&mocktrace.MockTracer{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a point in installing the global tracer in the test? The test does not use any API that calls into the global tracer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had trace.GlobalTracer().Start() but I don't need that as I can use mockTracer.Start(). |
||
Sampled: false, | ||
StartSpanId: &id, | ||
}) | ||
propagator := propagation.HttpTraceContextPropagator() | ||
tests := []struct { | ||
name string | ||
|
@@ -106,15 +109,15 @@ func TestInjectTraceContextToHTTPReq(t *testing.T) { | |
SpanID: spanID, | ||
TraceOptions: core.TraceOptionSampled, | ||
}, | ||
wantHeader: "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01", | ||
wantHeader: "00-4bf92f3577b34da6a3ce929d0e0e4736-0000000000000001-01", | ||
}, | ||
{ | ||
name: "valid spancontext, not sampled", | ||
sc: core.SpanContext{ | ||
TraceID: traceID, | ||
SpanID: spanID, | ||
}, | ||
wantHeader: "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-00", | ||
wantHeader: "00-4bf92f3577b34da6a3ce929d0e0e4736-0000000000000002-00", | ||
}, | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if there should be a test case with a valid span context having options like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with version '0' I added test to check that option should not be >2. For future version I added other values of option but expected value to be 01 or 00. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't see those tests anywhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for version 0, anything other than 01 should fail
For future version accept 01 and 00
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I meant the test for the inject part, where as input you have a span context like: core.SpanContext{
TraceID: core.TraceID{High: 0x4bf92f3577b34da6, Low: 0xa3ce929d0e0e4736},
SpanID: uint64(0x00f067aa0ba902b7),
TraceOptions: 0xff,
} and the expected generated "traceparent" HTTP header (for now) would be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added. |
||
name: "invalid spancontext", | ||
|
@@ -126,7 +129,9 @@ func TestInjectTraceContextToHTTPReq(t *testing.T) { | |
t.Run(tt.name, func(t *testing.T) { | ||
req, _ := http.NewRequest("GET", "http://example.com", nil) | ||
ctx := context.Background() | ||
ctx, _ = trace.GlobalTracer().Start(ctx, "inject", trace.CopyOfRemote(tt.sc)) | ||
if tt.sc.IsValid() { | ||
ctx, _ = trace.GlobalTracer().Start(ctx, "inject", trace.ChildOf(tt.sc)) | ||
} | ||
propagator.Inject(ctx, req.Header) | ||
|
||
gotHeader := req.Header.Get("traceparent") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And returns empty span context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.