RUMM-3374 fix: Manual trace injection APIs are not available in `DatadogTrace` #1415

ncreated · 2023-08-16T14:32:27Z

What and why?

🐞 This PR fixes the problem of *HTTPHeaderWriter APIs being not available after importing DatadogTrace in 2.0.0.

It was a miss during 1.x → 2.x APIs migration. As a workaround, users had to import DatadogInternal directly into their code. These APIs should be available with only import DatadogTrace.

How?

The actual fix are following exports in DatadogTrace module:

@_exported import class DatadogInternal.HTTPHeadersWriter
@_exported import class DatadogInternal.B3HTTPHeadersWriter
@_exported import class DatadogInternal.W3CHTTPHeadersWriter

Extra work done:

For coherence with other APIs, I aligned naming samplingRate → sampleRate.
The OTelHTTPHeaders* APIs are now deprecated and renamed to B3HTTPHeaders*, see RUMM-3374 fix: Manual trace injection APIs are not available in DatadogTrace #1415 (comment)
I fixed few issues and improved the general sense of API comments.
Last, I added debug screen to Example project to manually test if import DatadogTrace will make writers work (unit tests are already in place):

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
Run integration tests
Run smoke tests

…` APIs to unify wording in public APIs

ncreated · 2023-08-16T14:49:45Z

DatadogInternal/Sources/NetworkInstrumentation/OpenTelemetry/OTelHTTPHeadersWriter.swift

-/// The `OTelHTTPHeadersWriter` should be used to inject trace propagation headers to
-/// the network requests send to the backend instrumented with Open Telemetry.
-/// The injected headers conform to [Open Telemetry](https://github.com/openzipkin/b3-propagation) standard.
+/// The `OTelHTTPHeadersWriter` class facilitates the injection of trace propagation headers into network requests
+/// targeted at a backend expecting [B3 propagation format](https://github.com/openzipkin/b3-propagation).
 ///
 /// Usage:
 ///
 ///     var request = URLRequest(...)
 ///
 ///     let writer = OTelHTTPHeadersWriter(injectEncoding: .single)
-///     let span = DatadogTracer.shared().startSpan("network request")
-///     writer.inject(spanContext: span.context)
+///     let span = Tracer.shared().startRootSpan(operationName: "network request")
+///     Tracer.shared().inject(spanContext: span.context, writer: writer)
 ///
 ///     writer.traceHeaderFields.forEach { (field, value) in
 ///         request.setValue(value, forHTTPHeaderField: field)
 ///     }
 ///
 ///     // call span.finish() when the request completes
 ///
-///
 public class OTelHTTPHeadersWriter: TracePropagationHeadersWriter {


💡 Even with these changes, I'm concerned about the OTelHTTPHeadersWriter name. OTel doesn't dictate this or that headers format and using "OTel" definitely does not imply B3-* headers. It should be B3HTTPHeadersWriter IMO (consistent with W3CHTTPHeadersWriter and bare HTTPHeadersWriter which uses x-datadog-* headers).

I'd be up for deprecating this symbol and renaming it accordingly. WDYT @ganeshnj @maciejburda @maxep ?

Yeah, I likeB3HTTPHeadersWriter.

Agreed :) B3* is technically more correct 👍

Renamed OTelHTTPHeaders* to B3HTTPHeaders* with deprecating the prior. See: 4a3819c

datadog-datadog-prod-us1 · 2023-08-16T14:54:15Z

Datadog Report

Branch report: ncreated/RUMM-3374/fix-manual-trace-injection-impossible
Commit report: 99dbb48

✅ dd-sdk-ios: 0 Failed, 0 New Flaky, 118 Passed, 0 Skipped, 2m 57.85s Wall Time

maciejburda

Great stuff! I like renaming proposal

maciejburda · 2023-08-16T15:39:08Z

DatadogInternal/Sources/NetworkInstrumentation/OpenTelemetry/OTelHTTPHeadersWriter.swift

-/// The `OTelHTTPHeadersWriter` should be used to inject trace propagation headers to
-/// the network requests send to the backend instrumented with Open Telemetry.
-/// The injected headers conform to [Open Telemetry](https://github.com/openzipkin/b3-propagation) standard.
+/// The `OTelHTTPHeadersWriter` class facilitates the injection of trace propagation headers into network requests
+/// targeted at a backend expecting [B3 propagation format](https://github.com/openzipkin/b3-propagation).
 ///
 /// Usage:
 ///
 ///     var request = URLRequest(...)
 ///
 ///     let writer = OTelHTTPHeadersWriter(injectEncoding: .single)
-///     let span = DatadogTracer.shared().startSpan("network request")
-///     writer.inject(spanContext: span.context)
+///     let span = Tracer.shared().startRootSpan(operationName: "network request")
+///     Tracer.shared().inject(spanContext: span.context, writer: writer)
 ///
 ///     writer.traceHeaderFields.forEach { (field, value) in
 ///         request.setValue(value, forHTTPHeaderField: field)
 ///     }
 ///
 ///     // call span.finish() when the request completes
 ///
-///
 public class OTelHTTPHeadersWriter: TracePropagationHeadersWriter {


Yeah, I likeB3HTTPHeadersWriter.

maxep

Nice! Just for clarification: why only in DatadogTrace and not in DatadogRUM?

ncreated · 2023-08-17T14:24:18Z

Nice! Just for clarification: why only in DatadogTrace and not in DatadogRUM?

It's because the current use of HeaderWriters requires a span and Tracer:

Tracer.shared().inject(spanContext: span.context)

so it won't make sense for RUM. Also, even if trace context is injected manually into request, there is currently no way to pass it to manually created RUM Resource for RUM <> APM correlation.

…tion-impossible * develop: REPLAY-1936 fix access levels Update CHANGELOG.md REPLAY-1936 add objective-c interface

…tion-impossible

ncreated added 4 commits August 16, 2023 16:19

RUMM-3374 Rename samplingRate to sampleRate in `HTTPHeadersWriter…

480be64

…` APIs to unify wording in public APIs

RUMM-3374 Export HTTPHeaderWriters to DatadogTrace public API

d8c9ea0

RUMM-3374 Add debug utility for testing manual trace injection

46ab87c

RUMM-3374 Improve doc comments for trace propagation APIs

1518ece

ncreated self-assigned this Aug 16, 2023

ncreated changed the title ~~RUMM-3374 fix: Manual trace injection APIs are not exposed in DatadogTrace~~ RUMM-3374 fix: Manual trace injection APIs are not available in DatadogTrace Aug 16, 2023

RUMM-3374 Update CHANGELOG.md

3afe2ea

ncreated force-pushed the ncreated/RUMM-3374/fix-manual-trace-injection-impossible branch from 9cd8347 to 3afe2ea Compare August 16, 2023 14:34

ncreated marked this pull request as ready for review August 16, 2023 14:45

ncreated requested a review from a team as a code owner August 16, 2023 14:45

ncreated commented Aug 16, 2023

View reviewed changes

maciejburda previously approved these changes Aug 16, 2023

View reviewed changes

maxep previously approved these changes Aug 17, 2023

View reviewed changes

RUMM-3374 Deprecate OTelHTTPHeaders* by renaming to B3HTTPHeaders*

4a3819c

ncreated dismissed stale reviews from maxep and maciejburda via 4a3819c August 18, 2023 07:00

ncreated requested review from maxep and maciejburda August 18, 2023 07:11

Merge branch 'develop' into ncreated/RUMM-3374/fix-manual-trace-injec…

599f27a

…tion-impossible * develop: REPLAY-1936 fix access levels Update CHANGELOG.md REPLAY-1936 add objective-c interface

maxep previously approved these changes Aug 18, 2023

View reviewed changes

Merge branch 'develop' into ncreated/RUMM-3374/fix-manual-trace-injec…

99dbb48

…tion-impossible

ncreated dismissed maxep’s stale review via 99dbb48 August 18, 2023 12:44

maxep approved these changes Aug 18, 2023

View reviewed changes

ncreated merged commit 12a92d3 into develop Aug 18, 2023

ncreated deleted the ncreated/RUMM-3374/fix-manual-trace-injection-impossible branch August 18, 2023 13:01

ncreated mentioned this pull request Aug 18, 2023

Release 2.1.0 #1424

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUMM-3374 fix: Manual trace injection APIs are not available in `DatadogTrace` #1415

RUMM-3374 fix: Manual trace injection APIs are not available in `DatadogTrace` #1415

ncreated commented Aug 16, 2023 •

edited

Loading

ncreated Aug 16, 2023

maciejburda Aug 16, 2023

maxep Aug 17, 2023

ncreated Aug 18, 2023

datadog-datadog-prod-us1 bot commented Aug 16, 2023 •

edited

Loading

maciejburda left a comment

maciejburda Aug 16, 2023

maxep left a comment

ncreated commented Aug 17, 2023

RUMM-3374 fix: Manual trace injection APIs are not available in DatadogTrace #1415

RUMM-3374 fix: Manual trace injection APIs are not available in DatadogTrace #1415

Conversation

ncreated commented Aug 16, 2023 • edited Loading

What and why?

How?

Review checklist

Custom CI job configuration (optional)

ncreated Aug 16, 2023

Choose a reason for hiding this comment

maciejburda Aug 16, 2023

Choose a reason for hiding this comment

maxep Aug 17, 2023

Choose a reason for hiding this comment

ncreated Aug 18, 2023

Choose a reason for hiding this comment

datadog-datadog-prod-us1 bot commented Aug 16, 2023 • edited Loading

Datadog Report

maciejburda left a comment

Choose a reason for hiding this comment

maciejburda Aug 16, 2023

Choose a reason for hiding this comment

maxep left a comment

Choose a reason for hiding this comment

ncreated commented Aug 17, 2023

RUMM-3374 fix: Manual trace injection APIs are not available in `DatadogTrace` #1415

RUMM-3374 fix: Manual trace injection APIs are not available in `DatadogTrace` #1415

ncreated commented Aug 16, 2023 •

edited

Loading

datadog-datadog-prod-us1 bot commented Aug 16, 2023 •

edited

Loading