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: Add Tracer.non_recording_span to API #799

Merged
merged 8 commits into from
Jun 8, 2021
Merged

Conversation

fbogsany
Copy link
Contributor

@fbogsany fbogsany commented Jun 8, 2021

Fixes #793

There's a lot going on here. The TL;DR is that

  1. we had some crufty code that was long overdue for cleanup; and
  2. in order to comply with the spec, we need to make non_recording_span an instance method on Tracer (because Spans can only be created by a Tracer), and we had a bunch of places that abused Span.new (which we marked @api private) and didn't have a Tracer instance handy.

Copy link
Contributor

@ahayworth ahayworth left a comment

Choose a reason for hiding this comment

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

On a technical level, I think this change is 👍

On a spec level, it's a tad confusing to me so I feel like someone a little more knowledgeable about the spec should also sign-off. 😄

@fbogsany
Copy link
Contributor Author

fbogsany commented Jun 8, 2021

In light of #793 (comment) (TL;DR this method doesn't have to be an instance method on Tracer), do we prefer to:

  1. Keep things the way they are in this PR (instance method on Tracer)
  2. Make the method static on Tracer (leads to calls like OpenTelemetry::Trace::Tracer.non_recording_span(...)
  3. Move the method to Trace (leads to calls like OpenTelemetry::Trace.non_recording_span(...))

My 2c: static method on Tracer doesn't help much; method on Trace aligns with helpers like with_span and means we don't need to sprinkle Tracer instances around.

@robertlaurin
Copy link
Contributor

Move the method to Trace (leads to calls like OpenTelemetry::Trace.non_recording_span(...))

Seems like the most convenient/consistent choice.

@fbogsany fbogsany merged commit 2bf0e6a into main Jun 8, 2021
@fbogsany fbogsany deleted the non_recording_span branch June 8, 2021 14:59
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.

Explicitly define a NonPropagatingSpan function
3 participants