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 Context to SpanProcessor methods #1811

Closed
as-polyakov opened this issue Oct 15, 2020 · 5 comments
Closed

Add Context to SpanProcessor methods #1811

as-polyakov opened this issue Oct 15, 2020 · 5 comments
Labels
Feature Request Suggest an idea for this project SDK

Comments

@as-polyakov
Copy link
Contributor

Is your feature request related to a problem? Please describe.
I would like to use information from Context (Baggage in particular) in SpanProcessor's methods. Currently SpanProcessor's methods don't take Context

Describe the solution you'd like

public interface SpanProcessor {

  void onStart(Context ctx, ReadWriteSpan span);

  void onEnd(Context ctx, ReadableSpan span);
...
}

Describe alternatives you've considered
The only option is to rely on Context.current() in SpanProcessor which is not the right thing when in all cases - for example parent is explicitly set to the specific Context (and also, this will not work for e.g. Go version where there's no Context.current())

@as-polyakov as-polyakov added the Feature Request Suggest an idea for this project label Oct 15, 2020
@jkwatson jkwatson added the SDK label Oct 15, 2020
@jkwatson
Copy link
Contributor

HI @parallelstream .
In the main branch of the project, we already have the onStart signature that you are interested in.

If you want to change the onEnd(), that will require a specification change, as we'd want this to work the same in all languages. There is a related issue open in the specs right now: open-telemetry/opentelemetry-specification#1089. I would recommend putting in your 2 cents on that one, to help move it in the direction you are interested in.

Thanks!

@as-polyakov
Copy link
Contributor Author

Thanks, @jkwatson. Right now I will be fine with just onStart() changes, but think at some stage we would come to the need of onEnd() taking context as well for exact same reasoning.
The issue you mentioned is about R/W span in onEnd, do you want me to bundle the Context there as well or would you recommend just opening a new issue in specification ?

@jkwatson
Copy link
Contributor

I would go ahead and mention it, and your use-case in the discussion on that issue. I'm sure they'll tell you if they want to roll it in, or open a separate one. :)

@Oberon00
Copy link
Member

Oberon00 commented Oct 16, 2020

@parallelstream Which Context do you want in onEnd? Also the parent context? Then we would need to store the parent Context somewhere. In open-telemetry/opentelemetry-specification#875 that originally added this, I originally had specified to add the full parent Context to the SpanData, but that idea was shot down due to performance concerns (unfortunately in a Spec SIG meeting so there is not much documentation on the ticket: open-telemetry/opentelemetry-specification#875 (comment)). So IMHO: Instead of passing the context to OnEnd, we should either: add the parent Context to SpanData; or leave it as-is with not require storing the parent Context.

@bogdandrutu
Copy link
Member

@parallelstream closing this since the "End" call on the Span does not accept a context, because of this the only thing that the SDK can provide is the Context.current() which you can access as well if needed.

If we will add support for user to pass a different Context for end (which I am not sure why) we need to pass that to the processor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Suggest an idea for this project SDK
Projects
None yet
Development

No branches or pull requests

4 participants