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

128 bits trace id #2543

Merged
merged 41 commits into from
Mar 3, 2023
Merged

128 bits trace id #2543

merged 41 commits into from
Mar 3, 2023

Conversation

TonyCTHsu
Copy link
Contributor

@TonyCTHsu TonyCTHsu commented Jan 11, 2023

What does this PR do?

Support 128-bit trace id. With this change, our trace is backward compatible with 128-bit trace id, which is used for W3C trace context and b3 propagation.

  1. Implement backward compatible distributed tracing propagation with 128-bit trace id
  2. Implement backward compatible means for sending spans to agent endpoint
  3. Enable 128-bit trace id generation with environment variable DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED
  4. Option to log 128-bit trace id with environment variable DD_TRACE_128_BIT_TRACEID_LOGGING_ENABLED(Not yet supported)

@TonyCTHsu TonyCTHsu self-assigned this Jan 24, 2023
@TonyCTHsu TonyCTHsu force-pushed the tonycthsu/128bits-trace-id-propagation branch from f9b081a to 82c343e Compare January 24, 2023 15:24
@TonyCTHsu TonyCTHsu force-pushed the tonycthsu/128bits-trace-id-propagation branch from f97fbf3 to 7396eb3 Compare March 2, 2023 14:47
@TonyCTHsu TonyCTHsu force-pushed the tonycthsu/128bits-trace-id-propagation branch from 7396eb3 to 4f8a8ad Compare March 2, 2023 15:04
@codecov-commenter
Copy link

codecov-commenter commented Mar 2, 2023

Codecov Report

Merging #2543 (b618b70) into master (94721a7) will increase coverage by 0.01%.
The diff coverage is 99.60%.

@@            Coverage Diff             @@
##           master    #2543      +/-   ##
==========================================
+ Coverage   98.08%   98.09%   +0.01%     
==========================================
  Files        1166     1166              
  Lines       63832    64198     +366     
  Branches     2849     2861      +12     
==========================================
+ Hits        62607    62974     +367     
+ Misses       1225     1224       -1     
Impacted Files Coverage Δ
lib/datadog/tracing/distributed/fetcher.rb 100.00% <ø> (ø)
spec/datadog/tracing/distributed/fetcher_spec.rb 100.00% <ø> (ø)
lib/ddtrace/transport/trace_formatter.rb 98.92% <75.00%> (-1.08%) ⬇️
lib/ddtrace/transport/serializable_trace.rb 98.24% <85.71%> (+0.13%) ⬆️
lib/datadog/tracing/configuration/ext.rb 100.00% <100.00%> (ø)
lib/datadog/tracing/configuration/settings.rb 94.87% <100.00%> (+0.27%) ⬆️
lib/datadog/tracing/contrib/grpc/integration.rb 100.00% <100.00%> (ø)
lib/datadog/tracing/correlation.rb 100.00% <100.00%> (ø)
lib/datadog/tracing/distributed/b3_multi.rb 100.00% <100.00%> (ø)
lib/datadog/tracing/distributed/b3_single.rb 100.00% <100.00%> (ø)
... and 28 more

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

@@ -47,6 +47,7 @@ def format!
tag_rate_limiter_rate!
tag_sample_rate!
tag_sampling_decision_maker!
tag_high_order_trace_id!
Copy link
Member

Choose a reason for hiding this comment

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

We should also adjust the the span.trace_id on all spans in the TraceFormatter, so the TraceFormatter always makes sure the TraceSegment provided is ready to be serialized without any changes.

This simplifies the serializer's logic.

Copy link
Contributor Author

@TonyCTHsu TonyCTHsu Mar 3, 2023

Choose a reason for hiding this comment

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

I actually hesitate to commit to this change since TraceFormatter is performing at the trace level attributes which is a constraint of agent's API. I would prefer the current approach of decorating SerailizableSpan than mutating Span from TraceFormatter.

@TonyCTHsu TonyCTHsu force-pushed the tonycthsu/128bits-trace-id-propagation branch from 86ca9af to 0aab0da Compare March 3, 2023 13:12
@TonyCTHsu TonyCTHsu force-pushed the tonycthsu/128bits-trace-id-propagation branch from 0aab0da to c9ea54b Compare March 3, 2023 13:17
@TonyCTHsu TonyCTHsu marked this pull request as ready for review March 3, 2023 16:44
@TonyCTHsu TonyCTHsu requested a review from a team March 3, 2023 16:44
@TonyCTHsu TonyCTHsu merged commit 3514f9f into master Mar 3, 2023
@TonyCTHsu TonyCTHsu deleted the tonycthsu/128bits-trace-id-propagation branch March 3, 2023 22:43
@github-actions github-actions bot added this to the 1.10.0 milestone Mar 3, 2023
@TonyCTHsu TonyCTHsu mentioned this pull request Mar 6, 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.

3 participants