Conversation
…ectron into feat/startup-tracing
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛
Build / dependencies / internal 🔧Release
Other
Other
🤖 This preview updates automatically when you update the PR. |
AbhiPrasad
left a comment
AbhiPrasad
left a comment
There was a problem hiding this comment.
It would be nice to enable this by default, but we can probably do that in the next major.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| startTime, | ||
| }, | ||
| (span) => { | ||
| span.end(startTime * 1000); |
There was a problem hiding this comment.
Bug: The span.end() method is called with timestamps multiplied by 1000, likely passing milliseconds instead of the expected seconds, which will result in incorrect span end times.
Severity: HIGH
🔍 Detailed Analysis
The code retrieves timestamps in seconds, for example from (Date.now() - uptimeMs) / 1000 or a Sentry event's start_timestamp. While startSpanManual() correctly uses these timestamps as seconds, subsequent calls to span.end() incorrectly multiply the timestamp value by 1000. This likely passes a millisecond value to a method that expects seconds, which would result in spans having invalid end timestamps set far in the future. This inconsistent handling suggests the multiplication is an error.
💡 Suggested Fix
Remove the * 1000 multiplication from all calls to span.end() to ensure timestamps are passed consistently in seconds, matching how they are used in startSpanManual().
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/main/integrations/startup-tracing.ts#L66
Potential issue: The code retrieves timestamps in seconds, for example from `(Date.now()
- uptimeMs) / 1000` or a Sentry event's `start_timestamp`. While `startSpanManual()`
correctly uses these timestamps as seconds, subsequent calls to `span.end()` incorrectly
multiply the timestamp value by 1000. This likely passes a millisecond value to a method
that expects seconds, which would result in spans having invalid end timestamps set far
in the future. This inconsistent handling suggests the multiplication is an error.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8493281
There was a problem hiding this comment.
There is a bug in the Node SDK:
Until then I could add it to the docs config selector like we already do for browser tracing. |
2 participants
Used this like:
main.jsThe
startupTracingIntegrationcaptures Electron startup events as tracing spans.If you also use the
browserTracingIntegrationin the renderer, this envelope is intercepted and it's spans are added to the main process startup transaction.renderer.jsOutstanding issues
performance.traceOriginsentry#105903Once this is merged it will look like this: