set 128-bit trace_id to true by default #3266

ekump · 2023-11-20T19:35:36Z

What does this PR do?
Changes the default configuration for 128-bit trace_id generation to true.

Motivation:
Part of Datadog's migration to W3C/128-bit tracers for OTEL compatibility.

Additional Notes:
When using 128-bit IDs internally, we propagate the 64-bit version in x-datadog-tags, which we do not do for 64-bit IDs. Some unit tests had to be updated to reflect this.

How to test the change?
Since this changes default behavior you just have to use the tracer with default settings to see the change.

For Datadog employees:

If this PR touches code that signs or publishes builds or packages, or handles
credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

TonyCTHsu

Generally fine!

TonyCTHsu · 2023-11-20T20:27:56Z

spec/datadog/tracing/distributed/datadog_spec.rb

@@ -103,6 +109,8 @@
        end

        context 'nil' do
+          include_context 'with 128-bit trace_id generation disabled'


I would prefer making examples more explicit by repeating

before do Datadog.configure { |c| c.tracing.trace_id_128_bit_generation_enabled = false } end

Instead of adding an indirection

include_context 'with 128-bit trace_id generation disabled'

I agree here, given c.tracing.trace_id_128_bit_generation_enabled = false reads pretty much the same as with 128-bit trace_id generation disabled.

I disagree. I think the shared_context is a bit clearer in communicating intentions with the tests as well as removes some clutter. But since the actual context is so small I don't think it's the end of the world to revert this.

TonyCTHsu · 2023-11-20T20:31:39Z

lib/datadog/tracing/distributed/datadog.rb

@@ -46,11 +46,18 @@ def inject!(digest, data)
          data[@sampling_priority_key] = digest.trace_sampling_priority.to_s if digest.trace_sampling_priority
          data[@origin_key] = digest.trace_origin.to_s if digest.trace_origin

-          build_tags(digest).tap do |tags|
-            inject_tags!(tags, data) unless tags.empty?
+          begin


Is there any reason for this change?

When using 128-bit ids, if the tags are invalid because they aren't a hash it will generate an exception when we attempt to merge them here.

It's possible to have an encoding error at either the build_tags or the inject_tags! steps, so I moved the rescue up to here.

Interesting, I was not expecting build_tags would failed. Perhaps bad data from arbitrary digest?

I don't mind this changes that push the rescue logic higher up, but I think it slightly changing the contract of this method. Although inject! suggests this method contains side effect, the original method returns data explicitly(with or without exception). The changes with else statement alter the return value when there is an exception thrown.

Perhaps just return data in the last line explicitly without the else statement?

Closing the loop on this: Spoke with Tony and we decided to revert this and update the test instead. The likelihood of tags not being a hash is extremely unlikely, so we shouldn't be generating any exceptions in build_tags.

ekump · 2023-11-21T02:04:53Z

spec/datadog/opentelemetry_spec.rb

+            'x-datadog-sampling-priority' => '1',
+            'x-datadog-tags' => '_dd.p.dm=-0,_dd.p.tid=' +
+              Datadog::Tracing::Utils::TraceId.to_high_order(Datadog::Tracing.active_trace.id).to_s(16),
+            'x-datadog-trace-id' => Datadog::Tracing::Utils::TraceId.to_low_order(Datadog::Tracing.active_trace.id).to_s,


@marcotc I added a helper for this but it's scoped to Tracing::Contrib so I can't / shouldn't use it outside of that.

TonyCTHsu · 2023-11-21T12:14:15Z

spec/datadog/opentelemetry_spec.rb

@@ -339,18 +339,21 @@
      describe '#inject' do
        subject(:inject) { ::OpenTelemetry.propagation.inject(carrier) }
        let(:carrier) { {} }
-
+        let(:headers) do


This is fine, but as a reminder. let would cache the result when first invoked, this could potentially cause stale values within a test life cycle if not paying attention, especially the data depends on a variable Datadog::Tracing.active_trace that changes with context.

I recommend

def headers { ... } end

Good callout, updated the test.

TonyCTHsu · 2023-11-21T19:27:04Z

spec/datadog/tracing/distributed/datadog_spec.rb

@@ -185,7 +195,7 @@
          end

          context 'with invalid tags' do
-            let(:tags) { 'not_a_tag_hash' }
+            let(:tags) { { 'key with=spaces' => 'value' } }


