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

Remove BinaryFormat and HttpTextFormat from Tracer #214

Closed
wants to merge 2 commits into from

Conversation

songy23
Copy link
Member

@songy23 songy23 commented Aug 13, 2019

Fixes #112. Those formats are moved into Propagators.

@bogdandrutu
Copy link
Member

I think we still need this. We need a Binary/Http formats for SpanContext and for DistributedContext. Do you take a dependency on trace/distributedCtxt there?

@bogdandrutu
Copy link
Member

The propagators define a format that will be a template because they do not depend on SpanContext or DistributedContext. I am not sure how do I get an instance of the BinaryFormat that knows to serialize a SpanContext vs one that knows to serialize a DistributedContext.

@@ -16,8 +16,6 @@ Table of Content
* [WithSpan](#withspan)
* [SpanBuilder](#spanbuilder)
* [RecordSpanData](#recordspandata)
* [GetBinaryFormat](#getbinaryformat)
Copy link
Member

Choose a reason for hiding this comment

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

The propagators define a format that will be a template because they do not depend on SpanContext or DistributedContext. I am not sure how do I get an instance of the BinaryFormat that knows to serialize a SpanContext vs one that knows to serialize a DistributedContext.

@songy23
Copy link
Member Author

songy23 commented Aug 14, 2019

@bogdandrutu From my understanding the Propagators API will take a dependency on distributed context/span context. An example from https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-propagators.md:

ToBytes:
Required arguments:
the value to serialize, can be SpanContext or DistributedContext.

The point here is we should have a different entry that holds the formats rather than Tracer. So it will be something like

OpenTelemetry.getPropagator().getTracingBinaryFormat();

rather than

OpenTelemetry.getTracer().getBinaryFormat();

Which is closer to what we had in OpenCensus (PropagationComponent vs. Tracer).

@bogdandrutu
Copy link
Member

Ok so you need to propose the entire change not just removing the once in the tracer, currently I don't see anywhere documented the API you suggested.

@songy23
Copy link
Member Author

songy23 commented Aug 14, 2019

Added a note in 4aa2b6c. I think this matched what Go (open-telemetry/opentelemetry-go#85) and Python (open-telemetry/opentelemetry-python#78) did.

@carlosalberto
Copy link
Contributor

you need to propose the entire change not just removing the once in the tracer

Agree with this (unless that other changes happens to be too big).

@c24t c24t mentioned this pull request Aug 14, 2019
@rghetia
Copy link
Contributor

rghetia commented Aug 22, 2019

@songy23 is it fair to say that we have an agreement on principle but requires additional changes in this PR before it can be merged?

@songy23
Copy link
Member Author

songy23 commented Aug 22, 2019

Looks this is addressed in #209 (https://github.com/open-telemetry/opentelemetry-specification/pull/209/files#diff-ea4f4a46fe8755cf03f9fb3a6434cb0cR130):

The Tracer SHOULD allow end users to configure other tracing components that
control how Spans are passed across process boundaries, inclucding the binary
and text format Propagators used to serialize Spans created by the
Tracer.

Since it says "SHOULD" rather than "MUST" I would assume having Tracer.GetBinaryFormat and Tracer.GetHttpTextFormat is still recommended but not required.

@songy23
Copy link
Member Author

songy23 commented Aug 22, 2019

Superseded by #209.

@songy23 songy23 closed this Aug 22, 2019
@rghetia
Copy link
Contributor

rghetia commented Aug 23, 2019

The Tracer SHOULD allow end users to configure other tracing components that control how
Spans are passed across process boundaries, including the binary and text format Propagators
used to serialize Spans created by the Tracer.

And

The implementation MUST provide no-op binary and text Propagators, which the Tracer
SHOULD use by default if other propagators are not configured. SDKs SHOULD use the W3C
HTTP Trace Context as the default text format. For more details, see trace-context.

It is not clear. First paragraph says that Tracer SHOULD but DOESN'T HAVE TO allow end user..
In the second paragraph it says that Trace MUST provide no-op binary and text propagator.

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: Should propagating formats be part of Tracer or not?
6 participants