RUM-4116 Add the SpanLink support for Otel API implementation by mariusc83 · Pull Request #1993 · DataDog/dd-sdk-android

mariusc83 · 2024-04-17T09:59:35Z

What does this PR do?

The Otel SpanBuilder#addLink API delegates to the DDSpan under the hood and adds an AgentSpanLink to it.
In this PR we are adding the links support into our OtelDdSpanToSpanEventMapper by serializing the AgentSpanLink list and adding it as a String/String entry into the SpanEvent.meta map.
Example of a serialized span link into the meta section:

_dd.span_links -> "[
    {
        "attributes": {
            "key": "value"
        },
        "flags": 1,
        "span_id": "4ab9b106d00b9366",
        "trace_id": "000000000000000015b5cedc8e96fd32"
    }
]"

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)

codecov-commenter · 2024-04-17T12:03:07Z

Codecov Report

Attention: Patch coverage is 86.11111% with 5 lines in your changes missing coverage. Please review.

Project coverage is 60.29%. Comparing base (37e2e35) to head (f28a8cf).

Files with missing lines	Patch %	Lines
...ternal/domain/event/OtelDdSpanToSpanEventMapper.kt	88.57%	0 Missing and 4 partials ⚠️
...e/src/main/java/com/datadog/trace/core/DDSpan.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                   Coverage Diff                    @@
##           feature/otel-support    #1993      +/-   ##
========================================================
+ Coverage                 60.26%   60.29%   +0.04%     
========================================================
  Files                       808      808              
  Lines                     30184    30196      +12     
  Branches                   4945     4950       +5     
========================================================
+ Hits                      18188    18206      +18     
- Misses                    10772    10775       +3     
+ Partials                   1224     1215       -9

Files with missing lines	Coverage Δ
...dog/android/trace/internal/data/OtelTraceWriter.kt	`88.89% <ø> (+3.70%)`	⬆️
...e/src/main/java/com/datadog/trace/core/DDSpan.java	`55.98% <0.00%> (-0.24%)`	⬇️
...ternal/domain/event/OtelDdSpanToSpanEventMapper.kt	`88.42% <88.57%> (+2.06%)`	⬆️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

0xnm · 2024-04-17T14:00:57Z

+    private fun Forge.exhaustiveAttributes(): Map<String, String> {
+        return listOf(
+            aString()
+        ).map { anAlphabeticalString() to it }
+            .toMap(mutableMapOf())
+    }


no need to redefine it, you should be able to import one defined in tools:unit module (com.datadog.tools.unit.forge package). Anyway, in this particular case it is not exhaustive attributes, because it is always String to String

yes that is the reason why I did not import it but you are right I need to change that name

0xnm · 2024-04-17T14:02:08Z

            whenever(it.baggageItems).thenReturn(baggageItems)
            whenever(it.tags).thenReturn(tagsAndMetrics)
        }
+        val spanLinks = forge.aList(size = forge.anInt(min = 0, max = 10)) { forge.getForgery<AgentSpanLink>() }


Suggested change

val spanLinks = forge.aList(size = forge.anInt(min = 0, max = 10)) { forge.getForgery<AgentSpanLink>() }

val spanLinks = forge.aList(size = forge.anInt(min = 0, max = 10)) { getForgery<AgentSpanLink>() }

0xnm · 2024-04-17T14:20:34Z


    companion object {
-        internal const val RUM_ENDPOINT_REQUIRED_TRACE_ID_LENGTH = 16
+        private const val LESS_SIGNIFICANT_64_BITS_AS_HEXA_LENGTH = 16


We probably need to use word hex instead of hexa. The only usages of hexa I find is for color (which makes sense because it is RGB as hex + alpha as hex). Same applies for other mentions of hexa in this PR.

Also it should be least, not less for LSB (least significant bits).

Suggested change

private const val LESS_SIGNIFICANT_64_BITS_AS_HEXA_LENGTH = 16

private const val LEAST_SIGNIFICANT_64_BITS_AS_HEX_LENGTH = 16

0xnm · 2024-04-17T14:22:03Z

-        return model.traceId.toHexString().takeLast(RUM_ENDPOINT_REQUIRED_TRACE_ID_LENGTH)
+        // because the backend endpoint does not support 32 hexa characters trace ids we are going to take only the
+        // less significant 64 bits of the trace id. Later on when we will introduce the 128 bits support
+        // the more significant bits will be reported in a separated dedicated meta tag.


Suggested change

// the more significant bits will be reported in a separated dedicated meta tag.

// the most significant bits will be reported in a separated dedicated meta tag.

0xnm · 2024-04-17T14:23:09Z

        )
        val tags = event.tags.mapValues { it.value.toString() }
