-
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
Conversation
There is a discussion about moving propagator out of tracer() in the spec This PR partially moves propagator to This PR should be refactored according to the outcome of the spec issue. |
9411593
to
74ff726
Compare
94c9d5d
to
107023d
Compare
since open-telemetry/opentelemetry-specification#112 is fixed by open-telemetry/opentelemetry-specification#209, this PR is ready for review and merge. |
91bcbd8
to
35f8c48
Compare
|
||
var _ apipropagation.TextFormatPropagator = httpTraceContextPropagator{} | ||
|
||
// CarrierExtractor implements CarrierExtractor method of TextFormatPropagator interface. |
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.
That's a bit stuttery. What do you think about CarrierExtractor is a part of an implementation of the TextFormatPropagator interface.
?
Docs of other functions sound like this too, so if you think my proposal is fine, please update the other docs too.
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.
I changed it. Will upload the diff soon. LMK if it is ok.
if ok { | ||
return traceContextExtractor{req: req} | ||
} | ||
return traceContextExtractor{} |
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.
Maybe the propagation package, or API propagation package could provide a noop implementations of the interfaces? Then you could use it here and assume that traceContextExtractor
's req
field can't be nil
?
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.
Added noop propagator.
// W3C TraceContext Header and decodes SpanContext from the Header. | ||
func (tce traceContextExtractor) Extract() (sc core.SpanContext, tm tag.Map) { | ||
if tce.req == nil { | ||
return core.EmptySpanContext(), tag.NewEmptyMap() |
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.
That's a lot of core.EmptySpanContext(), tag.NewEmptyMap()
repetition. Maybe add some empty function like func noExtract() (sc core.SpanContext, m tag.Map) {return}
(or func noExtract() (core.SpanContext, tag.Map) {return core.EmptySpanContext(), tag.NewEmptyMap()}
) and use that?
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.
added noExtract().
api/propagation/propagator.go
Outdated
"go.opentelemetry.io/api/tag" | ||
) | ||
|
||
// TextFormatPropagator is an interface that specifies methods to create CarrierInjector |
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.
I'm not sure what this text format
pertains to. I see text and binary propagators mentioned in otel spec PR 209, but without any details. The spec of propagators feels to be specified in a way that this interface seems to satisfy the needs of the binary propagator too? If the "text format" is about the contents of the tag.Map
, then probably the only thing that needs to be a text are keys and maybe some values. If this is about the way extractors and injectors work (wire format?), I don't see anything limiting them to be textual. So would a name like Propagator
be fine too?
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.
There are two primary encoding for SpanContext and tag.Map (Distributed Context)
- Binary
- TextFormat
TextFormat has different format, like W3C TraceContext and B3.
These formats have different interface requirement hence they are separate.
066ce9a
to
4f5f4ea
Compare
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.
Sweet! Thanks for getting this latest update out the door @rghetia.
api/trace/pass_through_span.go
Outdated
"go.opentelemetry.io/api/tag" | ||
) | ||
|
||
type PassThroughSpan struct { |
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.
I'm questioning whether we need this PassThroughSpan
and PassThroughTracer
.
My mental model for extracting a context to start a child looks like:
childSpanContext := propagate.Extract(dataFromWire)
ctx, childSpan := tracer.Start("childName", trace.ChildOf(childSpanContext))
I guess the key idea here is that we don't need to install the extracted context as the current one in any sense, we only need to pass it to the Start
call as an option to signify an extracted parent 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.
I'm questioning whether we need this
PassThroughSpan
andPassThroughTracer
.My mental model for extracting a context to start a child looks like:
childSpanContext := propagate.Extract(dataFromWire) ctx, childSpan := tracer.Start("childName", trace.ChildOf(childSpanContext))
I guess the key idea here is that we don't need to install the extracted context as the current one in any sense, we only need to pass it to the
Start
call as an option to signify an extracted parent context.
Concept for PassThroughTracer was to enable propagating tracecontext without fully enabling tracing. For example, in Istio, envoy-proxy does all the instrumentation. However, it requires trace-context header be forwarded through application. In such case PassThroughTracer would be very useful.
May be it should be pulled out from here for now and can be added later it it is needed.
We discussed in today's SIG call some questions about how to support OT baggage in the bridge for inject/extract. @krnowak will be working on this. |
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.
Some nits and comments.
This PR seems to useful for implementing the base of the OTEP 42, but otherwise is not really usable at the moment, since there is no connection between the tracer and the propagator, so which propagator should be used when injecting or extracting needs either to be hardcoded or to have some nonOpenTelemetry infrastructure (normally it would be some kind of propagator registry from OTEP 42, but it's not there yet).
api/propagation/noop_propagator.go
Outdated
// GetAllKeys returns empty list of strings. | ||
func (np NoopTextFormatPropagator) GetAllKeys() []string { | ||
return []string{} | ||
} |
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.
missing newline
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.
api/trace/pass_through_span.go
Outdated
func (ds *PassThroughSpan) SetName(name string) { | ||
} | ||
|
||
// Tracer returns noop implementation of Tracer. |
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.
It returns a pass through tracer.
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.
That's an old comment, passthrough tracer seems to be gone.
api/trace/pass_through_tracer.go
Outdated
|
||
// WithSpan wraps around execution of func with noop span. | ||
func (t PassThroughTracer) 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it make a call to Start
first to create a span and put it into context? While I see that WithSpan
does not allow passing any options, which means that you can't specify a remote context, so Start
will return a NoopSpan
anyway, I still think that Start
should put this NoopSpan
into context too - it's predictable and maybe will allow for easier testing of propagators within the single process.
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.
That's an old comment, passthrough tracer seems to be gone.
api/trace/pass_through_tracer.go
Outdated
op(&opts) | ||
} | ||
if !opts.RemoteSpanContext.IsValid() { | ||
return ctx, NoopSpan{} |
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.
Shouldn't this put the NoopSpan into the context, just in case to cover some previous span during testing, for example?
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.
That's an old comment, passthrough tracer seems to be gone.
example/http/client/client.go
Outdated
ctx, req, inj := httptrace.W3C(ctx, req) | ||
|
||
trace.Inject(ctx, inj) | ||
propagator := propagation.HttpTraceContextPropagator() |
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.
For example purposes, I would write it as:
ctx, req = httptrace.W3C(ctx, req)
httptrace.Inject(ctx, req)
Otherwise it is confusing that for injection we explicitly get a propagator, but for extraction , we use httptrace.Extract
, so we don't use any propagator explicitly.
The example gets away with it, because internally, httptrace.Extract
uses the same propagator we use when we inject it here, but I don't think we would want to rely on that.
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.
internal/trace/mock_span.go
Outdated
|
||
var _ apitrace.Span = (*MockSpan)(nil) | ||
|
||
// SpancContext returns an invalid 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.
The comment is false. Also s/SpancContext/SpanContext/
.
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.
internal/trace/mock_span.go
Outdated
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
|
||
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't that do span := mt.Start(ctx, name)
and defer span.Finish()
before using the body
callback?
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.
If needed for testing then yes. Otherwise it should be a noop.
// HttpTraceContextPropagator creates a new text format propagator that propagates SpanContext | ||
// in W3C TraceContext format. | ||
// | ||
// The propagator is then used to create CarrierInjector and CarrierExtractor associated with a |
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.
This paragraph sounds like a comment from the future. I see no notion of CarrierInjectors or CarrierExtractors in this PR.
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.
comment was from past ;-). Earlier I had CarrierInjectors and CarrierExtractors.
Removed it.
|
||
func TestInjectTraceContextToHTTPReq(t *testing.T) { | ||
var id uint64 | ||
trace.SetGlobalTracer(&mocktrace.MockTracer{ |
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.
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 comment
The 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().
Removed installing global tracer.
The connection between tracer and propagator is not necessary. Typically plugins (like http plugins - othttp) would take propagators as an option. Now you could have propagator registry to provide the default propagator but you would still need to be able to override the default on per instance of othttp. For example, a service (A) may be interacting with two other services (B and C). One (C) of them is a legacy service that only accepts B3 Headers. So by default you would have W3C propagator but for B3 propagator for C. |
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.
Went through the trace context spec and noticed some things.
return core.EmptySpanContext() | ||
} | ||
version := int(ver[0]) | ||
if version > maxVersion { |
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.
I went through the spec and noticed this in 3.2.4 Versioning of traceparent:
This specification is opinionated about future versions of trace context. The current version of this specification assumes that future versions of the traceparent header will be additive to the current one.
So this needs to stay as is.
} | ||
sc.SpanID = result | ||
|
||
opts, err := hex.DecodeString(sections[3]) |
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.
We should check here if len(sections[3]) == 2
. This is also in 3.2.4 Versioning of traceparent:
Parse the
sampled
bit offlags
(2 characters from the third dash). Vendors MUST check that the 2 characters are either the end of the string or a dash.
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.
Also, I wonder if we should have a check like if version == 0 && opts[0] > 2 { return core.EmptySpanContext() }
as the only specified flag currently is a "sampled" flag for which the first bit is reserved and all other bits should be zero. See 3.2.2.3.3 Other Flags:
The behavior of other flags, such as (00000100) is not defined and is reserved for future use. Vendors MUST set those to zero.
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.
return core.EmptySpanContext() | ||
} | ||
sc.TraceID.Low = result | ||
|
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.
Should we add a check if sc.TraceID.Low == 0 && sc.TraceID.High == 0 { return core.EmptySpanContext() }
? See first paragraph of 3.2.2.3 trace-id:
All bytes as zero (00000000000000000000000000000000) is considered an invalid value.
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.
Never mind that - I see that we basically check it at the end of the function.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be err != nil || result == 0
? See first paragraph in 3.2.2.3.1 parent-id:
All bytes as zero (0000000000000000) is considered an invalid value.
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.
Never mind that, I see that we check it at the end of the function.
if len(sections) < 4 { | ||
return core.EmptySpanContext() | ||
} | ||
|
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.
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
HEXDIGLC = DIGIT / "a" / "b" / "c" / "d" / "e" / "f" ; lowercase hex character
or
Vendors MUST ignore the
traceparent
when theparent-id
is invalid (for example, if it contains non-lowercase hex characters).
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.
added the check.
{ | ||
name: "future version", | ||
header: "02-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01", | ||
wantSc: core.SpanContext{ |
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.
Yeah, that's what spec says, so it the test should stay as it is.
if err != nil || len(opts) < 1 { | ||
return core.EmptySpanContext() | ||
} | ||
sc.TraceOptions = opts[0] |
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.
I think this should be sc.TraceOptions = opts[0] &^ core.TraceOptionUnused
.
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.
header: "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7", | ||
wantSc: core.EmptySpanContext(), | ||
}, | ||
{ |
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.
Some suggestions for the tests:
- "wrong length of version", "0000-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01"
- "bogus version", "qw-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01"
- "invalid version", "ff-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01"
- "wrong length of trace id", "00-4bf92f3577b34da6a3ce929d0e0e47-00f067aa0ba902b7-01"
- "bogus trace id", "00-qwf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01"
- "wrong length of parent id", "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902-01"
- "bogus parent id", "00-4bf92f3577b34da6a3ce929d0e0e4736-qwf067aa0ba902b7-01"
- "valid trace id, zero parent id", "00-4bf92f3577b34da6a3ce929d0e0e4736-0000000000000000-01"
- "valid parent id, zero trace id", "00-00000000000000000000000000000000-00f067aa0ba902b7-01"
- "wrong length of options", "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-0101"
- "invalid options", "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-02"
- "bogus options", "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-qw"
- "ignored future options", "aa-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-ff"
- should result in valid span context with only sampling bit set in options
Not sure about those yet:
- "version with invalid hex characters", "0A-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01"
- "trace id with invalid hex characters", "00-4BF92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01"
- "parent id with invalid hex characters", "00-4BF92f3577b34da6a3ce929d0e0e4736-00F067AA0ba902b7-01"
- "options with invalid hex characters", "01-4BF92f3577b34da6a3ce929d0e0e4736-00F067AA0ba902b7-A1"
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.
added all test cases including upper-case hex characters.
Also added future additional data "aa-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-ff-XYZxsf09"
if diff := cmp.Diff(got, want); diff != "" { | ||
t.Errorf("GetAllKeys: -got +want %s", diff) | ||
} | ||
} |
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.
Missing newline at the end of the file.
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.
}, | ||
wantHeader: "00-4bf92f3577b34da6a3ce929d0e0e4736-0000000000000002-00", | ||
}, | ||
{ |
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.
I wonder if there should be a test case with a valid span context having options like 0xff
, so the expected header should have 01
as options, not ff
.
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.
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
for version 0, anything other than 01 should fail
{
name: "trace-flag unused bits set",
header: "00-ab000000000000000000000000000000-cd00000000000000-09",
},
For future version accept 01 and 00
{
name: "future options with sampled bit set",
header: "02-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-09",
wantSc: core.SpanContext{
TraceID: traceID,
SpanID: spanID,
TraceOptions: core.TraceOptionSampled,
},
},
{
name: "future options with sampled bit cleared",
header: "02-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-08",
wantSc: core.SpanContext{
TraceID: traceID,
SpanID: spanID,
},
},
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.
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 00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01
(not ff
at the end).
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.
added.
65820f7
to
63ae45b
Compare
@krnowak PTAL. |
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.
Nits mostly, but I think there are some tests for getting a header out of span context missing.
Makefile
Outdated
@@ -33,6 +33,9 @@ precommit: $(TOOLS_DIR)/golangci-lint $(TOOLS_DIR)/misspell $(TOOLS_DIR)/string | |||
PATH="$(abspath $(TOOLS_DIR)):$${PATH}" go generate ./... | |||
$(TOOLS_DIR)/golangci-lint run --fix # TODO: Fix this on windows. | |||
$(TOOLS_DIR)/misspell -w $(ALL_DOCS) | |||
|
|||
.PHONY: mod-tidy |
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.
I'm kinda on the fence with this. I guess it's fine as long as you make mod-tidy
a prerequisite of the precommit
target. That way CI can run go mod tidy
and yell at you when the go.mod
or go.sum
files change.
} | ||
|
||
if !traceCtxRegExp.MatchString(h) { | ||
fmt.Printf("header does not match regex %s\n", h) |
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.
I guess it's some debugging leftover.
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.
oops!
header: "00-ab000000000000000000000000000000-cd00000000000000-0100", | ||
}, | ||
{ | ||
name: "bogus version length", |
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.
Please drop the length
word here.
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.
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.
Did you forget to push the fix?
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.
I probably missed when I did search/replace.
header: "qw-00000000000000000000000000000000-0000000000000000-01", | ||
}, | ||
{ | ||
name: "bogus trace ID length", |
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.
Same here.
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.
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.
Did you forget to push the fix?
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.
same.
header: "00-qw000000000000000000000000000000-cd00000000000000-01", | ||
}, | ||
{ | ||
name: "bogus span ID length", |
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.
Same here.
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.
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.
Did you forget to push the fix?
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.
same.
header: "00-ab000000000000000000000000000000-cd00000000000000-qw", | ||
}, | ||
{ | ||
name: "upper case version length", |
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.
Same here.
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.
header: "A0-00000000000000000000000000000000-0000000000000000-01", | ||
}, | ||
{ | ||
name: "upper case trace ID length", |
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.
Same here.
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.
header: "00-AB000000000000000000000000000000-cd00000000000000-01", | ||
}, | ||
{ | ||
name: "upper case span ID length", |
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.
Same here.
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.
header: "00-ab000000000000000000000000000000-CD00000000000000-01", | ||
}, | ||
{ | ||
name: "upper case trace flag length", |
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.
Same here.
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.
}, | ||
wantHeader: "00-4bf92f3577b34da6a3ce929d0e0e4736-0000000000000002-00", | ||
}, | ||
{ |
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.
I can't see those tests anywhere.
- added Default Tracer which simply propagates SpanContext. - added CopyOfRemote option to simply create remote span.
- also remove PassThroughTracer
046dfa9
to
ed41e82
Compare
Partially fixes #8
Binary and B3 propagation will be in another PR..