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

Allow for setting an explicit SpanContext on Span creation #412

Closed

Conversation

nicktrav
Copy link

@nicktrav nicktrav commented Jan 2, 2020

In certain circumstances, information from an existing SpanContext is
desirable when instantiating a new Span. For instance, when a process is
accepting inbound spans in various formats before using an OpenTelemetry
exporter to forward to another destination.

Add a new StartOption, WithSpanContext, that allows setting an explicit
SpanContext on a trace.

Closes #411.

Signed-off-by: Nick Travers n.e.travers@gmail.com

In certain circumstances, information from an existing SpanContext is
desirable when instantiating a new Span. For instance, when a process is
accepting inbound spans in various formats before using an OpenTelemetry
exporter to forward to another destination.

Add a new StartOption, WithSpanContext, that allows setting an explicit
SpanContext on a trace.

Signed-off-by: Nick Travers <n.e.travers@gmail.com>
Signed-off-by: Nick Travers <n.e.travers@gmail.com>
Copy link
Contributor

@jmacd jmacd left a 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 the specification would agree with this change--at least I haven't seen anything about supporting span creation with a pre-determined context. If we agree with this (I'm not sure), it should go in the spec first.

On this note, see how in #381 I added an explicit WithParent(SpanContext) option for specifying the parent span. That is supported by the specification.

@nicktrav
Copy link
Author

nicktrav commented Jan 3, 2020

@jmacd - thanks for the feedback. Some thoughts ...

I'm not sure the specification would agree with this change--at least I haven't seen anything about supporting span creation with a pre-determined context. If we agree with this (I'm not sure), it should go in the spec first.

Agree - it seems as though creating a span with an explicit context is currently underspecified. If anywhere, it could probably be added to the list of API parameters for Span creation:
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#span-creation

Something along the lines of:

  • SpanContext - override the current context, if provided. Defaults to propagating an existing parent context, if present, and ultimately falls back on seeding a new context with a new trace and span ID.

Thoughts? I can open a PR on the specs repo and we can discuss there.

On this note, see how in #381 I added an explicit WithParent(SpanContext) option for specifying the parent span. That is supported by the specification.

Just to confirm, are you talking about this section in the spec?

The parent Span or parent Span context, and whether the new Span should be a root Span. API MAY also have an option for implicit parent context extraction from the current context as a default behavior.

I was also thinking that we could tack on a sentence here saying that the API "MAY" have an option to add an explicit context, but it feels like this would be a useful feature of the API and therefore would benefit from being a standalone "MUST".

@lizthegrey lizthegrey self-requested a review January 3, 2020 18:51
@lizthegrey
Copy link
Member

Withdrawing my approval as per @jmacd's comments

@MrAlias
Copy link
Contributor

MrAlias commented Jan 14, 2020

@nicktrav it would be a good idea to either propose this in gitter (the specification channel would be my guess of a good place) or in an OTEP Issue/PR. I think gitter would be the better first step. You'll be able to get more immediate feedback/questions related to the proposed change there.

I'm interested to hear more about your use case. It isn't immediately clear to me why you would build a span context in a process just to have Open Telemetry act as a pass-through-proxy. But, like I said, I'm interested to better understand your use case.

@paivagustavo paivagustavo added pkg:API Related to an API package blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made area:trace Part of OpenTelemetry tracing labels Feb 11, 2020
@jmacd
Copy link
Contributor

jmacd commented Mar 19, 2020

This requires a specification change, so I'm closing it. I imagine strong resistance to this idea as well, because of the potential for mis-use, but I think it's a worthwhile discussion because we may be able to find a better way to achieve the stated goal.

@jmacd jmacd closed this Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:trace Part of OpenTelemetry tracing blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made pkg:API Related to an API package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow SpanContext to be provided when starting a Span
5 participants