-
-
Notifications
You must be signed in to change notification settings - Fork 378
docs: clarify trace_id and span_id source in TelemetryScopeApplier #7439
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.
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
base: main
Are you sure you want to change the base?
Conversation
Add comments with links to develop.sentry.dev telemetry logs docs. span_id is only set for active span; trace_id from propagation context. No behavior change. For discussion (COCOA-1263 / #7415).
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. This PR will not appear in the changelog. 🤖 This preview updates automatically when you update the PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7439 +/- ##
=============================================
- Coverage 85.318% 85.041% -0.278%
=============================================
Files 480 480
Lines 28622 28625 +3
Branches 12384 12370 -14
=============================================
- Hits 24420 24343 -77
- Misses 4155 4232 +77
- Partials 47 50 +3
... and 13 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
itaybre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Update:
|
Description
Add documentation comments to
TelemetryScopeApplierclarifying howtrace_idandspan_idare set for logs (and other telemetry items), with links to the official SDK telemetry docs. No behavior change; this PR is for discussion and to capture what we discovered while investigating #7415This PR serves as common grounds as we need to discuss if either our develop docs are wrong.
Motivation and Context
Closes #7415
The issue asks to ensure
span_idis only set when there is an active span, and that a propagatedspan_idis not considered an active span.We cross-checked the implementation against develop.sentry.dev and the codebase and concluded the following.
Docs (develop.sentry.dev):
trace_id:span_id: "The span id of the span that was active when the log was collected."trace_idandspan_id(from incoming headers or random).Current implementation (
TelemetryScopeApplier.swift):trace_id:item.traceId = span?.traceId ?? propagationContextTraceId. When a span is active we use itstraceId; otherwise propagation context. When span is active, scope has been set with that span so the two sources match.span_id: Only set insideaddDefaultAttributeswhenself.spanis non-nil:if let span = self.span { attributes["span_id"] = .string(span.spanId.sentrySpanIdString) }. Sospan_idis only set whenscope.spanexists.Why no code change is required:
scope.spanin Sentry Cocoa is only set when a transaction/tracer is started (SentryHub.msetSpanwhen starting a tracer). It is never set from propagation context. Soscope.spanalways represents an active span we created.TelemetryScopeApplieronly receivespropagationContextTraceIdfrom the scope, not the propagation context'sspan_id. So we never have access to a "propagated span_id" in this path and cannot accidentally use it. The only span we see isscope.span, which is the active span.span_idwhen an active span exists. The comments we added document this and link to the docs so future readers and other SDKs can align.Possible follow-ups (not in this PR):
span_idas a top-level field in the log envelope payload. We currently putspan_idin theattributesdict. Moving it to a top-level field on the log payload might requireSentryLog/encoding changes and could be a separate change.How did you test it?
No behavior change.
make build-macosandmake test-ios ONLY_TESTING=TelemetryScopeApplierTestswere run; existing tests pass.Checklist
sendDefaultPiiis enabled.#skip-changelog