feat(telemetry): add span log correlation#969
Conversation
EhabY
force-pushed
the
feat/issue-925-span-log-correlation
branch
from
bd29095 to
e1f34f2
Compare
EhabY
force-pushed
the
feat/issue-925-span-log-correlation
branch
from
e1f34f2 to
a016d9a
Compare
|
/coder-agents-review |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, proportional addition. The Span.log()/logError() API surface is small, consistent with the existing phase() pattern, and tested across the full lifecycle: correlation, child spans, name sanitization, post-emit guards, throwing sinks, level-off suppression, and the auth interceptor integration. Test-to-code ratio is ~2:1. Netero found no mechanical issues.
The phase post-emit guard (new completed check on phase()) is a pre-existing bug fix bundled with this feature. Before this PR, calling phase() on an escaped span after the parent completed would emit an orphaned child event. Now it warns and delegates to NOOP_SPAN. Correct fix, properly tested, not mentioned in the PR description. Four reviewers independently noted this.
3 P3, 1 P4.
"'Correlated' is weasel-word territory: every event in a trace is correlated." (Leorio)
🤖 This review was automatically generated with Coder Agents.
EhabY
force-pushed
the
feat/issue-925-span-log-correlation
branch
from
45db4dd to
9afaa96
Compare
|
/coder-agents-review |
- Restore the OTel mapping on `parentEventId` and call out the dual case (phase children vs span logs). - Match `phase`'s doc style on `Span.log` / `logError`, noting absence of framework-set `result` and `durationMs`. - Replace "correlated" in test names with the concrete contract (traceId, parentEventId).
The behavior tests for post-emit guards now assert only on the visible contract (no event leak, fn still runs). One dedicated test exercises all post-emit methods (`setProperty`, `setMeasurement`, `markAborted`, `markFailure`, `log`, `logError`, `phase`) and verifies the developer-facing warn fires once for each.
EhabY
force-pushed
the
feat/issue-925-span-log-correlation
branch
from
9afaa96 to
7595903
Compare
There was a problem hiding this comment.
All four R1 findings addressed. The test restructuring is well done: behavioral tests focused on observable contracts (event suppression, callback execution) separated from the consolidated warning-count test that covers all 7 post-emit methods. wireFormat.ts doc now properly distinguishes the two OTel mappings. span.ts docs now match the phase() standard.
1 P3 (new). Otherwise clean.
"'Mutation dropped' misnames what happened. A developer debugging would look for a lost property change, not a suppressed log event." (Gon)
🤖 This review was automatically generated with Coder Agents.
mtojek
left a comment
mtojek
left a comment
There was a problem hiding this comment.
I don't see anything blocking; left a few comments 👍
`warnPostEmit` is shared by seven methods now, including `log`, `logError`, and `phase`, which aren't strictly mutations. "mutation dropped" was accurate only for the original setProperty/setMeasurement callers. Generalize to "call ignored".
EhabY
force-pushed
the
feat/issue-925-span-log-correlation
branch
from
fdc2eec to
3d6c632
Compare
2 participants
Summary
Span.log()andSpan.logError()for point-in-time telemetry logs correlated to the active span.traceIdplusparentEventIdwithoutdurationMs.auth.unauthorized_intercepted.receivedwhen 401 recovery starts.Tests
pnpm test:extension ./test/unit/telemetry/service.test.ts ./test/unit/api/authInterceptor.test.tspnpm typecheckpnpm lintpnpm format:checkCloses #925
Generated by Coder Agents.
Plan and decisions
durationMspresence as the span/log discriminator; do not addeventKind.span.log()/span.logError()after a span has completed.${span.eventName}.${logName}.auth.unauthorized_intercepted.received.