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

fix: untraced only works with parent-based sampler #1412

Merged
merged 8 commits into from
Jan 12, 2023
Merged

Conversation

fbogsany
Copy link
Contributor

@fbogsany fbogsany commented Dec 9, 2022

The untraced helper currently only works when a parent-based sampler is installed. As an example, it will not suppress spans if the ALWAYS_ON sampler is installed.

This fixes untraced to work regardless of the sampler. It does this by communicating via a private Context key. An additional helper untraced? is introduced to query the state of the key from the Context. The SDK Tracer#start_span implementation is updated to check for untraced? before delegating either to the API Tracer#start_span method or to TracerProvider#internal_start_span.

The OpenTelemetry JS implementation offers the same functionality implemented in the same way, and serves as inspiration for the implementation in this PR.

Copy link
Contributor

@arielvalentin arielvalentin left a comment

Choose a reason for hiding this comment

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

Assuming linter errors are addressed.

@fbogsany
Copy link
Contributor Author

fbogsany commented Dec 9, 2022

Assuming linter errors are addressed.

I blame Copilot.

@fbogsany
Copy link
Contributor Author

fbogsany commented Dec 9, 2022

We'll have to be careful with the version bumps when releasing this. We've added a method to the -common gem that the -sdk now depends on, but we haven't changed the -sdk interface.

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.

Shouldn't we add a dependency on -common to the sdk gemspec?

@fbogsany
Copy link
Contributor Author

fbogsany commented Dec 9, 2022

Shouldn't we add a dependency on -common to the sdk gemspec?

It's already there.

@ahayworth
Copy link
Contributor

Shouldn't we add a dependency on -common to the sdk gemspec?
It's already there.

🤦 Wow - it is indeed, I must have skipped over it when I was hunting for it. 🤦

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.

5 participants