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 API #98

Merged
merged 4 commits into from
May 7, 2020
Merged

Add Context API #98

merged 4 commits into from
May 7, 2020

Conversation

jtescher
Copy link
Member

This provides an implementation of the Context API Spec:

A context is a propagation mechanism which carries execution-scoped values across API boundaries and between logically associated execution units. Cross-cutting concerns access their data in-process using the same shared Context object.

It is implemented as an immutable thread-local collection of heterogeneous values that can be inserted and retrieved by their type. A thread's current context can be assigned via the attach method. The previous context is restored as the current context when the resulting context guard is dropped, allowing precise control and nesting of active context scopes.

This is the groundwork for correlation context propagation, and could also be used to replace constructs like the current span stack.

Alternative design choices

  • Currently this uses Rcs to track values so contexts are not Send / Sync, could be Arc instead if contexts need to be shared.
  • Access to the current context is a clone, alternatively we could expose get_current (which takes a closure with a context ref parameter) additionally or instead of the current method, would be more performant but is a bit more dangerous as attaching within that closure panics at runtime. Currently Context::current_with_value(new_value).attach() instead of get_current(|cx| cx.with_value(new_value)).attach().
  • get and with_value currently take a type parameter instead of the context spec's Key API as this seems like the more idiomatic rust way of accomplishing the same concept.

This provides an implementation of the [Context API
Spec](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context/context.md).
A context is a propagation mechanism which carries execution-scoped values
across API boundaries and between logically associated execution units.
Cross-cutting concerns access their data in-process using the same shared
`Context` object.

It is implemented as an immutable thread-local collection of heterogeneous
values that can be inserted and retrieved by their type. A thread's current
context can be assigned via the `attach` method. The previous context is
restored as the current context when the resulting context guard is dropped,
allowing precise control and nesting of active context scopes.
@jtescher jtescher requested a review from a team April 20, 2020 18:03
@jtescher
Copy link
Member Author

Any thoughts about this context as nested thread local storage? cc @MikeGoldsmith @hawkw

@jtescher
Copy link
Member Author

Can see examples of how it would change how active spans are tracked in sync / async code in #99.

Copy link
Member

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

This looks really good, thanks @jtescher.

Only query is regarding context::default and capacity of 0.

src/api/context/mod.rs Show resolved Hide resolved
@jtescher jtescher merged commit 3497774 into open-telemetry:master May 7, 2020
@jtescher jtescher deleted the add-context-api branch May 7, 2020 01:04
@joroKr21
Copy link

What do you think about exposing get_current? We want to use it as a structured logging context and it seems a bit unnecessary to clone every time we log.

@TommyCpp
Copy link
Contributor

What do you think about exposing get_current? We want to use it as a structured logging context and it seems a bit unnecessary to clone every time we log.

Does something like https://docs.rs/opentelemetry/latest/opentelemetry/struct.Context.html#method.current work for your use case 😃 ?

@joroKr21
Copy link

Does something like https://docs.rs/opentelemetry/latest/opentelemetry/struct.Context.html#method.current work for your use case 😃 ?

That's what we're using currently, but it clones on every access.

I was referring to this comment by @jtescher

Access to the current context is a clone, alternatively we could expose get_current (which takes a closure with a context ref parameter) additionally or instead of the current method, would be more performant but is a bit more dangerous as attaching within that closure panics at runtime.

I'm happy to open a PR if that makes sense.

@jtescher
Copy link
Member Author

@joroKr21 may be worth benchmarking your use case first to see the actual overhead of the Arc::clones. Happy to look at alternate APIs if this causes perf impact in practice.

@joroKr21
Copy link

@joroKr21 may be worth benchmarking your use case first to see the actual overhead of the Arc::clones. Happy to look at alternate APIs if this causes perf impact in practice.

Oh thanks for clarifying - I think in that case it will be fine. Sorry for not realising that on my own 🙈

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.

4 participants