fix(opentelemetry): Convert seconds timestamps in span.end() to milliseconds#19958
Open
fix(opentelemetry): Convert seconds timestamps in span.end() to milliseconds#19958
Conversation
…seconds `startTime` was already converted via `ensureTimestampInMilliseconds()`, but `span.end(timestamp)` passed the value directly to OTel which expects ms. Passing seconds (the Sentry convention) produced garbage timestamps. Patches the OTel span's `end()` method after creation so numeric timestamps are normalized before reaching OTel's native implementation. Closes #18697 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Contributor
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Deps
Bug Fixes 🐛Cloudflare
Core
Deps
Other
Internal Changes 🔧Deps Dev
Nuxt
Other
🤖 This preview updates automatically when you update the PR. |
Contributor
size-limit report 📦
|
Contributor
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a timestamp unit mismatch in @sentry/opentelemetry by ensuring numeric timestamps passed to span.end(timestamp) are converted from seconds (Sentry convention) to milliseconds (OTel expectation), matching existing startTime behavior.
Changes:
- Patch OTel spans’
end()method to run numericendTimevalues throughensureTimestampInMilliseconds()before delegating to the native OTel implementation. - Apply the
end()patch across the span creation pathways in_startSpan()andstartInactiveSpan(). - Add unit tests covering
span.end()with number (seconds/ms),Date,HrTime, and no-arg usage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/opentelemetry/src/trace.ts | Wraps Span.end() to convert numeric seconds timestamps to milliseconds before calling OTel. |
| packages/opentelemetry/test/trace.test.ts | Adds coverage validating end-time conversion behavior for multiple TimeInput forms. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
204
to
206
| function ensureTimestampInMilliseconds(timestamp: number): number { | ||
| const isMs = timestamp < 9999999999; | ||
| return isMs ? timestamp * 1000 : timestamp; |
There was a problem hiding this comment.
ensureTimestampInMilliseconds uses const isMs = timestamp < 9999999999, but that condition is actually detecting seconds (10-digit epoch) rather than milliseconds. The current variable name is misleading and makes it easy to accidentally invert the logic later. Rename the boolean (and/or extract a named constant for the threshold) to reflect that it’s checking for seconds so the intent stays clear.
Comment on lines
+2018
to
+2022
| // Use a timestamp in seconds that is after the span start (i.e. in the future) | ||
| // OTel resets endTime to startTime if endTime < startTime | ||
| const nowSec = Math.floor(Date.now() / 1000) + 1; | ||
| const span = startInactiveSpan({ name: 'test' }); | ||
| span.end(nowSec); |
There was a problem hiding this comment.
These tests use Math.floor(Date.now() / 1000) + 1 to ensure the end timestamp is after the span start time. With only a 1s buffer, slow CI (or a delay crossing a second boundary) can still produce endTime < startTime, causing OTel to clamp/reset the end time and making the assertions flaky. Consider deriving the end timestamp from the span’s actual startTime (or using a larger, deterministic offset) so the end time is guaranteed to be >= start time.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
end()method to run numeric timestamps throughensureTimestampInMilliseconds()before reaching OTel's native implementationstartTimealready had this conversion, butspan.end(timestamp)passed values directly to OTel which expects milliseconds — passing seconds (the Sentry convention) produced garbage timestamps_startSpan()andstartInactiveSpan()Closes #18697