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

feat: default tracer prototype #425

Closed
wants to merge 3 commits into from

Conversation

mwear
Copy link
Member

@mwear mwear commented Oct 3, 2020

Description

With the OTel Ruby API as it currently stands Tracer is the one stop shop for everything that a user needs / wants to do with a span. This design is intentional and was chosen for it's simplicity. There are currently questions around how interactions with context and the current span should be handled: see open-telemetry/opentelemetry-specification#1019.

This description is ridiculously long as it assumes little to no familiarity with the Ruby API. If you are familiar with the project, skip the Changes in this PR section towards the bottom.

For folks less familiar with the project, I'll show how the API currently works for a handful scenarios, and then go on to explain the changes added in this PR and what improvements they bring.

Tracer operations

First, get a handle on a tracer

# obtain a named tracer
tracer = OpenTelemetry.tracer_provider.tracer('my-app', '0.0.1')

The rest of the examples use this tracer

Span creation

Start span

# create and return a span / does not modify context
span = tracer.start_span('a-span')

Start span in current context

# create a new span, set it in the active context
tracer.in_span('a-span') do |span|
  # execute this block of code in the active context
end

Span and context managment

Read the current span

From the current (implicit) context
span = tracer.current_span
From an explicit context
span = tracer.current_span(some_context)

Set span in a context

Implicit context
new_context = tracer.context_with_span(span)
Explicit context
new_context = tracer.context_with_span(span, parent_context: some_context)
Execute block with span in a context
tracer.with_span(some_span) do |span, context|
  #run this block with span set in the active context
end

Changes in this PR

The Tracer#current_context and Tracer#span_with_context methods do not explicitly depend on state from a tracer instance, so they could be class (static) methods on a tracer. However, this introduces some unneccessary complications to the API.

As an alternative solution, this PR introduces an easy to access default tracer on the top-level OpenTelemetry module. It's available as OpenTelemetry.tracer and can be used whenever a user needs to access tracer methods, but does not have an explicit handle on one.

For example, users might want to grab the current_span out of the ether to add an attribute, or event.

OpenTelemetry.tracer.
             current_span&.
             add_event('an-event', attributes: {'k1': 'v1', 'k2': 'v2'})

The OpenTelemetry.tracer method is just a delegate to the global tracer provider. When called without parameters, it returns a tracer named "default". It can also take arguments for name and version which becomes a shortcut for OpenTelemetry.tracer_provider('tracer-name', 'tracer-version').

Alternatives

Originally I added a default_tracer method that delegates to the global tracer provider, obtains a tracer named 'default', and memoizes and returns the result. It has the benefit of not having to lookup the tracer each time it's invoked. I switched to the tracer delegate method as it solves the same use case, is less verbose, and is usable in other scenarios. I think it's worth the tradeoff, but could be convinced otherwise.

api/lib/opentelemetry.rb Outdated Show resolved Hide resolved
@mwear
Copy link
Member Author

mwear commented Oct 16, 2020

open-telemetry/opentelemetry-specification#1063 has merged. It technically allows for what we have in this PR where current_span and context_with_span are instance methods on a tracer. It seems like there would be a slight preference for these to be static methods on the Trace module, which we could do. We could also have them in both places, which might be slightly frowned upon. Either way, I can live with any of these options, I'm just looking for opinions. If you have one, let me know and I can get this cleaned up for review.

@fbogsany
Copy link
Contributor

Here's how the various options look from a client perspective:

Static methods on Trace module.

span = OpenTelemetry::Trace.current_span
span = OpenTelemetry::Trace.current_span(some_context)

new_context = OpenTelemetry::Trace.context_with_span(span)
new_context = OpenTelemetry::Trace.context_with_span(span, parent_context: some_context)

OpenTelemetry::Trace.with_span(span) do |span, context|
  # Run this block with span set in the active context.
end

This is actually pretty good. I'll come back to this later.

Static methods on a class in Trace module (may be named TracingContextUtilities).

