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

Use first valid extracted style for distributed tracing #2879

Merged
merged 4 commits into from
Sep 1, 2023

Conversation

TonyCTHsu
Copy link
Contributor

@TonyCTHsu TonyCTHsu commented May 30, 2023

What does this PR do?

Currently, when extracting from multiple contexts, our propagation would validate against each other to ensure the trace id and span id are identical. This approach ensure that if user includes Datadog proprietary propagation, those proprietary tags would be propagated and the user will remain having full functionalities that Datadog offers.

If there is a mismatched being detected, since we are not able to identify the source of truth, it would leads to a broken distributed trace. The third scenario on the right illustrated a possible scenario with a mismatched detected given 128 bits trace ID upstream.

128-bit-trace-id-conflict

This PR changes the propagation behaviour by removing the validation phase for multiple extractions. This means the first successful extraction would always wins. W3C trace context propagation is fully compatible but user prioritizing B3 extraction would lose Datadog's proprietary tags.

System test for trace header precedence: DataDog/system-tests#1454

@TonyCTHsu TonyCTHsu changed the title Fix distributed tracing extraction when conflict Change distributed tracing extraction when conflict May 30, 2023
@marcotc
Copy link
Member

marcotc commented Jun 14, 2023

This is a change of behavior (users that currently have conflicting propagation headers will see a behavior change IF: they have multiple extraction styles configured and Datadog is one of their extraction styles and Datadog is not the first extraction style).

This is unlikely to happen, but not impossible.

Given that all other traces behave like this PR, taking the extraction value from the first extraction style that is successfully parsed, I don't see much issue with this given. External services with Datadog propagation, written in other languages, are all already using the first extracted style, this then PR increases configuration consistency in heterogeneous deployments.

@marcotc marcotc changed the title Change distributed tracing extraction when conflict Use first valid extracted style for distributed tracing Jun 14, 2023
@TonyCTHsu TonyCTHsu marked this pull request as ready for review July 7, 2023 17:17
@TonyCTHsu TonyCTHsu requested a review from a team July 7, 2023 17:17
@marcotc
Copy link
Member

marcotc commented Jul 11, 2023

Note here: check if there are any system-tests that cover this change (or if the Ruby tracer has been skipped in any relevant system-test because this change wasn't implemented before).

@codecov-commenter
Copy link

Codecov Report

Merging #2879 (ad5ad9a) into master (8d3ef4f) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2879      +/-   ##
==========================================
- Coverage   98.08%   98.07%   -0.01%     
==========================================
  Files        1301     1301              
  Lines       72065    72058       -7     
  Branches     3310     3307       -3     
==========================================
- Hits        70683    70671      -12     
- Misses       1382     1387       +5     
Impacted Files Coverage Δ
lib/datadog/tracing/distributed/propagation.rb 91.89% <100.00%> (+0.78%) ⬆️
...ec/datadog/tracing/distributed/propagation_spec.rb 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@TonyCTHsu TonyCTHsu added this to the 1.13.0 milestone Jul 20, 2023
@TonyCTHsu TonyCTHsu removed this from the 1.13.0 milestone Jul 27, 2023
@TonyCTHsu TonyCTHsu force-pushed the tonycthsu/fix-distributed-tracing-extraction branch from ad5ad9a to 1449ea4 Compare August 3, 2023 11:14
GustavoCaso and others added 2 commits August 3, 2023 13:26
…ccesing tracing.distributed_tracing.propagation_inject_style and tracing.distributed_tracing.propagation_extract_style
@TonyCTHsu TonyCTHsu added this to the 1.13.1 milestone Aug 3, 2023
@TonyCTHsu TonyCTHsu modified the milestones: 1.13.1, 1.14.0 Aug 8, 2023
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

With system-tests coverage, I'm confident this will bring us closer to how Datadog advertises propagation precedence: DataDog/system-tests#1454

@TonyCTHsu TonyCTHsu removed this from the 1.14.0 milestone Aug 23, 2023
@TonyCTHsu TonyCTHsu merged commit 0318c77 into master Sep 1, 2023
361 of 362 checks passed
@TonyCTHsu TonyCTHsu deleted the tonycthsu/fix-distributed-tracing-extraction branch September 1, 2023 09:22
@github-actions github-actions bot added this to the 1.15.0 milestone Sep 1, 2023
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.

None yet

4 participants