ref(tracing): Reset IdleTimeout based on activities count by AbhiPrasad · Pull Request #5044 · getsentry/sentry-javascript

AbhiPrasad · 2022-05-05T19:17:43Z

Ports changes from #4531 into v7

Previously, when the activities count of an idle transaction hit 0, it would trigger a timeout for idleTimeout ms, 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 after idleTimeout ms, 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 idleTimeout be 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 idleTimeout timeout if there are still existing activities. The concern here is with polling. To prevent polling issues, we now also have an additional finalTimeout that is started when the idle transaction is created. This double timeout system (alongside the heartbeat), should always enforce that the transaction is finished.

An image explanation (opt-in)

Resolves https://getsentry.atlassian.net/browse/WEB-828

AbhiPrasad · 2022-05-05T19:23:28Z

packages/tracing/src/browser/browsertracing.ts

 }
-
-/** Adjusts transaction value based on max transaction duration */
-function adjustTransactionDuration(maxDuration: number, transaction: IdleTransaction, endTimestamp: number): void {


we can remove maxTransactionDuration because it will be effectively replaced by finalTimeout.

github-actions · 2022-05-05T19:35:01Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	18.72 KB (-7.04% 🔽)
@sentry/browser - ES5 CDN Bundle (minified)	58.51 KB (-9.45% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	17.53 KB (-7.04% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)	52.69 KB (-9.11% 🔽)
@sentry/browser - Webpack (gzipped + minified)	19.2 KB (-17.38% 🔽)
@sentry/browser - Webpack (minified)	62.07 KB (-24.05% 🔽)
@sentry/react - Webpack (gzipped + minified)	19.22 KB (-17.43% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified)	42.75 KB (-11.05% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	24.41 KB (-6.39% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	22.98 KB (-6.13% 🔽)

lforst

I left a lot of comments but this PR is actually good to go as-is. 👍 Comments are mostly minor stuff and hypotheticals.

lforst · 2022-05-06T07:19:35Z

packages/tracing/src/idletransaction.ts

  /**
-   * If a transaction is created and no activities are added, we want to make sure that
-   * it times out properly. This is cleared and not used when activities are added.
+   * Timer that tracks a


Let's finish this comment :D

lforst · 2022-05-06T07:38:27Z

packages/tracing/src/browser/backgroundtab.ts

          activeTransaction.setStatus(statusType);
        }
        activeTransaction.setTag('visibilitychange', 'document.hidden');
-        activeTransaction.setTag(FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS[2]);


Just because I am interested: What prompted this change? Was it just unnecessary?

We had these in when we were investigating ways to improve LCP accuracy, but now that the investigative work is over, we can remove these tags (and save bundle accordingly).

See: #4251

lforst · 2022-05-06T07:45:44Z

packages/tracing/src/idletransaction.ts


-    this._initTimeout = setTimeout(() => {
+    this._startIdleTimeout();
+    global.setTimeout(() => {


Are we sure we want to go through with global.setTimeout? getGlobalObject() may very well be just an empty object in some environments, whereas we can pretty much assume that setTimeout is just available in the global scope of all environments.

Yeah fair, I'll revert.

lforst · 2022-05-06T07:46:25Z

packages/tracing/src/idletransaction.ts

+export const DEFAULT_FINAL_TIMEOUT = 30000;
 export const HEARTBEAT_INTERVAL = 5000;

+const global = getGlobalObject<Window>();


Can we rename this to globalObject? Just to get rid of ambiguity with node's global object.

Removed the getGlobalObject call.

lforst · 2022-05-06T07:52:02Z

packages/tracing/src/idletransaction.ts

-    if (_idleHub && _onScope) {
+    if (_onScope) {
      // There should only be one active transaction on the scope
      clearActiveTransaction(_idleHub);


I think we can make the hub parameter of clearActiveTransaction() down at the bottom non-optional now.

lforst · 2022-05-06T08:23:59Z

packages/tracing/src/idletransaction.ts

-    private readonly _idleHub?: Hub,
+    private readonly _idleHub: Hub,
    /**
     * The time to wait in ms until the idle transaction will be finished.


Doesn't have to be worded this way but it took me a while to understand when this timer should be started and stopped.

Suggested change

* The time to wait in ms until the idle transaction will be finished.

* The time to wait in ms until the idle transaction will be finished. This timer is started each time there are no active spans on this transaction.

lforst · 2022-05-06T08:25:59Z

packages/tracing/src/idletransaction.ts

  /** {@inheritDoc} */
  public finish(endTimestamp: number = timestampWithMs()): string | undefined {
    this._finished = true;
+    this._cancelIdleTimeout();


Do we actually need to cancel here? _startIdleTimeout already checks for _finished before calling finish().

We do not, updated.

lforst · 2022-05-06T08:32:17Z

packages/tracing/src/idletransaction.ts

+   * Creates an idletimeout
+   */
+  private _startIdleTimeout(endTimestamp?: Parameters<IdleTransaction['finish']>[0]): void {
+    this._cancelIdleTimeout();


Even though I get why we would call cancel here (just to be safe), I think we could get away with not calling it, because everywhere we call start() there is no way a idletimeout is running. I'll leave it up to you to keep or remove it.

lforst · 2022-05-06T08:41:32Z

packages/tracing/src/idletransaction.ts

+    this._cancelIdleTimeout();
+    this._idleTimeoutID = global.setTimeout(() => {
+      if (!this._finished && Object.keys(this.activities).length === 0) {
+        this.finish(endTimestamp);


Do we maybe want to add a this.setStatus('deadline_exceeded'); here so that transactions that are cancelled because of the idleTimeout also have that status?

No - because idletimeout should be how all idle transactions end regularly.

Previously, when the activities count of an idle transaction hit 0, it would trigger a timeout for `idleTimeout` ms, 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 after `idleTimeout` ms, 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 `idleTimeout` be 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 `idleTimeout` timeout if there are still existing activities. The concern here is with polling. To prevent polling issues, we now also have an additional `finalTimeout` that is started when the idle transaction is created. This double timeout system (alongside the heartbeat), should always enforce that the transaction is finished.

AbhiPrasad requested review from a team, k-fish, lforst and lobsterkatie and removed request for a team May 5, 2022 19:17

AbhiPrasad force-pushed the v7-abhi-reset-idletimeout branch from 39be641 to 2c09d87 Compare May 5, 2022 19:21

AbhiPrasad commented May 5, 2022

View reviewed changes

AbhiPrasad force-pushed the v7-abhi-reset-idletimeout branch 2 times, most recently from 8a00af0 to 22d0094 Compare May 5, 2022 19:59

ref(tracing): Reset IdleTimeout based on activities count

fdde150

AbhiPrasad force-pushed the v7-abhi-reset-idletimeout branch from 22d0094 to fdde150 Compare May 5, 2022 20:25

lforst approved these changes May 6, 2022

View reviewed changes

PR review changes

fb3ea96

AbhiPrasad force-pushed the v7-abhi-reset-idletimeout branch from 9441106 to fb3ea96 Compare May 6, 2022 18:56

AbhiPrasad merged commit 8b16378 into 7.x May 6, 2022

AbhiPrasad deleted the v7-abhi-reset-idletimeout branch May 6, 2022 20:11

AbhiPrasad added this to the 7.0.0 milestone May 12, 2022

AbhiPrasad mentioned this pull request Jan 12, 2023

LCP is lower than FCP in most performance report data #6744

Closed

3 tasks

	* The time to wait in ms until the idle transaction will be finished.
	* The time to wait in ms until the idle transaction will be finished. This timer is started each time there are no active spans on this transaction.

Uh oh!

Comments

Conversation

AbhiPrasad commented May 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

lforst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented May 5, 2022 •

edited

Loading