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 TraceOperation to sampled=true by default #2279

Merged
merged 2 commits into from
Sep 15, 2022

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented Sep 15, 2022

Fixes #2101

Motivation

In v0.x of ddtrace, the currently active trace, represented by the Context class, would always have a sampled == true by default, as it inherited its value from Span#sampled:

@sampled = span.sampled

And Span#sampled defaults to true:

@sampled = true

In v1.x, the currently active trace is now represented by a TraceOperation.
TraceOperation#sampled defaults to false:

@sampled = sampled.nil? ? false : sampled

And there's no mechanism to inherit sampling from the active span in v1.x.

This doesn't normally affect traces, as the PrioritySampler, which is the default sampler, always sets sampled = true:

# NOTE: We'll want to leave `trace.sampled = true` here; all spans for priority sampling must
# be sent to the agent. Otherwise metrics for traces will not be accurate, since the
# agent will have an incomplete dataset.
#
# We also ensure that the agent knows we that our `post_sampler` is not performing true sampling,
# to avoid erroneous metric upscaling.
trace.sampled = true

But, sampling checks are completely skipped if the sampling decision was made in an upstream service. We only check the sampler if the current span is a root span (parent_id == 0):

sample_trace(event_trace_op) if event_span_op && event_span_op.parent_id == 0

Because distributed traces won't have any spans with parent_id == 0, as the local root span inherits the parent_id from the upstream distributed span, we never invoked the PrioritySampler.

What does this PR do?

This PR sets TraceOperation#sampled to true by default, as having traces being sampled by default was the desired intent in first place.

@marcotc marcotc added the bug Involves a bug label Sep 15, 2022
@marcotc marcotc self-assigned this Sep 15, 2022
@marcotc marcotc requested a review from a team September 15, 2022 20:23
@marcotc marcotc merged commit 7725e6a into 1.4-stable Sep 15, 2022
@marcotc marcotc deleted the fix-distributed-priority-sampling branch September 15, 2022 22:14
@marcotc marcotc added this to the 1.4.1 milestone Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant