RUM-1836 feat(otel-tracer): conform to otel Tracer, SpanBuilder and Span #1611

ganeshnj · 2024-01-02T13:32:38Z

Things that have fatal error - will be implemented in future PRs

What and why?

Otel tracer implementation.

How?

This PR conforms to Tracer, SpanBuilder and Span protocols from otel.

We are not trying to reinventing the wheel but rather use the existing implementation and make it complaint to otel. During the otel API usage, we keep a internal copy of the span which is a datadog span. When a span is closed, all the otel data is mapped to the datadog span and reported.

Things that are supported

Make the public API to get a tracer return both OTTracer and Otel Tracer.
start and end span APIs
Parent specific APIs - span parent
Attribute APIs - akin to tags in datadog
name API - akin to operation name in datadog

Review checklist

Feature or bugfix MUST have appropriate tests (unit, integration)
Make sure each commit and the PR mention the Issue number or JIRA reference
Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

Run unit tests for Core, RUM, Trace, Logs, CR and WVT
Run unit tests for Session Replay
Run integration tests
Run smoke tests
Run tests for tools/

datadog-datadog-prod-us1 · 2024-01-03T13:51:59Z

Datadog Report

Branch report: ganeshnj/feat/otel-tracer-conform
Commit report: 2eab9ac
Test service: dd-sdk-ios

✅ 0 Failed, 2727 Passed, 0 Skipped, 12m 27.49s Wall Time
⬇️ Test Sessions change in coverage: 7 decreased, 3 increased, 4 no change

⬇️ Code Coverage Decreases vs Default Branch (7)

This report shows up to 5 code coverage decreases.

test DatadogTraceTests iOS (-6.88%) - Details
test DatadogTraceTests tvOS (-6.78%) - Details
test DatadogCoreTests tvOS (-4.17%) - Details
test DatadogCoreTests iOS (-3.32%) - Details
test DatadogCrashReportingTests tvOS (-0.02%) - Details
test DatadogCrashReportingTests iOS (-0.02%) - Details
test DatadogLogsTests iOS (-0.01%) - Details

ganeshnj · 2024-01-03T09:45:12Z

DatadogTrace/Sources/DDNoOps.swift


 internal struct DDNoopGlobals {
    static let tracer = DDNoopTracer()
    static let span = DDNoopSpan()
    static let context = DDNoopSpanContext()
 }

