Support for traces_sampler option #910
Conversation
solnic
force-pushed
the
traces-sampler-support
branch
from
d01bcbd to
c0489ff
Compare
solnic
force-pushed
the
traces-sampler-support
branch
from
c0489ff to
bfd2cc9
Compare
solnic
force-pushed
the
traces-sampler-support
branch
from
bfd2cc9 to
2ebef7d
Compare
solnic
force-pushed
the
traces-sampler-support
branch
from
2ebef7d to
27bfc44
Compare
|
Are we close this time? Had everything working smoothly working in prod using the last branch, but lost it all with the revert. Hoping to see something completed this week if possible... |
lib/sentry/opentelemetry/sampler.ex
Outdated
| decision = if trace_sampled, do: :record_and_sample, else: :drop | ||
|
|
||
| {decision, [], tracestate} | ||
| if traces_sampler do |
There was a problem hiding this comment.
the traces_sampler has to be invoked only for root spans, see
https://github.com/getsentry/sentry-python/blob/c21525e4252805561d83cd2d726020dd41aa074d/sentry_sdk/opentelemetry/sampler.py#L246
so in this case I think the :inherit case shouldn't exist but only the :no_trace case below should invoke the traces_sampler
lib/sentry/opentelemetry/sampler.ex
Outdated
| } | ||
|
|
||
| if attributes && map_size(attributes) > 0 do | ||
| Map.merge(sampling_context, attributes) |
There was a problem hiding this comment.
attributes is already part of transaction_context above, why this merge?
There was a problem hiding this comment.
@sl0thentr0py accidental left-over after a refactoring :)
lib/sentry/opentelemetry/sampler.ex
Outdated
| result = call_traces_sampler(traces_sampler, sampling_context) | ||
| sample_rate = normalize_sampler_result(result) | ||
|
|
||
| cond do |
There was a problem hiding this comment.
could you just use make_sampling_decision after getting the rate here? wanna avoid duplication here especially
solnic
force-pushed
the
traces-sampler-support
branch
from
06a3499 to
38df5df
Compare
solnic
force-pushed
the
traces-sampler-support
branch
from
38df5df to
36a6b4d
Compare
@jwaldrip yes it's happening this time 🎉 we just need to wrap up integration tests and merge the big #902 again to master and this time it'll include properly implemented sampler. Then we'll push a new release with OTel support in beta 😄 |
* Revert "Revert "Add SpanProcessor for OpenTelemetry (#875)"" This reverts commit 2ced90e. * Better span sampler (#903) * Better span sampler This updates OpenTelemetry.Sampler so that it uses `traces_sample_rate` for sampling spans before they get sent to the span processor. This way we're not processing spans when they are dropped during sampling, which was previously the case as the Client was responsible for sampling just before attemping to send a transaction. * Enhance sampling logic to record discarded transactions * Clarify trace-level sampling decision-making Previously it incorrectly referred to "parent" where in reality we're dealing with spans from the same trace and respect sampling decisions that were already made for a given trace. This is not the same thing as parent/child spans that we're handling in the SpanProcessor as this type of relationship is established post-sampling. * Remove unnecessary sleep calls in sampler tests * Fix flaky test * Clean up make_sampling_decision * Simplify client reports for dropped transactions * Update traces_sample_rate default value to be nil * Account for `nil` sample_rate when sampling * Use put_test_config * Update traces_sample_rate spec to allow nil value * Support for traces_sampler option (#910) * Add support for `traces_sampler` config option * Update docs * Remove unnecessary attribute merging * Call traces_sampler only for root spans * Reuse sampling decision logic * Add test for handling invalid traces_sampler return values * Add tests for SamplingContext access functions * Add basic integration tests for OpenTelemetry (#912) * Update CHANGELOG
#skip-changelog