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

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)

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

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

Codecov Report

Merging #1993 (f28a8cf) into feature/otel-support (37e2e35) will increase coverage by 0.04%.
The diff coverage is 86.11%.

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

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

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

+    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

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

@@ -35,6 +36,8 @@ internal class CoreDDSpanForgeryFactory : ForgeryFactory<DDSpan> {
            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

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

    // endregion

    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

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

-        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

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

@@ -90,7 +104,10 @@ internal class OtelDdSpanToSpanEventMapper(
            view = event.tags[LogAttributes.RUM_VIEW_ID]?.let { SpanEvent.View(it as? String) }
        )
        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

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

+    }
+
+    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

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

+            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

detekt_custom.yml

@@ -1120,6 +1120,7 @@ datadog:
      - "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

...c/main/kotlin/com/datadog/android/trace/internal/domain/event/OtelDdSpanToSpanEventMapper.kt Outdated Show resolved Hide resolved

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
25 checks passed

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

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

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

mariusc83 commented Apr 17, 2024

codecov-commenter commented Apr 17, 2024 •

edited

Loading

0xnm Apr 17, 2024

mariusc83 Apr 17, 2024

0xnm Apr 17, 2024

0xnm Apr 17, 2024

0xnm Apr 17, 2024

0xnm Apr 17, 2024

0xnm Apr 17, 2024

0xnm Apr 17, 2024

0xnm Apr 18, 2024

	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()"

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

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

Conversation

mariusc83 commented Apr 17, 2024

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

codecov-commenter commented Apr 17, 2024 • edited Loading

Codecov Report

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 Apr 17, 2024 •

edited

Loading