Fix disabled tracer timing still records span end times - #7116
Conversation
fubhy
added
the
audit
🦋 Changeset detectedLatest commit: a53f866 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestion inline.
Reviewed changes
Reviewed the single added regression test in packages/effect/test/Tracer.test.ts that pins the disabled tracer timing behavior for Effect.useSpan and Effect.withSpan.
The test correctly reproduces the bug: makeSpanUnsafe already consults TracerTimingEnabled for startTime, but useSpan ends spans with clock.currentTimeNanosUnsafe() unconditionally, so endTime is non-zero when timing is disabled. Running the focused reproduction command confirms the intended failure (endTime: 1000000n vs. expected 0n). The implementation fix in packages/effect/src/internal/effect.ts is still pending on this branch, which matches the PR description's stated intent.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the incremental delta since the prior Pullfrog review: the implementation fix in packages/effect/src/internal/effect.ts, the added patch changeset, and the test cleanup in packages/effect/test/Tracer.test.ts.
- Added a patch changeset documenting the disabled-timing fix.
- Routed
useSpanspan completion through the existing timing-awareendSpanhelper, soendTimestays at0nwhenTracerTimingEnabledisfalse. This alignsuseSpanwithmakeSpanScoped. - Updated the regression test to use
Effect.withTracerTiming(false)consistently with sibling tests and removed the now-unnecessaryReferencesimport.
The focused regression test and the full Tracer.test.ts suite both pass.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
2 participants
Summary
The probe returned Ended with startTime=0n and endTime=1786089303652000000n.
Important
This PR includes the focused regression test, the implementation fix, and a patch changeset.
Disabled tracer timing still records span end times
Module:
packages/effect/src/internal/effect.tsAudit ID:
relsem-tracer-disabled-timing-endSeverity / confidence: low / high
What happens
The probe returned Ended with startTime=0n and endTime=1786089303652000000n.
Why it happens
makeSpanUnsafe consults TracerTimingEnabled for startTime, but useSpan captures only Clock and always uses its current time for endTime. The scoped path correctly captures timingEnabled and calls endSpan.
Expected behavior
When tracer timing is disabled, span timing fields must remain zero for the complete span lifetime, including endTime.
Relevant implementation
These links and excerpts are pinned to audit base
b206fa5d7655c1634c9993410a9203f6616a5ca2.packages/effect/src/internal/effect.ts:5833View problematic code at
packages/effect/src/internal/effect.ts:5833/** @internal */View exact lines on GitHub
Reproduction
Observed failure: Independently rerun; failed at the intended semantic assertion.
Validation
packages/effect/testsuite: 7,988 passed, 3 skippedAudit provenance
b206fa5d7655c1634c9993410a9203f6616a5ca2b206fa5d7655c1634c9993410a9203f6616a5ca2relsem-tracer-disabled-timing-enduseSpancompletion through the timing-awareendSpanhelperCloses EFF-551