-
-
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
Conversation
size-limit report 📦
|
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.
Before this get merged in, let's make sure they are documented in https://github.com/getsentry/sentry-conventions (and adjust the conventions accordingly if there are differences).
A while ago I also did this write up for uptime spans: https://www.notion.so/sentry/Uptime-Span-Conventions-1eb8b10e4b5d80799c70cec9a8a06402#1eb8b10e4b5d80799c70cec9a8a06402
Good points, thanks Abhi! So IIUC the notion doc correctly, the SDK should do the calculations for the specific phases in the request life cycle ("durations") instead of sending the raw start/end timestamp values? And in addition, send ttfb instead of something like I'll add whatever attributes we settle on to the conventions repo! |
@Lms24 sorry I should have made this more clear. I prefer the approach you have in this PR. Let's just send as much data as we can, minimizing transforms in the SDK. We can easily materialize this into the attributes that match what we have for uptime in Relay or during query time in the product. I just shared the notion doc as an FYI. |
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
75a4073
to
5a11163
Compare
I update this PR to use |
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.
I just moved this and the file below because these were incorrectly nested previously
if (isPerformanceResourceTiming(entry) && entry.name.endsWith(url)) { | ||
const spanAttributes = resourceTimingToSpanAttributes(entry); | ||
spanAttributes.forEach(attributeArray => span.setAttribute(...attributeArray)); | ||
span.setAttributes(resourceTimingToSpanAttributes(entry)); |
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.
resourceTimingToSpanAttributes
now directly returns an array, so we can directly call spanSetAttributes
here. Saves a few bytes.
// 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, |
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.
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 comment
The 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.
Nice!
// 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, |
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.
In terms of keeping the standard of seconds I would keep seconds here. It's a decimal number, right?
'http.request.worker_start': expect.any(Number), | ||
'http.request.response_end': expect.any(Number), | ||
'http.request.response_start': expect.any(Number), | ||
'http.request.time_to_first_byte': expect.any(Number), |
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.
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?
c3bad5e
to
32b3717
Compare
UPDATE: This PR was reworked quite extensively in e8fa689. PR description was updated accordingly. Would appreciate a fresh round of reviews!
This PR adds a few attributes to
resource.*
spans for request timing and status information. Most importantly:http.request.time_to_first_byte
fromresponseStart
which can be interpreted as TTFB for resource requests. This was requested via via [Performance] Include TTFB metric to the timing information of a resource in the event details sentry#63739.http.response.status_code
the status code of the resource request. Requested in Capture Status Code for Resource Requests in Sentry #16805 and [Span Statuses] Add status of resource request to span #10995To get these attributes, I adjusted the already existing
resourceTimingToSpanAttributes
a bit:http.request.redirect_end
,http.request.worker_start
attributes which are stable timing values available onPerformanceResourceTiming
but were previously missinghttp.request.worker_start
andhttp.request.redirect_end
attributes sentry-conventions#130http.request.time_to_first_byte
. Decided to add this attribute becausehttp.request.response_start
cannot be directly used as TTFB as its value is an absolute time stamp ofresponseStart
. Instead the TTFB attribute is the relative response start attribute converted to seconds (happy to change to ms if reviewers prefer).http.request.time_to_first_byte
attribute sentry-conventions#131Other consequences:
http.client
spans now also have the three additional attributesRemarks:
0
in case a value is not present. But decided to leave this as-is to avoid any behaviour change for http.client spans.closes #16805
closes #10995
closes #17554
ref getsentry/sentry#63739