span = OpenTelemetry::Trace::TracingContextUtilities.current_span
span = OpenTelemetry::Trace::TracingContextUtilities.current_span(some_context)

new_context = OpenTelemetry::Trace::TracingContextUtilities.context_with_span(span)
new_context = OpenTelemetry::Trace::TracingContextUtilities.context_with_span(span, parent_context: some_context)

OpenTelemetry::Trace::TracingContextUtilities.with_span(span) do |span, context|
  # Run this block with span set in the active context.
end

I think we can all agree this option looks terrible.

Methods on Tracer (using 'global default' tracer instance).

span = OpenTelemetry.tracer.current_span
span = OpenTelemetry.tracer.current_span(some_context)

new_context = OpenTelemetry.tracer.context_with_span(span)
new_context = OpenTelemetry.tracer.context_with_span(span, parent_context: some_context)

OpenTelemetry.tracer.with_span(span) do |span, context|
  # Run this block with span set in the active context.
end

This requires the same number of characters as the first option (static methods on the Trace module). Superficially it looks pretty good, but not necessarily any better than the first option.

Methods on Tracer (using instrumentation's tracer instance).

span = tracer.current_span
span = tracer.current_span(some_context)

new_context = tracer.context_with_span(span)
new_context = tracer.context_with_span(span, parent_context: some_context)

tracer.with_span(span) do |span, context|
  # Run this block with span set in the active context.
end

This is obviously the shortest and most pleasing to type and look at. It has some conceptual problems, though.

These "methods" are all really simple functions of the explicit or implicit context and an explicit span. They don't create new spans or modify the span provided or returned in any way. The Tracer API's primary purpose is to create spans in the (ahem) context of an InstrumentationLibrary. Methods/functions that manipulate the binding of (implicit or explicit) contexts and spans are really outside of this scope.

In fact, the context has a broader scope than an individual tracer instance, and the current_span in the implicit context may (and probably will) have been created by a tracer other than either the global tracer or the tracer the user has in their hands. This is confusing IMO, and suggests the methods above do not belong on Tracer.

Another concern is that the existence of a global tracer will lead new users to misuse and misunderstand the API. In particular, the following pattern will lead to spans not associated with an InstrumentationLibrary:

OpenTelemetry.tracer.in_span('...') do |span, context|
  # Run this block with span set in the active context.
end

Given all that, I'd strongly prefer to move the current_span, context_with_span and with_span methods to be 'static' methods on OpenTelemetry::Trace. This means moving the CURRENT_SPAN_KEY constant to that module as well. I'd also propose removing current_span_key from ContextKeys. From my reading of its uses, we don't need to (and shouldn't) expose that on ContextKeys - people can just use the API on OpenTelemetry::Trace instead. I might be mistaken, but it looks like we can remove the ContextKeys module altogether.

@fbogsany
Copy link
Contributor

I've implemented the first option in #439. It is obviously a lot more invasive, but I think the result is better and more in the spirit of the spec.

@mwear
Copy link
Member Author

mwear commented Oct 16, 2020

I've implemented the first option in #439. It is obviously a lot more invasive, but I think the result is better and more in the spirit of the spec.

I agree. I'll go ahead and close this.

@mwear mwear closed this Oct 16, 2020
@mwear
Copy link
Member Author

mwear commented Oct 16, 2020

I guess one thing that isn't in #439 is the default tracer (the OpenTelemetry.tracer delegate). Is this something we're interested in? If so, I can bring this PR back in some form.

@fbogsany
Copy link
Contributor

I guess one thing that isn't in #439 is the default tracer (the OpenTelemetry.tracer delegate). Is this something we're interested in? If so, I can bring this PR back in some form.

I don't think that's something we should do. As mentioned above:

Another concern is that the existence of a global tracer will lead new users to misuse and misunderstand the API. In particular, the following pattern will lead to spans not associated with an InstrumentationLibrary:

OpenTelemetry.tracer.in_span('...') do |span, context|
  # Run this block with span set in the active context.
end

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