RUM-1829 Provide 128 bits support for the trace ids in the Tracing sdk #2089

mariusc83 · 2024-06-19T09:10:07Z

What does this PR do?

In this PR we are adding the 128 bits support for our OpenTelemetry and OpenTracing manual APIs + the distributed traces handled through our DatadogInterceptor. The PR contains 3 parts:

128 bits support for OpenTracing manual API + tests + integration tests updated
128 bits support for OpenTelemetry manual API + tests + integration tests updated
128 bits support for the distributed traces in the DatadogInterceptor

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

0xnm · 2024-06-19T09:19:07Z

features/dd-sdk-android-trace/src/main/java/com/datadog/opentracing/DDTracer.java

@@ -50,6 +51,9 @@
 public class DDTracer implements io.opentracing.Tracer, Closeable, Tracer {
    // UINT64 max value


seems like comment should be updated?

0xnm · 2024-06-19T09:25:39Z

...dd-sdk-android-trace/src/main/java/com/datadog/opentracing/propagation/DatadogHttpCodec.java

+                int samplingPriority = PrioritySampling.UNSET;
+                String origin = null;
+                String mostSignificantTraceId = "0";
+                String lessSignificantTraceId = "0";


Suggested change

String lessSignificantTraceId = "0";

String leastSignificantTraceId = "0";

0xnm · 2024-06-19T09:26:56Z

features/dd-sdk-android-trace/src/main/java/com/datadog/opentracing/propagation/HttpCodec.java

-        || parsedValue.compareTo(DDTracer.TRACE_ID_MAX) > 0) {
+        || parsedValue.compareTo(DDTracer.SPAN_ID_MAX) > 0) {


why this change is needed?

check the method name...this validates a 64bits number and we introduced another one below for 128 bits. We also introduced a SPAN_ID_MAX and TRACE_ID_MAX (one for 64 and the other for 128). Maybe I should re - name these TRACE_ID_64 and TRACE_ID_128 ?

naming should be changed indeed, because using trace word for the lower boundary and span word for the upper boundary is kind of confusing.

0xnm · 2024-06-19T09:27:45Z

...oid-trace/src/main/kotlin/com/datadog/android/trace/internal/domain/event/BigIntegerUtils.kt

+    private const val HEX_RADIX = 16
+    private val LONG_MASK = BigInteger("ffffffffffffffff", HEX_RADIX)
+
+    fun lessSignificantUnsignedLongAsHexa(traceId: BigInteger): String {


Suggested change

fun lessSignificantUnsignedLongAsHexa(traceId: BigInteger): String {

fun leastSignificantUnsignedLongAsHex(traceId: BigInteger): String {

0xnm · 2024-06-19T09:28:27Z

...oid-trace/src/main/kotlin/com/datadog/android/trace/internal/domain/event/BigIntegerUtils.kt

+        @Suppress("MagicNumber")
+        return traceId.and(LONG_MASK).toString(HEX_RADIX)
+    }
+
+    fun lessSignificantUnsignedLongAsDecimal(traceId: BigInteger): String {
+        @Suppress("MagicNumber")
+        return traceId.and(LONG_MASK).toString()
+    }
+
+    fun mostSignificantUnsignedLongAsHexa(traceId: BigInteger): String {
+        @Suppress("MagicNumber")
+        return traceId.shiftRight(LONG_BITS_SIZE).toString(HEX_RADIX)


is Suppress(MagicNumber) still needed here? all the arguments are named

0xnm · 2024-06-19T09:34:32Z

...es/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/trace/assertj/SpanEventAssert.kt

@@ -35,16 +36,27 @@ internal class SpanEventAssert(actual: SpanEvent) :
        return this
    }

-    fun hasTraceId(traceId: String): SpanEventAssert {
+    fun hasLessSignificantTraceId(traceId: String): SpanEventAssert {


Suggested change

fun hasLessSignificantTraceId(traceId: String): SpanEventAssert {

fun hasLeastSignificantTraceId(traceId: String): SpanEventAssert {

0xnm · 2024-06-19T09:34:40Z

...es/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/trace/assertj/SpanEventAssert.kt

        assertThat(actual.traceId)
            .overridingErrorMessage(
-                "Expected SpanEvent to have traceId: $traceId" +
+                "Expected SpanEvent to have less significant traceId: $traceId" +


Suggested change

"Expected SpanEvent to have less significant traceId: $traceId" +

"Expected SpanEvent to have least significant traceId: $traceId" +

0xnm · 2024-06-19T09:34:56Z

...es/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/trace/assertj/SpanEventAssert.kt

                    " but instead was: ${actual.traceId}"
            )
            .isEqualTo(traceId)
        return this
    }

+    fun hasMostSignificantTraceId(traceId: String): SpanEventAssert {
+        val lessSignificantTraceId = actual.meta.additionalProperties[TRACE_ID_META_KEY]


Suggested change

val lessSignificantTraceId = actual.meta.additionalProperties[TRACE_ID_META_KEY]

val mostSignificantTraceId = actual.meta.additionalProperties[TRACE_ID_META_KEY]

0xnm · 2024-06-19T09:36:12Z

sample/kotlin/src/main/kotlin/com/datadog/android/sample/SampleApplication.kt

+        TracingHeaderType.B3,
+        TracingHeaderType.B3MULTI


do we have this set up on the backend side? is it useful?

nope this was just for tests will be removed in the actual PR

0xnm · 2024-06-19T09:37:06Z

sample/kotlin/src/main/kotlin/com/datadog/android/sample/SampleApplication.kt

+                firstPartyHostsWithHeaderType = tracedHosts.associateWith {
+                    tracingHeaderTypes
+                }


alternatively we can set them up in the core configuration

codecov-commenter · 2024-06-24T11:59:45Z

Codecov Report

Attention: Patch coverage is 72.36842% with 42 lines in your changes missing coverage. Please review.

Project coverage is 70.18%. Comparing base (fa1a5f0) to head (446a7b4).
Report is 2 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2089      +/-   ##
===========================================
+ Coverage    69.68%   70.18%   +0.49%     
===========================================
  Files          718      721       +3     
  Lines        26662    26759      +97     
  Branches      4468     4485      +17     
===========================================
+ Hits         18579    18779     +200     
+ Misses        6857     6733     -124     
- Partials      1226     1247      +21

Files	Coverage Δ
.../android/trace/opentelemetry/OtelTracerProvider.kt	`96.50% <100.00%> (ø)`
.../kotlin/com/datadog/android/trace/AndroidTracer.kt	`96.00% <100.00%> (ø)`
...al/domain/event/CoreTracerSpanToSpanEventMapper.kt	`88.66% <100.00%> (-0.81%)`	⬇️
...e/internal/domain/event/DdSpanToSpanEventMapper.kt	`91.80% <100.00%> (+4.30%)`	⬆️
...n/com/datadog/android/okhttp/DatadogInterceptor.kt	`73.47% <100.00%> (ø)`
...datadog/android/okhttp/trace/TracingInterceptor.kt	`81.99% <100.00%> (+0.07%)`	⬆️
...rc/main/java/com/datadog/opentracing/DDTracer.java	`58.82% <91.67%> (+1.18%)`	⬆️
...m/datadog/opentracing/propagation/B3HttpCodec.java	`5.77% <0.00%> (ø)`
.../datadog/opentracing/propagation/B3MHttpCodec.java	`0.00% <0.00%> (-6.00%)`	⬇️
...dog/opentracing/propagation/HaystackHttpCodec.java	`0.00% <0.00%> (ø)`
... and 7 more

... and 30 files with indirect coverage changes

jonathanmos

Nice work! I left a few comments/questions

...dd-sdk-android-trace/src/main/java/com/datadog/opentracing/propagation/DatadogHttpCodec.java

jonathanmos · 2024-06-24T12:44:06Z

...oid-trace/src/main/kotlin/com/datadog/android/trace/internal/domain/event/BigIntegerUtils.kt

+    // BigInteger overflow cannot happen in this context
+
+    @Suppress("SwallowedException")
+    fun leastSignificant64BitsAsHex(traceId: BigInteger): String {


minor: we could add 'safe' to these method names to indicate they won't throw when called

good call but I am afraid I am going to run of out space for that method name. Can you give me an example of how this method should be named in that case ?

...trace/src/test/kotlin/com/datadog/android/trace/internal/domain/event/BigIntegerUtilsTest.kt

...android-trace/src/test/kotlin/com/datadog/android/utils/forge/DDSpanContextForgeryFactory.kt

jonathanmos · 2024-06-24T12:52:22Z

integrations/dd-sdk-android-okhttp-otel/transitiveDependencies

@@ -2,13 +2,11 @@ Dependencies List

 com.squareup.okhttp3:okhttp:4.11.0                              :  768 Kb


I've seen this particular change in a few prs unintentionally so just making sure -> in this pr we want this change?

As I replied to Nikita above not sure why, need to check what's going on.

jonathanmos · 2024-06-24T12:54:03Z

...droid-okhttp/src/test/kotlin/com/datadog/android/okhttp/internal/utils/SpanContextExtTest.kt

+
+    @Test
+    fun `M return empty string W traceIdAsHexString() { ddSpanContext id is null}`() {
+        // When


Should be Given I think - same for some others in this class

jonathanmos · 2024-06-24T12:55:45Z

...ingle-fit/trace/src/test/kotlin/com/datadog/android/trace/integration/opentracing/SpanExt.kt

- * Returns the span's traceId in hex format.
- * The [SpanContext.toTraceId] method returns a string in decimal format,
- * which doesn't match what we send in our events
+ * Returns the span's less significant trace id in hex format (the last 64 bits from the 128 bits trace id)


Suggested change

* Returns the span's less significant trace id in hex format (the last 64 bits from the 128 bits trace id)

* Returns the span's least significant trace id in hex format (the last 64 bits from the 128 bits trace id)

0xnm · 2024-06-24T12:37:51Z

...tel/src/test/kotlin/com/datadog/android/trace/opentelemetry/OtelTracerBuilderProviderTest.kt

@@ -251,7 +251,7 @@ internal class OtelTracerBuilderProviderTest {
        val traceId = span.spanContext.traceId

        // Then
-        assertThat(traceId.substring(0, 16)).isEqualTo("0000000000000000")
+        assertThat(traceId.length).isEqualTo(32)


minor proposal for a better message:

Suggested change

assertThat(traceId.length).isEqualTo(32)

assertThat(traceId).hasSize(32)

0xnm · 2024-06-24T12:43:41Z

...dd-sdk-android-trace/src/main/java/com/datadog/opentracing/propagation/DatadogHttpCodec.java

-  private static final String OT_BAGGAGE_PREFIX = "ot-baggage-";
-  private static final String TRACE_ID_KEY = "x-datadog-trace-id";
-  private static final String SPAN_ID_KEY = "x-datadog-parent-id";
-  private static final String SAMPLING_PRIORITY_KEY = "x-datadog-sampling-priority";
-  private static final String ORIGIN_KEY = "x-datadog-origin";
-
-  private DatadogHttpCodec() {
-    // This class should not be created. This also makes code coverage checks happy.
-  }
-
-  public static class Injector implements HttpCodec.Injector {
-
-    @Override
-    public void inject(final DDSpanContext context, final TextMapInject carrier) {
-      carrier.put(TRACE_ID_KEY, context.getTraceId().toString());
-      carrier.put(SPAN_ID_KEY, context.getSpanId().toString());
-      final String origin = context.getOrigin();
-      if (origin != null) {
-        carrier.put(ORIGIN_KEY, origin);
-      }
-
-      for (final Map.Entry<String, String> entry : context.baggageItems()) {
-        carrier.put(OT_BAGGAGE_PREFIX + entry.getKey(), HttpCodec.encode(entry.getValue()));
-      }
-
-      // always use max sampling priority for Android traces
-      carrier.put(SAMPLING_PRIORITY_KEY, "1");
+    public static final String OT_BAGGAGE_PREFIX = "ot-baggage-";
+    public static final String LEAST_SIGNIFICANT_TRACE_ID_KEY = "x-datadog-trace-id";
+    public static final String MOST_SIGNIFICANT_TRACE_ID_KEY = "_dd.p.tid";
+    public static final String DATADOG_TAGS_KEY = "x-datadog-tags";
+    public static final String SPAN_ID_KEY = "x-datadog-parent-id";
+    public static final String SAMPLING_PRIORITY_KEY = "x-datadog-sampling-priority";
+    public static final String ORIGIN_KEY = "x-datadog-origin";


is private -> public change necessary?

Upd: ok, probably for the test?

0xnm · 2024-06-24T12:45:07Z

...dd-sdk-android-trace/src/main/java/com/datadog/opentracing/propagation/DatadogHttpCodec.java

+            final String leastSignificantTraceId = bigIntegerUtils.leastSignificant64BitsAsDecimal(traceId);
+            final String mostSignificantTraceId = bigIntegerUtils.mostSignificant64BitsAsHex(traceId);


why to we take lower part as decimal, but upper part as hex. some compatibility thing?

as stated in the RFC

0xnm · 2024-06-24T12:46:16Z

...dd-sdk-android-trace/src/main/java/com/datadog/opentracing/propagation/DatadogHttpCodec.java

+                        tags.put(taggedHeaders.get(key), HttpCodec.decode(value));
+                    }
+                }
+                if(leastSignificant64BitsTraceIdAsDecimal==null|| mostSignificant64BitsTraceIdAsHex==null){


Suggested change

if(leastSignificant64BitsTraceIdAsDecimal==null|| mostSignificant64BitsTraceIdAsHex==null){

if (leastSignificant64BitsTraceIdAsDecimal == null || mostSignificant64BitsTraceIdAsHex == null) {

0xnm · 2024-06-24T12:47:44Z

...dd-sdk-android-trace/src/main/java/com/datadog/opentracing/propagation/DatadogHttpCodec.java

+            final String[] tagArray = tags.split(",");
+            for (String tag : tagArray) {
+                final String[] tagKeyValue = tag.split("=");
+                if (tagKeyValue.length >= 2 && tagKeyValue[0].equals(MOST_SIGNIFICANT_TRACE_ID_KEY)) {


Suggested change

if (tagKeyValue.length >= 2 && tagKeyValue[0].equals(MOST_SIGNIFICANT_TRACE_ID_KEY)) {

if (tagKeyValue.length >= 2 && MOST_SIGNIFICANT_TRACE_ID_KEY.equals(tagKeyValue[0]) {

0xnm · 2024-06-24T13:11:31Z

integrations/dd-sdk-android-okhttp-otel/transitiveDependencies

should be reverted? why this change comes?

Yes I need to see why is this happening as I did not change anything there

0xnm · 2024-06-24T13:14:49Z

...dk-android-trace/src/test/kotlin/com/datadog/opentracing/propagation/DatadogHttpCodecTest.kt

+    }
+
+    @Test
+    fun `M return null W extract { out of bounds less significant trace id }`(forge: Forge) {


Suggested change

fun `M return null W extract { out of bounds less significant trace id }`(forge: Forge) {

fun `M return null W extract { out of bounds least significant trace id }`(forge: Forge) {

0xnm · 2024-06-24T13:15:05Z

...dk-android-trace/src/test/kotlin/com/datadog/opentracing/propagation/DatadogHttpCodecTest.kt

+        val fakeOutOfBoundsLessSignficantTraceId =
+            BigInteger(forge.aStringMatching("[0-9][a-f]{$outOfBoundsSize}"), 16).toString()
+        val headers = resolveExtractedHeadersFromSpanContext(fakeDDSpanContext, forge).apply {
+            set(DatadogHttpCodec.LEAST_SIGNIFICANT_TRACE_ID_KEY, fakeOutOfBoundsLessSignficantTraceId)


Suggested change

val fakeOutOfBoundsLessSignficantTraceId =

BigInteger(forge.aStringMatching("[0-9][a-f]{$outOfBoundsSize}"), 16).toString()

val headers = resolveExtractedHeadersFromSpanContext(fakeDDSpanContext, forge).apply {

set(DatadogHttpCodec.LEAST_SIGNIFICANT_TRACE_ID_KEY, fakeOutOfBoundsLessSignficantTraceId)

val fakeOutOfBoundsLeastSignficantTraceId =

BigInteger(forge.aStringMatching("[0-9][a-f]{$outOfBoundsSize}"), 16).toString()

val headers = resolveExtractedHeadersFromSpanContext(fakeDDSpanContext, forge).apply {

set(DatadogHttpCodec.LEAST_SIGNIFICANT_TRACE_ID_KEY, fakeOutOfBoundsLeastSignficantTraceId)

0xnm · 2024-06-24T13:15:21Z

...dk-android-trace/src/test/kotlin/com/datadog/opentracing/propagation/DatadogHttpCodecTest.kt

+        @MethodSource("spanContextValues")
+        fun spanContextValues(): List<DDSpanContext> {
+            val max128BitsHexa = forge.aStringMatching("[f]{32}")
+            val minLessSignificant128BitsHexa = forge.aStringMatching("[0]{31}[1]{1}")


Suggested change

val minLessSignificant128BitsHexa = forge.aStringMatching("[0]{31}[1]{1}")

val minLeastSignificant128BitsHexa = forge.aStringMatching("[0]{31}[1]{1}")

0xnm · 2024-06-24T13:15:36Z

...t/trace/src/test/kotlin/com/datadog/android/trace/integration/otel/OtelTracerProviderTest.kt

@@ -290,22 +302,24 @@ internal class OtelTracerProviderTest {
        val testedProvider = OtelTracerProvider.Builder(stubSdkCore).build()
        val tracer = testedProvider.tracerBuilder(fakeInstrumentationName).build()
        val blockingWriterWrapper = tracer.useBlockingWriter()
-        var parentSpanTraceId: String
+        var lessSignificantParentSpanTraceId: String


Suggested change

var lessSignificantParentSpanTraceId: String

var leastSignificantParentSpanTraceId: String

still a valid suggestion

jonathanmos · 2024-06-25T08:17:28Z

...dk-android-trace/src/test/kotlin/com/datadog/opentracing/propagation/DatadogHttpCodecTest.kt

+
+    private lateinit var fakeTaggedHeaders: Map<String, String>
+
+    private lateinit var fakeIdAsHexaString: String


Suggested change

private lateinit var fakeIdAsHexaString: String

private lateinit var fakeIdAsHexString: String

jonathanmos · 2024-06-25T08:17:54Z

...android-trace/src/test/kotlin/com/datadog/android/trace/internal/utils/SpanContextExtTest.kt

+    }
+
+    @Test
+    fun `M return the expected hexa padded string W traceIdAsHexString()`() {


Suggested change

fun `M return the expected hexa padded string W traceIdAsHexString()`() {

fun `M return the expected hex padded string W traceIdAsHexString()`() {

ganeshnj · 2024-06-25T08:50:24Z

...tel/src/test/kotlin/com/datadog/android/trace/opentelemetry/OtelTracerBuilderProviderTest.kt

@@ -251,7 +251,7 @@ internal class OtelTracerBuilderProviderTest {
        val traceId = span.spanContext.traceId

        // Then
-        assertThat(traceId.substring(0, 16)).isEqualTo("0000000000000000")


/improvement

we should assert on the content of the traceId as well, not just the size.

what do you mean the content ? how would you assess that ? Do you mean regex assessment ? That will be too much IMO

that's the only way we can make sure final version is correct with a plain string assert.

ganeshnj · 2024-06-25T08:52:44Z

features/dd-sdk-android-trace/src/main/java/com/datadog/opentracing/DDTracer.java

-    // UINT64 max value
-    public static final BigInteger TRACE_ID_MAX =
+    // UINT128 max value
+    public static final BigInteger TRACE_ID_128_BITS_MAX =


/question

why do we need to subtract 1? Theoretically an id can be all bits set to 1. ie BigInteger's max value.

2^128-1 (we remove the 0). That was already in the java code I am just following the pattern.

ganeshnj · 2024-06-25T08:55:30Z

features/dd-sdk-android-trace/src/main/java/com/datadog/opentracing/DDTracer.java

@@ -592,6 +596,19 @@ private BigInteger generateNewId() {
            return value;
        }

+        private BigInteger generateNewTraceId() {


/blocking

This shouldn't be any random id but must be in chronologically sorted pattern.

Check https://github.com/DataDog/dd-sdk-ios/blob/380bdf4938519b676d5a1d06bd2ce6928637a0a6/DatadogInternal/Sources/NetworkInstrumentation/TraceID.swift#L222

TLDR

<32-bit unix seconds> <32 bits of zero> <64 random bits>

that's the old java code and we need to be consistent with the way the id is generated for spans. I am not touching that part here.

why does it need to be chronologically sorted ? Is there a blocking reason behind it ?

Ok I see why, we will switch to the new CoreTracer way of generating ids.

ganeshnj · 2024-06-25T09:06:27Z

...ttp/src/test/kotlin/com/datadog/android/okhttp/trace/TracingInterceptorNotSendingSpanTest.kt

            assertThat(lastValue.header(TracingInterceptor.DATADOG_SPAN_ID_HEADER)).isNull()
            assertThat(lastValue.header(TracingInterceptor.DATADOG_ORIGIN_HEADER)).isNull()
+            assertThat(lastValue.header(TracingInterceptor.DATADOG_TAGS)).isNull()


/improvement

could you also assert on the tag value? this should be higher order part.

not sure if it is possible or is not going to be too much. Need to check.

ganeshnj · 2024-06-25T09:11:15Z

...fit/trace/src/test/kotlin/com/datadog/android/trace/integration/opentracing/UtilitiesTest.kt

@@ -228,7 +233,7 @@ class UtilitiesTest {
        assertThat(eventsWritten).hasSize(1)
        val event0 = JsonParser.parseString(eventsWritten[0].eventData) as JsonObject
        assertThat(event0.getString("env")).isEqualTo(stubSdkCore.getDatadogContext().env)
-        assertThat(event0.getString("spans[0].trace_id")).isEqualTo(traceId.toHexString())
+        assertThat(event0.getString("spans[0].trace_id")).isEqualTo(leastSignificantTraceId)


/improvement

could you also assert on meta._dd.p.tid?

you mean meta._dd.p.id I think ? this property always confuses me as it is _dd.p.id into the distributed traces headers.

ganeshnj

Overall looks good, could you do some manual testing

Distributed traces with all context headers are 128 bits
Spans are now shown with 128 bit ids in datadog web app
Logs correlations are now using 128 bit
RUM events now show 128 bit ids

ganeshnj · 2024-06-27T11:05:10Z

features/dd-sdk-android-trace/src/test/kotlin/com/datadog/trace/api/IdGenerationStrategyTest.kt

+                assertThat(traceId).isNotNull
+                assertThat(traceId).isNotEqualTo("foo")
+                assertThat(traceId).isNotEqualTo(DDTraceId.ZERO)
+                assertThat(traceId).isEqualTo(traceId)


/blocking

this is always going to be true.

Assuming Sequential here is what we default do. Could you make sure we assert on the right things here

Something like https://github.com/DataDog/dd-sdk-ios/blob/725e59b118935100c36fe90792c3cb2a26438fe2/DatadogInternal/Tests/NetworkInstrumentation/TraceIDGeneratorTests.swift#L22-L36

yes I ported this test from the APM code, that's weird indeed let me check what they have in original groovy test. I might have missed something

I checked, all good that's the test, I think the reason why is to test the reflexivity of equals method: "an object must equal itself"

mariusc83 · 2024-06-27T11:36:58Z

Overall looks good, could you do some manual testing

Distributed traces with all context headers are 128 bits

Spans are not shown with 128 bit ids in datadog web app

Logs correlations are now using 128 bit

RUM events now show 128 bit ids

I basically performed all these tests + many other but will double check it now.

0xnm

lgtm, I reviewed it briefly (because I believe that @ganeshnj already reviewed the functional part). There are still some opened comments/suggestions.

0xnm · 2024-06-28T09:20:49Z

...trace/src/test/kotlin/com/datadog/android/trace/internal/domain/event/BigIntegerUtilsTest.kt

+    }
+
+    @Test
+    fun `M extract unsigned less and most significant bits W extract LSB and MSB`() {


Suggested change

fun `M extract unsigned less and most significant bits W extract LSB and MSB`() {

fun `M extract unsigned least and most significant bits W extract LSB and MSB`() {

0xnm · 2024-06-28T09:23:53Z

...trace/src/test/kotlin/com/datadog/android/trace/internal/domain/event/BigIntegerUtilsTest.kt

+    companion object {
+        private const val MAX_UNSIGNED_LONG_HEXA_STRING = "ffffffffffffffff"
+        private const val MIN_UNSIGNED_LONG_HEXA_STRING = "0000000000000000"
+        private val MAX_EXEC_TIME_IN_NANOS = TimeUnit.MILLISECONDS.toNanos(8)


it is not really relevant, because these are the unit tests, which are executed on JVM, on a server CPU, etc. The execution number you get here is be quite different from the one on the real device.

0xnm · 2024-06-28T09:26:11Z

...d/integration/src/androidTest/kotlin/com/datadog/android/sdk/integration/trace/TracesTest.kt

@@ -68,8 +68,12 @@ internal abstract class TracesTest {
        assertThat(expectedSpans.size).isEqualTo(sentSpansObjects.size)
        expectedSpans.forEach { span ->
            val json = sentSpansObjects.first {
-                it.get(TRACE_ID_KEY).asString == Long.toHexString((span.traceId.toLong())) &&
-                    it.get(SPAN_ID_KEY).asString == Long.toHexString((span.spanId.toLong()))
+                val leasSignificantTraceId = it.get(TRACE_ID_KEY).asString


this suggestion is still valid

0xnm · 2024-06-28T09:26:22Z

...d/integration/src/androidTest/kotlin/com/datadog/android/sdk/integration/trace/TracesTest.kt

+                val leasSignificantTraceId = it.get(TRACE_ID_KEY).asString
+                val mostSignificantTraceId = it.getAsJsonObject("meta")
+                    .getAsJsonPrimitive(MOST_SIGNIFICANT_64_BITS_TRACE_ID_KEY).asString
+                leasSignificantTraceId == span.leastSignificant64BitsTraceId() &&


this suggestion is still valid

0xnm · 2024-06-28T09:27:55Z

...ingle-fit/trace/src/test/kotlin/com/datadog/android/trace/integration/opentracing/SpanExt.kt

-fun Span.traceIdAsHexString(): String {
-    return context().toTraceId().toLong().toHexString()
+fun Span.leastSignificant64BitsTraceId(): String {
+    return (this as? DDSpan)?.traceId?.toString(16)?.padStart(32, '0')?.takeLast(16) ?: ""


will we be aware about the cases when we get empty string? nothing will crash?

based on the code we have there is no way we could get empty strings here as the generator is tested above and other tests will fail first. Here the debate is either we add these logs just for the sake of it or we just don't and consider this safe.

also we will be aware as I added this extra checks:

hasValidMostSignificant64BitsTraceId() hasValidLeastSignificant64BitsTraceId()

They validate the regEx

0xnm · 2024-06-28T09:29:00Z

...t/trace/src/test/kotlin/com/datadog/android/trace/integration/otel/OtelTracerProviderTest.kt

@@ -290,22 +302,24 @@ internal class OtelTracerProviderTest {
        val testedProvider = OtelTracerProvider.Builder(stubSdkCore).build()
        val tracer = testedProvider.tracerBuilder(fakeInstrumentationName).build()
        val blockingWriterWrapper = tracer.useBlockingWriter()
-        var parentSpanTraceId: String
+        var lessSignificantParentSpanTraceId: String


still a valid suggestion

…reTracer

mariusc83 self-assigned this Jun 19, 2024

0xnm reviewed Jun 19, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-1829/use-128-bit-trace-ids branch 6 times, most recently from 8a2f82e to 28c1051 Compare June 24, 2024 11:26

mariusc83 force-pushed the mconstantin/rum-1829/use-128-bit-trace-ids branch from 28c1051 to c385c73 Compare June 24, 2024 12:18

mariusc83 marked this pull request as ready for review June 24, 2024 12:18

mariusc83 requested review from a team as code owners June 24, 2024 12:18

mariusc83 requested a review from 0xnm June 24, 2024 12:41

jonathanmos reviewed Jun 24, 2024

View reviewed changes

0xnm requested changes Jun 24, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-1829/use-128-bit-trace-ids branch from c385c73 to 2a5b4e4 Compare June 25, 2024 07:50

mariusc83 requested review from jonathanmos and 0xnm June 25, 2024 07:57

mariusc83 force-pushed the mconstantin/rum-1829/use-128-bit-trace-ids branch 2 times, most recently from 6cc1e75 to 60ee7be Compare June 25, 2024 08:00

jonathanmos approved these changes Jun 25, 2024

View reviewed changes

ganeshnj requested changes Jun 25, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-1829/use-128-bit-trace-ids branch 2 times, most recently from 4a9da7e to 975a0e7 Compare June 25, 2024 14:35

mariusc83 requested a review from ganeshnj June 25, 2024 14:36

mariusc83 force-pushed the mconstantin/rum-1829/use-128-bit-trace-ids branch from 975a0e7 to f15c224 Compare June 26, 2024 07:05

ganeshnj reviewed Jun 27, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-1829/use-128-bit-trace-ids branch from f15c224 to ce9e1f3 Compare June 27, 2024 11:46

mariusc83 force-pushed the mconstantin/rum-1829/use-128-bit-trace-ids branch 2 times, most recently from 256c37e to 8b3c6ab Compare June 27, 2024 12:41

mariusc83 requested a review from ganeshnj June 27, 2024 17:52

ganeshnj approved these changes Jun 28, 2024

View reviewed changes

0xnm approved these changes Jun 28, 2024

View reviewed changes

mariusc83 added 4 commits June 28, 2024 11:44

Add 128 bits trace id support for OpenTelemetry API

1b551d2

Add 128 bits trace id support for OpenTracing API

c09c08c

Add the 128 bits trace id support for the distributed traces

eb6f19b

Generate the traceId in the DdTracer using the new strategy in the Co…

446a7b4

…reTracer

mariusc83 force-pushed the mconstantin/rum-1829/use-128-bit-trace-ids branch from 8b3c6ab to 446a7b4 Compare June 28, 2024 09:44

mariusc83 merged commit 6772159 into develop Jun 28, 2024
19 checks passed

mariusc83 deleted the mconstantin/rum-1829/use-128-bit-trace-ids branch June 28, 2024 12:33

xgouchet added this to the 2.12.x milestone Jul 31, 2024

		@@ -50,6 +51,9 @@
		public class DDTracer implements io.opentracing.Tracer, Closeable, Tracer {
		// UINT64 max value

	String lessSignificantTraceId = "0";
	String leastSignificantTraceId = "0";

		\|\| parsedValue.compareTo(DDTracer.TRACE_ID_MAX) > 0) {
		\|\| parsedValue.compareTo(DDTracer.SPAN_ID_MAX) > 0) {

	fun lessSignificantUnsignedLongAsHexa(traceId: BigInteger): String {
	fun leastSignificantUnsignedLongAsHex(traceId: BigInteger): String {

	fun hasLessSignificantTraceId(traceId: String): SpanEventAssert {
	fun hasLeastSignificantTraceId(traceId: String): SpanEventAssert {

	"Expected SpanEvent to have less significant traceId: $traceId" +
	"Expected SpanEvent to have least significant traceId: $traceId" +

	val lessSignificantTraceId = actual.meta.additionalProperties[TRACE_ID_META_KEY]
	val mostSignificantTraceId = actual.meta.additionalProperties[TRACE_ID_META_KEY]

		@@ -2,13 +2,11 @@ Dependencies List

		com.squareup.okhttp3:okhttp:4.11.0 : 768 Kb

	* Returns the span's less significant trace id in hex format (the last 64 bits from the 128 bits trace id)
	* Returns the span's least significant trace id in hex format (the last 64 bits from the 128 bits trace id)

	assertThat(traceId.length).isEqualTo(32)
	assertThat(traceId).hasSize(32)

		final String leastSignificantTraceId = bigIntegerUtils.leastSignificant64BitsAsDecimal(traceId);
		final String mostSignificantTraceId = bigIntegerUtils.mostSignificant64BitsAsHex(traceId);

	if(leastSignificant64BitsTraceIdAsDecimal==null\|\| mostSignificant64BitsTraceIdAsHex==null){
	if (leastSignificant64BitsTraceIdAsDecimal == null \|\| mostSignificant64BitsTraceIdAsHex == null) {

	if (tagKeyValue.length >= 2 && tagKeyValue[0].equals(MOST_SIGNIFICANT_TRACE_ID_KEY)) {
	if (tagKeyValue.length >= 2 && MOST_SIGNIFICANT_TRACE_ID_KEY.equals(tagKeyValue[0]) {

	fun `M return null W extract { out of bounds less significant trace id }`(forge: Forge) {
	fun `M return null W extract { out of bounds least significant trace id }`(forge: Forge) {

	val minLessSignificant128BitsHexa = forge.aStringMatching("[0]{31}[1]{1}")
	val minLeastSignificant128BitsHexa = forge.aStringMatching("[0]{31}[1]{1}")

	var lessSignificantParentSpanTraceId: String
	var leastSignificantParentSpanTraceId: String


		private lateinit var fakeTaggedHeaders: Map<String, String>

		private lateinit var fakeIdAsHexaString: String

	private lateinit var fakeIdAsHexaString: String
	private lateinit var fakeIdAsHexString: String

	fun `M return the expected hexa padded string W traceIdAsHexString()`() {
	fun `M return the expected hex padded string W traceIdAsHexString()`() {

	fun `M extract unsigned less and most significant bits W extract LSB and MSB`() {
	fun `M extract unsigned least and most significant bits W extract LSB and MSB`() {

RUM-1829 Provide 128 bits support for the trace ids in the Tracing sdk #2089

RUM-1829 Provide 128 bits support for the trace ids in the Tracing sdk #2089

Conversation

mariusc83 commented Jun 19, 2024 • edited Loading

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

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

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 Jun 24, 2024 • edited Loading

Codecov Report

jonathanmos left a comment • 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

Choose a reason for hiding this comment

0xnm Jun 24, 2024 • 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

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

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

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

ganeshnj left a comment • 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

mariusc83 commented Jun 27, 2024

0xnm 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusc83 commented Jun 19, 2024 •

edited

Loading

codecov-commenter commented Jun 24, 2024 •

edited

Loading

jonathanmos left a comment •

edited

Loading

0xnm Jun 24, 2024 •

edited

Loading

ganeshnj left a comment •

edited

Loading