feat(tracing): Track PerformanceResourceTiming.renderBlockingStatus
#7127
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As per https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming/renderBlockingStatus the resource timing has information about whether an asset is blocking render or not, which is useful for determining which assets need to be addressed for fixing critical path.
k-fish
requested review from
a team,
lforst and
AbhiPrasad
and removed request for
a team
k-fish
added a commit
to getsentry/develop
that referenced
this pull request
Feb 9, 2023
Adds mention of new data on resource spans, see getsentry/sentry-javascript#7127
size-limit report 📦
|
AbhiPrasad
reviewed
Feb 9, 2023
| @@ -361,6 +362,9 @@ export function _addResourceSpans( | |||
| if ('decodedBodySize' in entry) { | |||
| data['Decoded Body Size'] = entry.decodedBodySize; | |||
| } | |||
| if ('renderBlockingStatus' in entry) { | |||
| data['Render Blocking Status'] = entry.renderBlockingStatus; | |||
There was a problem hiding this comment.
so, I would like our data fields to move toward being similar to otel span attributes in formatting, lowercase and snake case, with period separators similar to span operation.
Can we use resource.render_blocking_status instead here?
see trace semantic conventions: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/README.md
There was a problem hiding this comment.
I was actually going to mention this, it was wild there are spaces etc. Very in 👍
There was a problem hiding this comment.
yeah not ideal we have Decoded Body Size, maybe we can change this too though? I kept it as is for backwards compat sake, but if we can update the detectors we can change it here as well.
There was a problem hiding this comment.
If you want to look at changing these existing fields, we'd have to have both for a while (maybe 30d), are we fine with that?
Replay SDK metrics 🚀
develop |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Revision | LCP | CLS | CPU | JS heap avg | JS heap max | netTx | netRx | netCount | netTime |
|---|---|---|---|---|---|---|---|---|---|
| 538c3a6 | +63.63 ms | +0.00 ms | +14.14 pp | +1.07 MB | +2.16 MB | +2.6 kB | +41 B | +1 | +112.08 ms |
| fc7b716 | +52.44 ms | +0.00 ms | +14.74 pp | +1.09 MB | +2.21 MB | +2.68 kB | +41 B | +1 | +89.64 ms |
*) pp - percentage points - an absolute difference between two percentages.
Last updated: Thu, 09 Feb 2023 20:39:45 GMT
Last updated: Thu, 09 Feb 2023 20:39:45 GMT
AbhiPrasad
reviewed
Feb 10, 2023
| @@ -77,6 +77,7 @@ export function Transaction(obj?: Partial<Event>): any { | |||
| 'Transfer Size': 1097, | |||
| 'Encoded Body Size': 797, | |||
| 'Decoded Body Size': 1885, | |||
| 'resource.render_blocking_status': 'non-blocking', | |||
There was a problem hiding this comment.
One more bikeshed convo :p
Right now the blocked_main_thread span data key is set in the mobile sdks, to get File I/O perf issues working.
Could we re-use that? I'm fine with not - this works with me as well.
There was a problem hiding this comment.
There was a problem hiding this comment.
To be clear you want to re-use the same key? I don't know about that, I'd prefer more closely matching the vendor implementation we're scraping from otherwise there will be work in the product to constantly explain the differences, vs. letting these keys be self-evident
There was a problem hiding this comment.
Yup makes sense, let's keep this unique to browser!
AbhiPrasad
approved these changes
Feb 10, 2023
AbhiPrasad
changed the title
AbhiPrasad
changed the title
AbhiPrasad
changed the title
PerformanceResourceTiming.renderBlockingStatus
k-fish
added a commit
to getsentry/develop
that referenced
this pull request
Feb 13, 2023
* docs(sdk): Update render blocking status. Adds mention of new data on resource spans, see getsentry/sentry-javascript#7127
AbhiPrasad
pushed a commit
that referenced
this pull request
Feb 14, 2023
…#7127) * ref(perf): Add renderBlockingStatus As per https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming/renderBlockingStatus the resource timing has information about whether an asset is blocking render or not, which is useful for determining which assets need to be addressed for fixing critical path.
ramchaik
pushed a commit
to ramchaik/sentry-javascript
that referenced
this pull request
Feb 14, 2023
…getsentry#7127) * ref(perf): Add renderBlockingStatus As per https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming/renderBlockingStatus the resource timing has information about whether an asset is blocking render or not, which is useful for determining which assets need to be addressed for fixing critical path.
mjq-sentry
added a commit
to getsentry/sentry
that referenced
this pull request
Feb 22, 2023
…44981) As of browser SDK 7.38.0, we now [log the render-blocking status](getsentry/sentry-javascript#7127) reported by the browser in resource spans' `data` hash. (Background info: [MDN](https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming/renderBlockingStatus)). If a value is present and that span is `non-blocking`, ignore our heuristics and treat the resource as non-blocking in the Large Render Blocking Asset detector. This commit also updates each test case to start from a valid event and then minimally mutate it into an invalid one, because I was losing confidence that each test was really testing what it should.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As per https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming/renderBlockingStatus the resource timing has information about whether an asset is blocking render or not, which is useful for determining which assets need to be addressed for fixing critical path.