feat(tracing): Reset IdleTimeout based on activities count#4531
feat(tracing): Reset IdleTimeout based on activities count#4531getsentry-bot merged 15 commits intomasterfrom
Conversation
size-limit report
|
AbhiPrasad
force-pushed
the
abhi-reset-idletimeout
branch
from
9e40065 to
8c35b6d
Compare
This branch tracks the beta for `6.17.8-beta.0`. For more information, see: #4531.
CHANGELOG for non-beta items: https://github.com/getsentry/sentry-javascript/blob/master/CHANGELOG.md https://github.com/getsentry/sentry-javascript/releases/tag/6.17.8-beta.0 See getsentry/sentry-javascript#4531 for changes we are testing
CHANGELOG for non-beta items: https://github.com/getsentry/sentry-javascript/blob/master/CHANGELOG.md https://github.com/getsentry/sentry-javascript/releases/tag/6.17.8-beta.0 See getsentry/sentry-javascript#4531 for changes we are testing
It's okay - I just lowered it 4%, so we're still ahead. 😛 |
lobsterkatie
left a comment
lobsterkatie
left a comment
There was a problem hiding this comment.
A few comments, but overall looks good! (I actually didn't realize we don't already do this.)
| idleTimeout: number; | ||
|
|
||
| /** | ||
| * The max transaction duration for a transaction. If a transaction duration hits the `finalTimeout` value, it |
There was a problem hiding this comment.
| * The max transaction duration for a transaction. If a transaction duration hits the `finalTimeout` value, it | |
| * The max duration for a transaction. If a transaction duration hits the `finalTimeout` value, it |
|
|
||
| this._initTimeout = setTimeout(() => { | ||
| this._startIdleTimeout(); | ||
| global.setTimeout(() => { |
There was a problem hiding this comment.
Does this need to be called on global?
There was a problem hiding this comment.
It makes it less likely to fail in the future - so I set it this way.
| /** | ||
| * Creates an idletimeout | ||
| */ | ||
| private _startIdleTimeout(end?: Parameters<IdleTransaction['finish']>[0]): void { |
There was a problem hiding this comment.
When using Parameters, I think it's helpful to give the variable the same name as the parameter in question. (Also, renaming this makes it clearer what it actually is.)
| private _startIdleTimeout(end?: Parameters<IdleTransaction['finish']>[0]): void { | |
| private _startIdleTimeout(endTimestamp?: Parameters<IdleTransaction['finish']>[0]): void { |
| this._idleTimeoutID = global.setTimeout(() => { | ||
| if (!this._finished && Object.keys(this.activities).length === 0) { | ||
| this.setTag(FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS[1]); | ||
| this.finish(end); |
There was a problem hiding this comment.
| this.finish(end); | |
| this.finish(endTimestamp); |
| this.finish(end); | ||
| } | ||
| }, timeout); | ||
| this._startIdleTimeout(end); |
There was a problem hiding this comment.
| this._startIdleTimeout(end); | |
| this._startIdleTimeout(endTimestamp); |
(It won't let me fix the line above, because it hasn't been changed, but there, too.)
| * Creates an idletimeout | ||
| */ | ||
| private _startIdleTimeout(end?: Parameters<IdleTransaction['finish']>[0]): void { | ||
| this._cancelIdleTimeout(); |
There was a problem hiding this comment.
I wonder what test covers the behaviour changes?
Does its 2 test cases will to it?
https://github.com/getsentry/sentry-javascript/pull/4442/files#diff-69402bcbb230fa3d9774ae3cc7f75cecc78e369775e6fa222ae406f1d4dcfef2R197-R230
| 'heartbeatFailed', | ||
| 'idleTimeout', | ||
| 'documentHidden', | ||
| 'finalTimeout', |
There was a problem hiding this comment.
given that this shows up in the UI, and I as a user of sentry would probably not understand what this means if I hadn't already found this issue and read this PR, does it make more sense to name this something that indicates it's a forced timeout due to thrashing? hard to come up with a concise name but maybe something like thrashedSpanTimeout or timeoutInfinitePoll? (idleTimeout was a similarly confusing name to me, it's not obvious that's the sort of "intended finish reason" for normal transactions, but realize that probably can't be changed anymore)
There was a problem hiding this comment.
Good catch.
Its behaviour seems like as the maxTransactionDuration configuration.
There was a problem hiding this comment.
does it make sense to re-use that configuration option here then (as opposed to creating a new one), and similarly re-use the "deadline_exceeded" finishReason? or is there value in having them both be separate? I guess a default timeout of 600 seconds is pretty excessive for this particular use case...
|
@snoozbuster @alquerci thanks for the comments. We're running some tests off a beta on the Sentry frontend right now (getsentry/sentry#31761), and once we've got that data, I'll come back and formally reply to the review comments. We'll also share the data here - and give y'all an opportunity to give feedback. Appreciate the patience! |
This reverts commit e68a672.
|
Uhh that wasn't supposed to happen We are close to shipping this though. |
getsentry/sentry-javascript#4531 This will also fix the console array problem.
getsentry/sentry-javascript#4531 This will also fix the console array problem.
|
I can't locate these changes in master even though the PR says they've been merged. Is there another 6.x release I can use or should I go directly to 7.x for this? |
|
These changes are part of the latest |
|
I just switched to the 7.x branch and my staggered activities problems immediately disappeared. Thanks and great work! |
6 participants
Previously, when the activities count of an idle transaction hit 0, it would trigger a timeout for
idleTimeoutms, and then always end the transaction. This means that if a span (like fetch/xhr request) started right after the activities count went to 0 (1 -> 0 -> 1), the transaction would always finish afteridleTimeoutms, instead of waiting for the newest activity to finish.This was primarily done to prevent polling conditions and to not artificially inflate duration. Nowadays though, web vitals like LCP are a lot more important as a measurement in transactions than the strict duration (as with activities, they are a bit arbitrary). By making
idleTimeoutbe strict about finish after activities go to 0, we often times would miss the LCP value that the browser would record, leading to many transactions not having proper LCPs.To get the LCP value close to browser quality as possible, we now reset the
idleTimeouttimeout if there are still existing activities. The concern here is with polling. To prevent polling issues, we now also have an additionalfinalTimeoutthat is started when the idle transaction is created. This double timeout system (alongside the heartbeat), should always enforce that the transaction is finished.To test out these new options, we are cutting a beta to test on getsentry/sentry first, and then we will think about how to do a full release with this.
An image explanation (opt-in)
Sorry for adding bytes tracing bundle 😢