marcotc · 2023-11-21T19:27:43Z

spec/datadog/tracing/contrib/support/tracer_helpers.rb

+    # Wraps call to Tracing::Utils::TraceId.to_low_order for better test readability
+    def low_order_trace_id(trace_id)
+      Datadog::Tracing::Utils::TraceId.to_low_order(trace_id)
+    end
+
+    # Wraps call to Tracing::Utils::TraceId.to_high_order and converts to hex
+    # for better test readability
+    def high_order_hex_trace_id(trace_id)
+      Datadog::Tracing::Utils::TraceId.to_high_order(trace_id).to_s(16)
+    end


I think these helpers are related to tracing, and not specifically contrib, as the 128-bit trace id generation is a part of core tracing.

I suggest moving these helpers to https://github.com/DataDog/dd-trace-rb/blob/master/spec/support/tracer_helpers.rb instead.

codecov-commenter · 2023-11-21T21:48:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1cba6bc) 98.22% compared to head (139e795) 98.22%.
Report is 8 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3266   +/-   ##
=======================================
  Coverage   98.22%   98.22%           
=======================================
  Files        1253     1253           
  Lines       72198    72290   +92     
  Branches     3350     3361   +11     
=======================================
+ Hits        70916    71010   +94     
+ Misses       1282     1280    -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ekump requested review from a team as code owners November 20, 2023 19:35

github-actions bot added the tracing label Nov 20, 2023

ekump marked this pull request as draft November 20, 2023 19:38

TonyCTHsu reviewed Nov 20, 2023

View reviewed changes

TonyCTHsu added this to the 1.17.0 milestone Nov 20, 2023

ekump force-pushed the ekump/128-bit-ids-by-default branch 2 times, most recently from a6f4814 to 4f65aba Compare November 21, 2023 01:47

ekump marked this pull request as ready for review November 21, 2023 02:00

ekump commented Nov 21, 2023

View reviewed changes

TonyCTHsu reviewed Nov 21, 2023

View reviewed changes

drichards-87 approved these changes Nov 21, 2023

View reviewed changes

ekump force-pushed the ekump/128-bit-ids-by-default branch from b1884ac to 3a617f1 Compare November 21, 2023 18:08

set 128-bit trace_id to true by default

007bb9c

ekump force-pushed the ekump/128-bit-ids-by-default branch from 3a617f1 to 95b2d08 Compare November 21, 2023 18:22

ekump added 2 commits November 21, 2023 13:36

update contrib tests for 128-bit default trace_ids

1ecd978

update opentracer to work with 128-bit trace_ids

45e2cc9

ekump force-pushed the ekump/128-bit-ids-by-default branch from 95b2d08 to 45e2cc9 Compare November 21, 2023 18:36

TonyCTHsu approved these changes Nov 21, 2023

View reviewed changes

marcotc reviewed Nov 21, 2023

View reviewed changes

marcotc approved these changes Nov 21, 2023

View reviewed changes

clenaup tests for 128-bit default enabling

139e795

ekump merged commit 3becb40 into master Nov 21, 2023
218 checks passed

ekump deleted the ekump/128-bit-ids-by-default branch November 21, 2023 22:32

anmarchenko mentioned this pull request Nov 22, 2023

disable 128-bit trace id generation in CI mode DataDog/datadog-ci-rb#70

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set 128-bit trace_id to true by default #3266

set 128-bit trace_id to true by default #3266

ekump commented Nov 20, 2023 •

edited

Loading

TonyCTHsu left a comment

TonyCTHsu Nov 20, 2023

marcotc Nov 21, 2023

ekump Nov 21, 2023

TonyCTHsu Nov 20, 2023

ekump Nov 21, 2023

TonyCTHsu Nov 21, 2023 •

edited

Loading

ekump Nov 21, 2023

ekump Nov 21, 2023

TonyCTHsu Nov 21, 2023

ekump Nov 21, 2023

TonyCTHsu Nov 21, 2023

marcotc Nov 21, 2023

codecov-commenter commented Nov 21, 2023

set 128-bit trace_id to true by default #3266

set 128-bit trace_id to true by default #3266

Conversation

ekump commented Nov 20, 2023 • edited Loading

TonyCTHsu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonyCTHsu Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 21, 2023

Codecov Report

ekump commented Nov 20, 2023 •

edited

Loading

TonyCTHsu Nov 21, 2023 •

edited

Loading