Skip to content

fix(apm): Check activities count before finishing idle transaction #2672

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

fix(apm): Check activities count before finishing idle transaction #2672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ed26a7e
fix(apm): Check activities count before finishing idle transaction
AbhiPrasad Jun 15, 2020
302a881
docs: Add CHANGELOG
AbhiPrasad Jun 15, 2020
8af8fd8
fix(apm): Spelling mistakes
AbhiPrasad Jun 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
- [core] fix: Call `bindClient` when creating new `Hub` to make integrations work automatically (#2665)
- [gatsby] feat: Add @sentry/gatsby package (#2652)
- [core] ref: Rename `whitelistUrls/blacklistUrls` to `allowUrls/denyUrls`
- [apm] fix: Check activities count before finishing idle transaction (#2672)

## 5.17.0

Expand Down
19 changes: 10 additions & 9 deletions packages/apm/src/integrations/tracing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export interface TracingOptions {
/**
* Flag Transactions where tabs moved to background with "cancelled". Browser background tab timing is
* not suited towards doing precise measurements of operations. Background transaction can mess up your
* statistics in non deterministic ways that's why we by default recommend leaving this opition enabled.
* statistics in non deterministic ways that's why we by default recommend leaving this option enabled.
*
* Default: true
*/
Expand Down Expand Up @@ -194,7 +194,7 @@ export class Tracing implements Integration {
traceXHR: true,
tracingOrigins: defaultTracingOrigins,
};
// NOTE: Logger doesn't work in contructors, as it's initialized after integrations instances
// NOTE: Logger doesn't work in constructors, as it's initialized after integrations instances
if (!_options || !Array.isArray(_options.tracingOrigins) || _options.tracingOrigins.length === 0) {
this._emitOptionsWarning = true;
}
Expand Down Expand Up @@ -481,7 +481,7 @@ export class Tracing implements Integration {
Tracing._log('[Tracing] cancelling span since transaction ended early', JSON.stringify(span, undefined, 2));
}

// We remove all spans that happend after the end of the transaction
// We remove all spans that occurred after the end of the transaction
// This is here to prevent super long transactions and timing issues
const keepSpan = span.startTimestamp < endTimestamp;
if (!keepSpan) {
Expand Down Expand Up @@ -764,7 +764,7 @@ export class Tracing implements Integration {
}

/**
* Starts tracking for a specifc activity
* Starts tracking for a specific activity
*
* @param name Name of the activity, can be any string (Only used internally to identify the activity)
* @param spanContext If provided a Span with the SpanContext will be created.
Expand Down Expand Up @@ -801,7 +801,7 @@ export class Tracing implements Integration {
}

Tracing._log(`[Tracing] pushActivity: ${name}#${Tracing._currentIndex}`);
Tracing._log('[Tracing] activies count', Object.keys(Tracing._activities).length);
Tracing._log('[Tracing] activities count', Object.keys(Tracing._activities).length);
if (options && typeof options.autoPopAfter === 'number') {
Tracing._log(`[Tracing] auto pop of: ${name}#${Tracing._currentIndex} in ${options.autoPopAfter}ms`);
const index = Tracing._currentIndex;
Expand Down Expand Up @@ -852,17 +852,18 @@ export class Tracing implements Integration {
}

const count = Object.keys(Tracing._activities).length;

Tracing._log('[Tracing] activies count', count);
Tracing._log('[Tracing] activities count', count);

if (count === 0 && Tracing._activeTransaction) {
const timeout = Tracing.options && Tracing.options.idleTimeout;
Tracing._log(`[Tracing] Flushing Transaction in ${timeout}ms`);
// We need to add the timeout here to have the real endtimestamp of the transaction
// Remeber timestampWithMs is in seconds, timeout is in ms
// Remember timestampWithMs is in seconds, timeout is in ms
const end = timestampWithMs() + timeout / 1000;
setTimeout(() => {
Tracing.finishIdleTransaction(end);
if (Object.keys(Tracing._activities).length === 0 && Tracing._activeTransaction) {
Tracing.finishIdleTransaction(end);
}
Comment on lines +864 to +866
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to be careful here. I tinkered around with this for a long time.
While in your specific example it might improve the transaction, in other cases this has the potential of not sending the transaction at all.
Also, this change was made on purpose, before (see git history) we have a debouce timer that kept the Transaction alive which resulted in may weird very long Transaction. This change was made so after all the initial requests are done and the first "idle" occurs, we finish the Transaction no matter what (to prevent long transactions).
We finish Span that would have been started and mark them as cancelled.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. The issue here is that right now the moment the activities goes to 0, the idle transaction ends.

I see what you mean by long running transaction (like if a page keeps creating single activities), but a user can easily just adjust the timeout option to instantly call finish idle transaction.

We could also apply a heuristic here, and say something like if the activities went from 0 -> 1 -> 0 -> 1 a certain amount of times, we should just end the idle transaction.

This change should still lead to transactions finishing through the heartbeat though right?

Right now testing on multiple Sentry pages, a single page load fails to account for a lot of our requests, and I’d rather optimize for that use case then 0 -> 1 -> 0 ->1 causing long running transactions.

}, timeout);
}
}
Expand Down