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: Make sure fetch requests are being timed correctly #2772

Merged
merged 3 commits into from Jul 24, 2020

Conversation

AbhiPrasad
Copy link
Member

Motivation

Earlier today, Daniel raised up some concerns about incorrect timings for fetch spans. This PR splits up each of those concerns into actionable items, and aims to address them.

Implementation

First it takes a look at an issue where fetch spans were being doubled in @sentry/tracing. This happens because we did not store the span correctly, so there was no span to finish. I have fixed this and added tests.

handlerData.fetchData.__span = span.spanId was added for the fix.

Second, we took a look at an issue affecting both @sentry/apm and @sentry/tracing where the end timing for spans with the same description ended up being the same. This was because we have logic in performance to adjust fetch and xhr span timestamps based on description.

Because we could not figure out a good way to keep this while recognizing unique URLs, we decided to delete this code as our tracking of spans should be enough to ensure accurate timestamps.

@getsentry-bot
Copy link
Contributor

getsentry-bot commented Jul 23, 2020

Messages
📖

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

📖 ✅ TSLint passed

Generated by 🚫 dangerJS against 234a598

@github-actions
Copy link
Contributor

github-actions bot commented Jul 23, 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.52 KB (-0.24% 🔽)

@kamilogorek
Copy link
Contributor

Good refactor 👍

@@ -40,9 +40,9 @@ export interface RequestInstrumentationOptions {
}

/** Data returned from fetch callback */
interface FetchData {
export interface FetchData {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to export FetchData and fetchCallback?

Copy link
Contributor

Choose a reason for hiding this comment

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

For tests

@@ -63,7 +63,7 @@ export class IdleTransaction extends Transaction {
private _prevHeartbeatString: string | undefined;

// Amount of times heartbeat has counted. Will cause transaction to finish after 3 beats.
private _heartbeatCounter: number = 1;
private _heartbeatCounter: number = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this wrong before?

Copy link
Member

Choose a reason for hiding this comment

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

Off by 1

@AbhiPrasad AbhiPrasad merged commit c6c54a1 into master Jul 24, 2020
@AbhiPrasad AbhiPrasad deleted the abhi/fix/tracing-timing branch July 24, 2020 14:04
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

5 participants