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
ref(apm): Always use monotonic clock for time calculations #2485
Conversation
Can we get this in the same PR as well please? getsentry/sentry-python#644 (comment) |
@kamilogorek I think this is not relevant, we never did this. |
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.
We managed to do a nice clean up, awesome!!
@@ -460,6 +453,7 @@ export class Tracing implements Integration { | |||
logger.log('[Tracing] finishIdleTransaction', active.transaction); | |||
// true = use timestamp of last span | |||
active.finish(true); | |||
Tracing._resetActiveTransaction(); |
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.
@HazAT could you remind me what this fixes?
Wondering if we need to call it from more places.
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.
Since we didn't "unset" the transction (clean up) after it flushed, it produced a weird debug output if you switched tabs that the active transaction was "unset" because of page going to background.
@@ -542,6 +536,11 @@ export class Tracing implements Integration { | |||
const startTime = Tracing._msToSec(entry.startTime as number); | |||
const duration = Tracing._msToSec(entry.duration as number); | |||
|
|||
if (transactionSpan.op === 'navigation' && timeOrigin + startTime < transactionSpan.startTimestamp) { | |||
logger.log('[Tracing] Discarded performance entry because of navigation'); |
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.
Was useful for debugging.
- Do we expect this to cause a lot of pollution in the console in SPAs?
- Is there enough info if we need to debug a problem? We logged the entry when manually testing this.
Just want to gauge the noise/signal ratio here.
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.
Good point, I think we can remove this.
For now, my premise was, since this integration is anyway very beta I thought the more output the better.
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.
logger.log('[Tracing] Discarded performance entry because of navigation'); | |
logger.log('[Tracing] Discarded performance entry because of navigation', entry); |
Could we do that? If we discard an entry, will be good to know what it was instead of just seeing a number of log messages that are equal in the console.
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.
Talked outside of GH and decided to remove it for now.
We can always add it back if needed.
Failed Travis build: https://travis-ci.com/getsentry/sentry-javascript/jobs/296533185 Build output (truncated)
Debugging it with |
Tests ran but job is still hanging: https://travis-ci.com/getsentry/sentry-javascript/jobs/296559174
Nothing helpful from |
For the record, there's an open issue in Wondering if anything changed in our end recently that would trigger that. |
Also remove offset time calculations since everything is based on timeOrigin
Document the argument using the same format used in microsoft/TypeScript code base.
Co-Authored-By: Rodolfo Carvalho <rodolfo.carvalho@sentry.io>
5596731
to
bbf54ad
Compare
Removing the empty tests in bbf54ad seemed to have fixed it (tested locally). |
We may reintroduce this later on-demand. For now, reduce noise.
All green in the relevant parts of https://travis-ci.com/github/getsentry/sentry-javascript/builds/152669617 |
Only CI failure is integration tests in browserstack https://travis-ci.com/github/getsentry/sentry-javascript/jobs/296606639 Seems like just a flake again.... do we even have tracing integration tests?! |
Refactoring of our time calculations, now everything is based on monotonic timeOrigin.