-internal struct DDNoopTracer: OTTracer {
+internal class DDNoopTracer: OTTracer, OpenTelemetryApi.Tracer {


OpenTelemetryApi.Tracer is class only protocol.

…Builder and Span

…tracer

maciejburda

Good stuff and decent coverage! 🙇

Left some minor comments :)

maciejburda · 2024-01-04T10:53:00Z

DatadogTrace/Sources/OpenTelemetry/OTelNoOpSpan.swift

+    func addEvent(name: String, attributes: [String: OpenTelemetryApi.AttributeValue], timestamp: Date) {}
+
+    func end() {
+        OpenTelemetry.instance.contextProvider.removeContextForSpan(self)


just curious: is there a use case for noop to have this implementation?

probably never, unless someone adds the span manually to the context.

There is an API for it OpenTelemetry.instance.contextProvider.setActiveSpan(span)

maciejburda · 2024-01-04T10:56:15Z

DatadogTrace/Sources/OpenTelemetry/OTelSpan.swift

+    var description: String {
+        return "OTelSpan"
+    }


Maybe we can dump a little bit more data. Will be useful when debugging with print

that's debugDescription

https://developer.apple.com/documentation/swift/customdebugstringconvertible

I can put a separate PR for a pretty print.

maciejburda · 2024-01-04T10:58:49Z

DatadogTrace/Sources/OpenTelemetry/OTelSpanId+Datadog.swift

+extension OpenTelemetryApi.SpanId {
+    /// Converts OpenTelemetry `SpanId` to Datadog `SpanID`.
+    /// - Returns: Datadog `SpanID`.
+    func toDatadog() -> SpanID {


Having Id and ID is a little bit awkward :/

Is there any way we could easily normalise class naming to one fashion?

that's otel (SpanId) vs datadog (SpanID), given these are public types - we don't want to change them.

DatadogTrace/Tests/OpenTelemetry/OTelSpanTests.swift

ncreated

Nice to see that it fits the existing DatadogTracer 👌. I left few questions and pointed to some blockers on thread safety.

DatadogTrace/Sources/DatadogTracer.swift

DatadogTrace/Sources/OpenTelemetry/OTelNoOpSpan.swift

DatadogTrace/Sources/OpenTelemetry/OTelSpan.swift

ncreated · 2024-01-05T09:25:24Z

DatadogTrace/Sources/OpenTelemetry/OTelSpanBuilder.swift

+    var tracer: DatadogTracer
+    var spanName: String
+    var spanKind = SpanKind.client
+    var attributes: [String: OpenTelemetryApi.AttributeValue]
+    var startTime: Date?
+    var active: Bool
+    var parent: Parent


blocking/ It's all a mutable state modified by user and it lacks synchronisation. @ReadWriteLock might do the job.

I would argue that this needs to thread safe given the purpose of the builder. When a customer is building the span using the SpanBuilder it must be all done from the same thread. Once they start the span, it can be move around different threads.

On the same hypothesis https://opentelemetry.io/docs/specs/otel/trace/api/#concurrency

DatadogTrace/Sources/OpenTelemetry/OTelSpan.swift

ncreated · 2024-01-05T09:32:52Z

DatadogTrace/Sources/OpenTelemetry/OTelSpanBuilder.swift

+    func startSpan() -> OpenTelemetryApi.Span {
+        let parentContext = parent.context()
+        let traceId: TraceId
+        let spanId = SpanId.random()


suggestion/ Abstracting trace IDs generation to separate type might be handy in some unit tests.

this is not required at this point - so going to keep as it is. We can refactor when needed.

DatadogTrace/Tests/OpenTelemetry/OTelSpanTests.swift

DatadogTrace/Sources/OpenTelemetry/OTelSpan.swift

ncreated

Looks good 👍, I left only one question.

Please be aware of the impact that #1615 may have on this work.

DatadogTrace/Sources/OpenTelemetry/OTelSpan.swift

ganeshnj · 2024-01-05T16:19:15Z

Looks good 👍, I left only one question.

Please be aware of the impact that #1615 may have on this work.

Yes, I'm aware of that. Had a glance on your PR. Thanks for headsup.

ganeshnj changed the title ~~RUM-1836 feat(otel-tracer): scaffold conformance to otel Tracer, SpanBuilder and Span~~ RUM-1836 feat(otel-tracer): conform to otel Tracer, SpanBuilder and Span Jan 2, 2024

ganeshnj force-pushed the ganeshnj/feat/otel-tracer-conform branch from 7baf47a to deb6977 Compare January 2, 2024 13:47

Base automatically changed from ganeshnj/feat/otel-tracer-spm-dep to ganeshnj/feat/otel-tracer January 2, 2024 17:10

ganeshnj force-pushed the ganeshnj/feat/otel-tracer-conform branch from deb6977 to 6e552e1 Compare January 3, 2024 09:42

ganeshnj changed the base branch from ganeshnj/feat/otel-tracer to ganeshnj/feat/otel-tracer-project-dependency January 3, 2024 13:34

ganeshnj force-pushed the ganeshnj/feat/otel-tracer-conform branch from b8700b0 to 95d5382 Compare January 3, 2024 13:40

ganeshnj commented Jan 3, 2024

View reviewed changes

ganeshnj marked this pull request as ready for review January 3, 2024 16:00

ganeshnj requested review from a team as code owners January 3, 2024 16:00

Base automatically changed from ganeshnj/feat/otel-tracer-project-dependency to ganeshnj/feat/otel-tracer January 3, 2024 16:29

ganeshnj added 7 commits January 3, 2024 17:30

RUM-1836 feat(otel-tracer): scaffold conformance to otel Tracer, Span…

4b61a0a

…Builder and Span

RUM-1836 feat(otel-tracer): implement core functionality of the otel …

27ecb2a

…tracer

RUM-1836 feat(otel-tracer): add test cases

c9143b0

RUM-1836 feat(otel-tracer): fix linter

bf55a6c

RUM-1836 feat(otel-tracer): add tests

156454d

RUM-1836 feat(otel-tracer): add source files to project

3a8b2ca

RUM-1836 feat(otel-tracer): casing - now all OTel

84a7f78

ganeshnj force-pushed the ganeshnj/feat/otel-tracer-conform branch from 8f37dd0 to 84a7f78 Compare January 3, 2024 16:30

ganeshnj marked this pull request as draft January 4, 2024 08:18

ganeshnj added 3 commits January 4, 2024 09:41

RUM-1836 feat(otel-tracer): remove status API for now, will add later

f03cd63

RUM-1836 feat(otel-tracer): handle attribute cases one by one

dec5b26

RUM-1836 feat(otel-tracer): remove unused variables

c045052

ganeshnj marked this pull request as ready for review January 4, 2024 09:42

maciejburda approved these changes Jan 4, 2024

View reviewed changes

ncreated requested changes Jan 5, 2024

View reviewed changes

RUM-1836 feat(otel-tracer): PR feedback

a53814e

ganeshnj requested a review from ncreated January 5, 2024 12:26

RUM-1836 feat(otel-tracer): corresponding reference change of rename

140c95b

ncreated approved these changes Jan 5, 2024

View reviewed changes

DatadogTrace/Sources/OpenTelemetry/OTelSpan.swift Outdated Show resolved Hide resolved

RUM-1836 feat(otel-tracer): remove semaphore usage

2eab9ac

ganeshnj merged commit 43ac91a into ganeshnj/feat/otel-tracer Jan 5, 2024
8 checks passed

ganeshnj deleted the ganeshnj/feat/otel-tracer-conform branch January 5, 2024 16:50

ncreated mentioned this pull request May 31, 2024

Release 2.12.0 #1871

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUM-1836 feat(otel-tracer): conform to otel Tracer, SpanBuilder and Span #1611

RUM-1836 feat(otel-tracer): conform to otel Tracer, SpanBuilder and Span #1611

ganeshnj commented Jan 2, 2024 •

edited

Loading

datadog-datadog-prod-us1 bot commented Jan 3, 2024 •

edited

Loading

ganeshnj Jan 3, 2024

maciejburda left a comment

maciejburda Jan 4, 2024

ganeshnj Jan 4, 2024

maciejburda Jan 4, 2024

ganeshnj Jan 4, 2024

maciejburda Jan 4, 2024

ganeshnj Jan 4, 2024

ncreated left a comment

ncreated Jan 5, 2024

ganeshnj Jan 5, 2024

ganeshnj Jan 5, 2024

ncreated Jan 5, 2024

ganeshnj Jan 5, 2024

ncreated left a comment

ganeshnj commented Jan 5, 2024

RUM-1836 feat(otel-tracer): conform to otel Tracer, SpanBuilder and Span #1611

RUM-1836 feat(otel-tracer): conform to otel Tracer, SpanBuilder and Span #1611

Conversation

ganeshnj commented Jan 2, 2024 • edited Loading

What and why?

How?

Review checklist

Custom CI job configuration (optional)

datadog-datadog-prod-us1 bot commented Jan 3, 2024 • edited Loading

Datadog Report

⬇️ Code Coverage Decreases vs Default Branch (7)

Choose a reason for hiding this comment

maciejburda 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

ncreated 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

ncreated left a comment

Choose a reason for hiding this comment

ganeshnj commented Jan 5, 2024

ganeshnj commented Jan 2, 2024 •

edited

Loading

datadog-datadog-prod-us1 bot commented Jan 3, 2024 •

edited

Loading