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

Context API improvements #215

Merged
merged 4 commits into from
Apr 1, 2020
Merged

Context API improvements #215

merged 4 commits into from
Apr 1, 2020

Conversation

mwear
Copy link
Member

@mwear mwear commented Mar 29, 2020

As mentioned in #201 there are some situations where our current APIs fall short. This PR makes some improvements to our existing APIs to make some situations easier to handle. Below is a summary of the changes with some examples demonstrating the improvements.

I have a span, but I want it in a context

We have discussed how it's sometimes covenient to have a handle on span, where in other situations, you need to have a handle on a context containing a span. The Go SIG solved this by having API methods return both a span and a context. While multiple return values are not the best choice for Ruby, we can yield multiple parameters to blocks for Tracer#in_span and Tracer#with_span to achieve the same effect.

Before:

Tracer.in_span('my-span') { ctx = Context.current }

Now:

Tracer.in_span('my-span') { |span, ctx| ...}

I have a context, but I want a span

Tracer#current_span takes an optional context parameter to read the current span from a context other than Context.current.

Before:

span = context[OpenTelemetry::Trace::Propagation::ContextKeys.current_span_key]

Now:

span = Tracer.current_span(context)

Context.with_* methods yield context

The various Context.with_* now yield context arguments to blocks they execute.

Before:

# would only yield values
Context.with_values(k1 => 'v1', k2 => 'v2') { |values| ctx = Context.current }

Now:

# yields context and values
Context.with_values(k1 => 'v1', k2 => 'v2') { |ctx, values| ... }

There might be more scenarios we need utilities for, but this adds convenience to already existing methods for some use cases that were previously more challenging than they should be.

@fbogsany fbogsany merged commit ed96e93 into open-telemetry:master Apr 1, 2020
@fbogsany fbogsany mentioned this pull request Apr 1, 2020
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.

2 participants