-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(browser): Add timing and status atttributes to resource spans #17562
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
Changes from all commits
0a38dc6
ace4f38
0da56f5
fe24c43
6d59bb0
37edc3e
ce5c8e6
c608dab
4ebf1f7
993c23b
dc2085d
32b3717
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
import type { SpanAttributes } from '@sentry/core'; | ||
import { browserPerformanceTimeOrigin } from '@sentry/core'; | ||
import { extractNetworkProtocol, getBrowserPerformanceAPI } from './utils'; | ||
|
||
function getAbsoluteTime(time = 0): number { | ||
return ((browserPerformanceTimeOrigin() || performance.timeOrigin) + time) / 1000; | ||
} | ||
|
||
/** | ||
* Converts a PerformanceResourceTiming entry to span data for the resource span. Most importantly, | ||
* it converts the timing values from timestamps relative to the `performance.timeOrigin` to absolute timestamps | ||
* in seconds. | ||
* | ||
* @see https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming#timestamps | ||
* | ||
* @param resourceTiming | ||
* @returns An array where the first element is the attribute name and the second element is the attribute value. | ||
*/ | ||
export function resourceTimingToSpanAttributes(resourceTiming: PerformanceResourceTiming): SpanAttributes { | ||
const timingSpanData: SpanAttributes = {}; | ||
// Checking for only `undefined` and `null` is intentional because it's | ||
// valid for `nextHopProtocol` to be an empty string. | ||
if (resourceTiming.nextHopProtocol != undefined) { | ||
const { name, version } = extractNetworkProtocol(resourceTiming.nextHopProtocol); | ||
timingSpanData['network.protocol.version'] = version; | ||
timingSpanData['network.protocol.name'] = name; | ||
} | ||
|
||
if (!(browserPerformanceTimeOrigin() || getBrowserPerformanceAPI()?.timeOrigin)) { | ||
return timingSpanData; | ||
} | ||
Lms24 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return { | ||
...timingSpanData, | ||
|
||
'http.request.redirect_start': getAbsoluteTime(resourceTiming.redirectStart), | ||
'http.request.redirect_end': getAbsoluteTime(resourceTiming.redirectEnd), | ||
|
||
'http.request.worker_start': getAbsoluteTime(resourceTiming.workerStart), | ||
|
||
'http.request.fetch_start': getAbsoluteTime(resourceTiming.fetchStart), | ||
|
||
'http.request.domain_lookup_start': getAbsoluteTime(resourceTiming.domainLookupStart), | ||
'http.request.domain_lookup_end': getAbsoluteTime(resourceTiming.domainLookupEnd), | ||
|
||
'http.request.connect_start': getAbsoluteTime(resourceTiming.connectStart), | ||
'http.request.secure_connection_start': getAbsoluteTime(resourceTiming.secureConnectionStart), | ||
'http.request.connection_end': getAbsoluteTime(resourceTiming.connectEnd), | ||
|
||
'http.request.request_start': getAbsoluteTime(resourceTiming.requestStart), | ||
|
||
'http.request.response_start': getAbsoluteTime(resourceTiming.responseStart), | ||
'http.request.response_end': getAbsoluteTime(resourceTiming.responseEnd), | ||
|
||
// For TTFB we actually want the relative time from timeOrigin to responseStart | ||
// This way, TTFB always measures the "first page load" experience. | ||
// see: https://web.dev/articles/ttfb#measure-resource-requests | ||
'http.request.time_to_first_byte': (resourceTiming.responseStart ?? 0) / 1000, | ||
Comment on lines
+55
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious on reviewers' opinions on this one: I decided to convert this value to seconds to stick with us mostly sending seconds-based values. Happy to leave at ms if reviewers prefer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In terms of keeping the standard of seconds I would keep seconds here. It's a decimal number, right? |
||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda related to the other comment: Maybe we can check here that it's a decimal number.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm so theoretically, this could also be an integer, if TTFB happens to be exactly 1000ms for example. We could test against something like
expect(Number.isIntegrer(attributes['http.request.time_to_first_byte'])).toBe(false)
but this could introduce flakiness (albeit unlikely). WDYT about rather doing a range check instead? 0 < ttfb < 10?