Skip to content
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

fix(apm/tracing): update pageload op transaction start timestamps #2773

Merged
merged 4 commits into from Jul 24, 2020

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Jul 24, 2020

Background

When I was testing my react router integration, I found an issue where the duration of pageload transactions in the performance view was different than the duration shown in the span view. Note: The pageload transactions that I am referring to are the one's automatically generated by the @sentry/apm or @sentry/tracing Tracing integration.

The transaction duration in the performance view is computed straight from the transaction payload, end_timestamp - start_timestamp of the transaction. Meanwhile, the transaction duration in the span view is actually adjusted with some helper functions in the UI itself. This was to make sure that things were normalized so that strange traces would not be rendered in the UI. See the helpers here: https://github.com/getsentry/sentry/blob/master/src/sentry/static/sentry/app/components/events/interfaces/spans/utils.tsx#L500-L530

This meant there were cases where pageload transactions had a computed duration of something like 140 ms, but the span view would show a duration of 300ms. This also meant that if a user got alerted by a metric alert for average duration, they would see a different number in the individual transaction view (as metric alerts rely on transaction duration computed from snuba - end_timestamp - start_timestamp from event payload).

As this issue was only occurring in a) automatically created transactions from browser b) pageload transactions, my first instinct was to check the performance spans we add to transactions. It turns out that was causing the issue.

The Issue

The problem was caused by the fact that when we add performance spans (resource, script etc.), we do not adjust transaction start time. When you add certain performance related spans, they might have a start timestamp that is before the transaction's initial start time (they occured before sentry-tracing-init). This means that the transaction duration becomes much smaller than the duration of it's child spans.

The helpers in the Sentry UI were automatically checking to make sure that if this was the case, that the span UI would render a transaction that had the start_timestamp of the earliest child span.

In the example below, sentry-tracing-init only occurs 5000ms after all the performance related spans. This means that before the change in this PR, the computed duration in from the transaction payload would be 5000ms less than what actually is.

image

Fix

This is fixed by adjusting the performance spans to adjust transaction start time accordingly. See the helper function _startChild for more details. This fix was made to both @sentry/apm and @sentry/tracing.

This was tested with unit tests in @sentry/tracing and verified on Sentry master.

Notes

It's important to note that these changes may break existing users alerts, because naturally pageload durations will rise as it now accounts for these extra spans.

@AbhiPrasad AbhiPrasad requested review from dashed and a team July 24, 2020 13:00
@getsentry-bot
Copy link
Contributor

getsentry-bot commented Jul 24, 2020

Messages
📖

@sentry/browser bundle gzip'ed minified size: (ES5: 17.248 kB) (ES6: 16.3643 kB)

📖 ✅ TSLint passed

Generated by 🚫 dangerJS against 3ab9c2e

@github-actions
Copy link
Contributor

github-actions bot commented Jul 24, 2020

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 17.65 KB (0%)
@sentry/browser - Webpack 18.42 KB (0%)
@sentry/react - Webpack 18.42 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 22.64 KB (+0.53% 🔺)

Copy link
Member

@dashed dashed left a comment

Choose a reason for hiding this comment

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

👍 💯 🥳

packages/apm/src/integrations/tracing.ts Outdated Show resolved Hide resolved
packages/tracing/src/browser/metrics.ts Outdated Show resolved Hide resolved
@AbhiPrasad AbhiPrasad merged commit 7d7f0db into master Jul 24, 2020
@AbhiPrasad AbhiPrasad deleted the abhi/fix-pageload-timing branch July 24, 2020 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants