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.

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

Merged
merged 28 commits into from
Sep 23, 2019

Conversation

rghetia
Copy link
Contributor

@rghetia rghetia commented Aug 9, 2019

Partially fixes #8

Binary and B3 propagation will be in another PR..

@rghetia
Copy link
Contributor Author

rghetia commented Aug 12, 2019

There is a discussion about moving propagator out of tracer() in the spec
open-telemetry/opentelemetry-specification#112

This PR partially moves propagator to propagation package but Injector/Extractor are still part of tracer package. For example, HTTPPropagator interface is part of propagation package. It uses Injector/Extractor interface defined in trace package but there is not need for those to be in trace pacakge. Injector/Extractor should be moved to propagation package as well.

This PR should be refactored according to the outcome of the spec issue.

@rghetia
Copy link
Contributor Author

rghetia commented Aug 23, 2019

since open-telemetry/opentelemetry-specification#112 is fixed by open-telemetry/opentelemetry-specification#209, this PR is ready for review and merge.


var _ apipropagation.TextFormatPropagator = httpTraceContextPropagator{}

// CarrierExtractor implements CarrierExtractor method of TextFormatPropagator interface.
Copy link
Member

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.

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 changed it. Will upload the diff soon. LMK if it is ok.

if ok {
return traceContextExtractor{req: req}
}
return traceContextExtractor{}
Copy link
Member

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?

Copy link
Contributor Author

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()
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added noExtract().

"go.opentelemetry.io/api/tag"
)

// TextFormatPropagator is an interface that specifies methods to create CarrierInjector
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link

@tedsuo tedsuo left a 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.

"go.opentelemetry.io/api/tag"
)

type PassThroughSpan struct {
Copy link
Contributor

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.

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'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.

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.

@rghetia
Copy link
Contributor Author

rghetia commented Sep 9, 2019

@jmacd , @tedsuo and @krnowak PTAL.
I added MockTracer and MockSpan under internal directory to test Inject method. MockTracer is also used in current_test.go which I will replace once this is approved and merged.

@jmacd
Copy link
Contributor

jmacd commented Sep 12, 2019

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.

Copy link
Member

@krnowak krnowak left a 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).

// GetAllKeys returns empty list of strings.
func (np NoopTextFormatPropagator) GetAllKeys() []string {
return []string{}
}
Copy link
Member

Choose a reason for hiding this comment

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

missing newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

func (ds *PassThroughSpan) SetName(name string) {
}

// Tracer returns noop implementation of Tracer.
Copy link
Member

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.

Copy link
Member

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.


// 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)
Copy link
Member

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.

Copy link
Member

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.

op(&opts)
}
if !opts.RemoteSpanContext.IsValid() {
return ctx, NoopSpan{}
Copy link
Member

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?

Copy link
Member

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.

ctx, req, inj := httptrace.W3C(ctx, req)

trace.Inject(ctx, inj)
propagator := propagation.HttpTraceContextPropagator()
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.


var _ apitrace.Span = (*MockSpan)(nil)

// SpancContext returns an invalid span context.
Copy link
Member

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/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

func (ms *MockSpan) SetName(name string) {
}

// Tracer returns noop implementation of Tracer.
Copy link
Member

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.

Copy link
Contributor Author

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)
Copy link
Member

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?

Copy link
Contributor Author

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
Copy link
Member

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.

Copy link
Contributor Author

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{
Copy link
Member

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.

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 had trace.GlobalTracer().Start() but I don't need that as I can use mockTracer.Start().
Removed installing global tracer.

@rghetia
Copy link
Contributor Author

rghetia commented Sep 18, 2019

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).

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.

Copy link
Member

@krnowak krnowak left a 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 {
Copy link
Member

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])
Copy link
Member

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 of flags (2 characters from the third dash). Vendors MUST check that the 2 characters are either the end of the string or a dash.

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Member

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 {
Copy link
Member

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.

Copy link
Member

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()
}

Copy link
Member

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 the parent-id is invalid (for example, if it contains non-lowercase hex characters).

Copy link
Contributor Author

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{
Copy link
Member

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]
Copy link
Member

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.

Copy link
Contributor Author

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(),
},
{
Copy link
Member

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"

Copy link
Contributor Author

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)
}
}
Copy link
Member

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.

Copy link
Contributor Author

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",
},
{
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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,
			},
		},

Copy link
Member

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added.

@rghetia rghetia force-pushed the api_tc_propagation branch 2 times, most recently from 65820f7 to 63ae45b Compare September 19, 2019 21:26
@rghetia
Copy link
Contributor Author

rghetia commented Sep 20, 2019

@krnowak PTAL.

Copy link
Member

@krnowak krnowak left a 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
Copy link
Member

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)
Copy link
Member

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.

Copy link
Contributor Author

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",
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

Copy link
Member

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?

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 probably missed when I did search/replace.

header: "qw-00000000000000000000000000000000-0000000000000000-01",
},
{
name: "bogus trace ID length",
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

Copy link
Member

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?

Copy link
Contributor Author

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",
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

Copy link
Member

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?

Copy link
Contributor Author

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",
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

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",
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

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",
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

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",
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

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",
},
{
Copy link
Member

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.

@rghetia rghetia merged commit 83935b2 into open-telemetry:master Sep 23, 2019
@rghetia rghetia deleted the api_tc_propagation branch October 8, 2019 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API: Inject/Extract
4 participants