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

Set sampler other than the default #321

Closed
Asafb26 opened this issue Aug 19, 2020 · 8 comments
Closed

Set sampler other than the default #321

Asafb26 opened this issue Aug 19, 2020 · 8 comments
Labels

Comments

@Asafb26
Copy link
Contributor

Asafb26 commented Aug 19, 2020

Hello,
I can't find and setter to the sampler in TracerProvider, Is there any option to change the default sampler within the SDK's configuration?

@fbogsany
Copy link
Contributor

def initialize(sampler: DEFAULT_SAMPLER,
and

So something like:

sampler = OpenTelemetry::SDK::Trace::Samplers.parent_or_else(OpenTelemetry::SDK::Trace::Samplers.probability(0.001))
trace_config = OpenTelemetry::SDK::Trace::Config::TraceConfig.new(sampler: sampler)
OpenTelemetry.tracer_provider.active_trace_config = trace_config

The heavily nested namespaces are decidedly unpleasant when you see them like this 😞 . The spec is less prescriptive than it used to be about configuration, so we should carefully consider whether this should be more convenient. To my mind, I'd prefer to configure the tracer_provider directly, e.g.:

OpenTelemetry.tracer_provider.sampler = OpenTelemetry::SDK::Trace::Samplers.parent_or_else(OpenTelemetry::SDK::Trace::Samplers.probability(0.001))

There's still heavy nesting of the Samplers namespace - I don't have good ideas of how to fix that - I played around with a DSL, but it seems less direct and more complicated, and not as useful if you have custom samplers.

I suppose we could tweak the interface of parent_or_else to handle all the built-in samplers:

OpenTelemetry.tracer_provider.sampler = OpenTelemetry::SDK::Trace::Samplers.parent_or_else(probability: 0.001)
OpenTelemetry.tracer_provider.sampler = OpenTelemetry::SDK::Trace::Samplers.parent_or_else(:always_on)
OpenTelemetry.tracer_provider.sampler = OpenTelemetry::SDK::Trace::Samplers.parent_or_else(:always_off)
OpenTelemetry.tracer_provider.sampler = OpenTelemetry::SDK::Trace::Samplers.parent_or_else(MyCustomSampler.new)

Note that the spec for Probability Sampler has changed since our implementation, and it no longer needs to handle the parent case -- that's expected to be done by wrapping it in parent_or_else.

@Asafb26
Copy link
Contributor Author

Asafb26 commented Aug 19, 2020

Thanks, what about exposing the sampler as an SDK configuration method?
something like

OpenTelemetry::SDK.configure do |config|
  config.sampler OpenTelemetry::SDK::Trace::Samplers.parent_or_probability(0.001)
  ...
end

@fbogsany
Copy link
Contributor

Thanks, what about exposing the sampler as an SDK configuration method?

We can do that, but the spec also allows the sampler to be replaced after configuration (MAY rather than MUST, but we already support it). Also, I think we want to be clear that this is the trace sampler.

Anyway, I'll address the SDK configuration in a separate PR.

@fbogsany
Copy link
Contributor

And actually, I think we want to make the configuration even less wordy by providing helpers for the standard samplers:

OpenTelemetry::SDK.configure do |c|
  c.trace_sampler = c.parent_or_else(c.probability(0.001))
  ...
end

@johnnyshields
Copy link
Contributor

Is this implemented yet? i.e. is there now a way to set overall sampling probability in the global OpenTelemetry::SDK configuration?

@ahayworth
Copy link
Contributor

@johnnyshields I actually don't think the configurator block supports this yet - I believe the best way to do it globally is via environment variables. See the test suite here for examples:

it 'configures samplers from environment' do

@fbogsany This is probably something we should still plan on doing, yeah?

@johnnyshields
Copy link
Contributor

@ahayworth thank you. Would be great if configurator could support it as well.

Copy link
Contributor

👋 This issue has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this issue will be closed eventually by the stale bot.

@github-actions github-actions bot added the stale label Apr 27, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants