Skip to content

RUM-2149 Propagate Parent Span #1627

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 1, 2024
Merged

RUM-2149 Propagate Parent Span #1627

merged 1 commit into from
Feb 1, 2024

Conversation

maxep
Copy link
Member

@maxep maxep commented Jan 10, 2024 •
edited
Loading

What and why?

Proposal on propagating parent span id to distributing tracing.
Additionally, stop overwriting propagation headers.

How?

DatadogTrace now uses the active span as parent context in distributing tracing, if available.

Note

Parent span propagation only works with APM, not with RUM-to-APM.

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/

@maxep maxep self-assigned this Jan 10, 2024
@maxep maxep force-pushed the maxep/RUM-2149/propagate-parent-span branch 3 times, most recently from d273d9b to eb50bb4 Compare January 11, 2024 13:00
@datadog-datadog-prod-us1
Copy link

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

Datadog Report

Branch report: maxep/RUM-2149/propagate-parent-span
Commit report: eb50bb4
Test service: dd-sdk-ios

✅ 0 Failed, 2749 Passed, 0 Skipped, 11m 32.31s Wall Time
🔻 Test Sessions change in coverage: 9 decreased, 5 increased

🔻 Code Coverage Decreases vs Default Branch (9)

This report shows up to 5 code coverage decreases.

  • test DatadogLogsTests tvOS 44.76% (Δ-0.19%) - Details
  • test DatadogLogsTests iOS 44.70% (Δ-0.19%) - Details
  • test DatadogInternalTests tvOS 81.15% (Δ-0.12%) - Details
  • test DatadogInternalTests iOS 81.14% (Δ-0.09%) - Details
  • test DatadogRUMTests iOS 79.33% (Δ-0.08%) - Details

@maxep maxep marked this pull request as ready for review January 15, 2024 11:53
@maxep maxep requested review from a team as code owners January 15, 2024 11:53
@ganeshnj
Copy link
Contributor

I wonder why there is no change in the implementations of TracePropagationHeadersWriter?

For example

ncreated
ncreated previously requested changes Jan 19, 2024
View reviewed changes
@maxep maxep force-pushed the maxep/RUM-2149/propagate-parent-span branch from eb50bb4 to d6fa725 Compare January 31, 2024 10:51
@maxep maxep requested review from ncreated and a team and removed request for ncreated January 31, 2024 10:53
@maxep
Copy link
Member Author

maxep commented Jan 31, 2024 •
edited
Loading

@ganeshnj

I wonder why there is no change in the implementations of TracePropagationHeadersWriter?

Because most propagation headers do not support propagation of the parent, only b3 does.

@maxep maxep force-pushed the maxep/RUM-2149/propagate-parent-span branch from d6fa725 to f6a6df3 Compare January 31, 2024 10:57
@maxep maxep requested review from a team and removed request for a team January 31, 2024 11:00
@datadog-datadog-prod-us1
Copy link

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

Datadog Report

Branch report: maxep/RUM-2149/propagate-parent-span
Commit report: f6a6df3
Test service: dd-sdk-ios

✅ 0 Failed, 2763 Passed, 0 Skipped, 10m 46.05s Wall Time
🔻 Test Sessions change in coverage: 9 decreased, 4 increased, 1 no change

🔻 Code Coverage Decreases vs Default Branch (9)

This report shows up to 5 code coverage decreases.

  • test DatadogLogsTests tvOS 44.61% (-0.34%) - Details
  • test DatadogLogsTests iOS 44.55% (-0.34%) - Details
  • test DatadogCrashReportingTests tvOS 28.77% (-0.28%) - Details
  • test DatadogCrashReportingTests iOS 28.73% (-0.28%) - Details
  • test DatadogWebViewTrackingTests iOS 22.81% (-0.27%) - Details

maciejburda
maciejburda approved these changes Jan 31, 2024
View reviewed changes
Copy link
Member

@maciejburda maciejburda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small suggestion, otherwise LGTM

DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionTaskInterception.swift
spanID: spanID,
parentSpanID: parentSpanID
)
public func register(trace: TraceContext) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can potentially make var trace public and drop the register() function. I assume it's protocol conformance only?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, tho I'm following the pattern in place for the class.

@maxep maxep merged commit 7ecc89e into develop Feb 1, 2024
@maciejburda maciejburda mentioned this pull request Mar 19, 2024
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maciejburda maciejburda maciejburda approved these changes

@ganeshnj ganeshnj ganeshnj approved these changes

@ncreated ncreated Awaiting requested review from ncreated

Assignees

@maxep maxep

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maxep @ganeshnj @ncreated @maciejburda