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 2 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
5 changes: 3 additions & 2 deletions packages/apm/src/integrations/tracing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -852,7 +852,6 @@ export class Tracing implements Integration {
}

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

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

if (count === 0 && Tracing._activeTransaction) {
Expand All @@ -862,7 +861,9 @@ export class Tracing implements Integration {
// Remeber 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