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(span_links): ensures trace_flags is consistent with the sampling priority #3596

Merged
merged 2 commits into from
Apr 26, 2024

Conversation

mabdinur
Copy link
Contributor

What does this PR do?

Currently the SpanLink.trace_flags is set using TraceDigest.trace_flags. The trace_flag field in a trace_digest store sampling decisions propagated by tracecontext distributed headers. Therefore for non-remote spans and spans generated via datadog/b3 distributed tracing headers will have a trace_flag of nil even when a sampling decision exists.

SpanLink.trace_flag must have one of the following values:

  • nil If there is NO sampling decision on the linked span
  • 1 If the linked span is sampled
  • 0 If the linked span is NOT sampled (span will be dropped by the agent)

Since trace_flag is ONLY a function of sampling priority, we can ignore TraceDigest.trace_flags and always use sampling_priority to compute the valid when a span link is generated.

@mabdinur mabdinur changed the title fix(span_links): ensure links evaluate sampling priority when setting trace_flags fix(span_links): ensures trace_flags is consistent with the sampling priority Apr 17, 2024
@mabdinur mabdinur force-pushed the munir/ensure-span-links-use-sampling-priority branch from 71a8664 to abae73a Compare April 17, 2024 17:42
@mabdinur mabdinur marked this pull request as ready for review April 17, 2024 18:16
@mabdinur mabdinur requested a review from a team as a code owner April 17, 2024 18:16
@mabdinur mabdinur force-pushed the munir/ensure-span-links-use-sampling-priority branch from abae73a to e7a96b4 Compare April 17, 2024 19:23
Base automatically changed from 2.0 to master April 18, 2024 14:32
@mabdinur mabdinur requested a review from TonyCTHsu April 18, 2024 21:17
Copy link
Contributor

@TonyCTHsu TonyCTHsu left a comment

Choose a reason for hiding this comment

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

The code is generally good.

Question: When would digest be nil? I thinking about using null object pattern to avoid those nil check

@mabdinur
Copy link
Contributor Author

mabdinur commented Apr 19, 2024

The code is generally good.

Question: When would digest be nil? I thinking about using null object pattern to avoid those nil check

Digest shouldn't be nil. I'll make this change. We can make digest a required argument since the span_link class has not been released.

@zarirhamza zarirhamza merged commit ccdc38a into master Apr 26, 2024
164 of 165 checks passed
@zarirhamza zarirhamza deleted the munir/ensure-span-links-use-sampling-priority branch April 26, 2024 12:24
@github-actions github-actions bot added this to the 2.0.0 milestone Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants