-
Notifications
You must be signed in to change notification settings - Fork 761
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 #55
Conversation
8e8441b
to
9149be2
Compare
* @param format the format of the carrier. | ||
* @param carrier the carrier of propagation fields, such as an http request. | ||
*/ | ||
inject(spanContext: SpanContext, format: string, carrier: unknown): void; |
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.
What are the backwards compatibility guarantees that opentelem is trying to make? In opentracing-javascript, inject is accepts either a SpanContext or Span. I'm in favor of only allowing a span context since the opentracing code ends up doing if (context instanceof Span) { context = span.context() }
anyway, but want to make sure the API difference is captured in the event we make a shim down the line.
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 in favor of accepting a Span
everywhere a SpanContext
is accepted which makes the API easier to work with.
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.
Opened an issue to discuss #58
More of a basic question here - what's the advantage of having an That way the functionality of the propagation is really all about serializing and deserializing trace context and doesn't need to worry about how to actually inject those headers. There could then be a separate utility that takes the headers and injects them into something like an HTTP, gRPC, etc. context |
* Injects the given {@link SpanContext} instance to transmit over the wire. | ||
* | ||
* OpenTelemetry defines a common set of format values (BinaryFormat and | ||
* HTTPTextFormat), and each has an expected `carrier` type. |
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.
Propagation can be done on non-HTTP protocols. There should be a distinction between HTTP headers and text map, similar to OpenTracing.
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 interface is inspired from OpenTracing codebase and OTel specs. Let me know if I missed anything here.
import { SpanContext } from '../../trace/span_context'; | ||
|
||
/** Defines a propagation interface. */ | ||
export interface Propagation { |
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.
Propagation sounds more like a namespace. The interface should probably be called Propagator
.
Also, as discussed in #48 (comment) there needs to be an additional abstraction than SpanContext
if SpanContext
represents the W3C 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.
Renamed to Propagator
.
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.
Let's revisit HTTPTextFormat
in a future revision since it's semantically incorrect.
* chore: move baggage methods in propagation namespace * chore: add upgrade guidelines
* chore: move baggage methods in propagation namespace * chore: add upgrade guidelines
This PR adds
Propagator
interface.