-        val meta = (event.baggage + tags).toMutableMap()
+        val meta: MutableMap<String, String> = mutableMapOf()


minor suggestion

Suggested change

val meta: MutableMap<String, String> = mutableMapOf()

val meta = mutableMapOf<String, String>()

0xnm · 2024-04-17T14:23:58Z

+    }
+
+    private fun resolveSpanLink(link: AgentSpanLink): JsonObject {
+        // The SpanLinks support the full 128 bits trace so we can use the full hexa string


Suggested change

// The SpanLinks support the full 128 bits trace so we can use the full hexa string

// The SpanLinks support the full 128 bits trace so we can use the full hex string

0xnm · 2024-04-17T15:00:52Z

+            val actualLink = deserializedLinks[index].asJsonObject
+            val actualTraceId = actualLink.get("trace_id").asString
+            val traceId = link.traceId().toHexString()
+            assertThat(actualTraceId)
+                .overridingErrorMessage(
+                    "Expected SpanEvent to have span link trace id: " +
+                        "$traceId but " +
+                        "instead was: $actualTraceId"
+                )
+                .isEqualTo(traceId)
+            val actualSpanId = actualLink.get("span_id").asString
+            val spanId = DDSpanId.toHexStringPadded(link.spanId())
+            assertThat(actualSpanId)
+                .overridingErrorMessage(
+                    "Expected SpanEvent to have span link span id: " +
+                        "$spanId but " +
+                        "instead was: $$actualSpanId"
+                )
+                .isEqualTo(spanId)
+            val traceFlags = link.traceFlags()
+            if (traceFlags.toInt() != 0) {
+                val actualTraceFlags = actualLink.get("flags").asByte
+                assertThat(actualTraceFlags)
+                    .overridingErrorMessage(
+                        "Expected SpanEvent to have span link flags: " +
+                            "$traceFlags but " +
+                            "instead was: $actualTraceFlags"
+                    )
+                    .isEqualTo(traceFlags)
+            } else {
+                assertThat(actualLink.has("flags"))
+                    .overridingErrorMessage(
+                        "Expected SpanEvent to not have span link flags but " +
+                            "instead was: $actualLink"
+                    )
+                    .isFalse()
+            }
+            val actualTraceState = actualLink.get("tracestate").asString
+            if (actualTraceState.isNotEmpty()) {
+                val traceState = link.traceState()
+                assertThat(actualTraceState)
+                    .overridingErrorMessage(
+                        "Expected SpanEvent to have span link trace state: " +
+                            "$traceState but " +
+                            "instead was: $actualTraceState"
+                    )
+                    .isEqualTo(traceState)
+            } else {
+                assertThat(actualLink.has("tracestate"))
+                    .overridingErrorMessage(
+                        "Expected SpanEvent to not have span link trace state but " +
+                            "instead was: $actualLink"
+                    )
+                    .isFalse()
+            }
+            val actualAttributes = actualLink.get("attributes").toString()
+            val expectedAttributes = link.attributes().asMap()
+            val jsonObject = JsonObject()
+            expectedAttributes.forEach { (key, value) ->
+                jsonObject.addProperty(key, value)
+            }
+            val expectedAttributesJsonString = jsonObject.toString()
+            assertThat(actualAttributes)
+                .overridingErrorMessage(
+                    "Expected SpanEvent to have span link attributes: " +
+                        "$expectedAttributesJsonString but " +
+                        "instead was: $actualAttributes"
+                )
+                .isEqualTo(expectedAttributesJsonString)


not blocking: maybe extract this into a dedicated assert / split assertions? a bit hard to read

0xnm · 2024-04-18T12:18:40Z

      - "kotlin.String.substringAfter(kotlin.Char, kotlin.String)"
      - "kotlin.String.substringAfterLast(kotlin.Char, kotlin.String)"
      - "kotlin.String.substringBefore(kotlin.Char, kotlin.String)"
+      - "kotlin.String.takeLessSignificant64Bits()"


Suggested change

- "kotlin.String.takeLessSignificant64Bits()"

- "kotlin.String.takeLeastSignificant64Bits()"

mariusc83 force-pushed the mconstantin/rum-4116/add-spanlink-support-for-otel-tracer branch from 799a676 to 89ec036 Compare April 17, 2024 11:34

mariusc83 marked this pull request as ready for review April 17, 2024 11:34

mariusc83 requested review from a team as code owners April 17, 2024 11:34

mariusc83 commented Apr 17, 2024

View reviewed changes

Comment thread ...c/main/kotlin/com/datadog/android/trace/internal/domain/event/OtelDdSpanToSpanEventMapper.kt Outdated

mariusc83 force-pushed the mconstantin/rum-4116/add-spanlink-support-for-otel-tracer branch from 89ec036 to fec3fba Compare April 17, 2024 11:42

mariusc83 self-assigned this Apr 17, 2024

xgouchet previously approved these changes Apr 17, 2024

View reviewed changes

0xnm reviewed Apr 17, 2024

View reviewed changes

mariusc83 dismissed xgouchet’s stale review via 94ba118 April 17, 2024 15:44

mariusc83 force-pushed the mconstantin/rum-4116/add-spanlink-support-for-otel-tracer branch from fec3fba to 94ba118 Compare April 17, 2024 15:44

mariusc83 requested review from 0xnm and xgouchet April 17, 2024 15:44

mariusc83 force-pushed the mconstantin/rum-4116/add-spanlink-support-for-otel-tracer branch 2 times, most recently from cd0841b to 40ec44a Compare April 17, 2024 15:46

jonathanmos previously approved these changes Apr 18, 2024

View reviewed changes

0xnm previously approved these changes Apr 18, 2024

View reviewed changes

RUM-4116 Add the SpanLink support for Otel API implementation

f28a8cf

mariusc83 dismissed stale reviews from 0xnm and jonathanmos via f28a8cf April 18, 2024 12:26

mariusc83 force-pushed the mconstantin/rum-4116/add-spanlink-support-for-otel-tracer branch from 40ec44a to f28a8cf Compare April 18, 2024 12:26

0xnm approved these changes Apr 18, 2024

View reviewed changes

mariusc83 merged commit c88c271 into feature/otel-support Apr 18, 2024

mariusc83 deleted the mconstantin/rum-4116/add-spanlink-support-for-otel-tracer branch April 18, 2024 13:18

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

RUM-4116 Add the SpanLink support for Otel API implementation#1993

RUM-4116 Add the SpanLink support for Otel API implementation#1993
mariusc83 merged 1 commit into
feature/otel-supportfrom
mconstantin/rum-4116/add-spanlink-support-for-otel-tracer

mariusc83 commented Apr 17, 2024

Uh oh!

Uh oh!

codecov-commenter commented Apr 17, 2024 •

edited

Loading

Uh oh!

0xnm Apr 17, 2024

Uh oh!

mariusc83 Apr 17, 2024

Uh oh!

0xnm Apr 17, 2024

Uh oh!

0xnm Apr 17, 2024

Uh oh!

0xnm Apr 17, 2024

Uh oh!

0xnm Apr 17, 2024

Uh oh!

0xnm Apr 17, 2024

Uh oh!

0xnm Apr 17, 2024

Uh oh!

0xnm Apr 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	val spanLinks = forge.aList(size = forge.anInt(min = 0, max = 10)) { forge.getForgery<AgentSpanLink>() }
	val spanLinks = forge.aList(size = forge.anInt(min = 0, max = 10)) { getForgery<AgentSpanLink>() }

	private const val LESS_SIGNIFICANT_64_BITS_AS_HEXA_LENGTH = 16
	private const val LEAST_SIGNIFICANT_64_BITS_AS_HEX_LENGTH = 16

	// the more significant bits will be reported in a separated dedicated meta tag.
	// the most significant bits will be reported in a separated dedicated meta tag.

	val meta: MutableMap<String, String> = mutableMapOf()
	val meta = mutableMapOf<String, String>()

	// The SpanLinks support the full 128 bits trace so we can use the full hexa string
	// The SpanLinks support the full 128 bits trace so we can use the full hex string

	- "kotlin.String.takeLessSignificant64Bits()"
	- "kotlin.String.takeLeastSignificant64Bits()"

Uh oh!

Conversation

mariusc83 commented Apr 17, 2024

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

Uh oh!

codecov-commenter commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Apr 17, 2024 •

edited

Loading