-
Notifications
You must be signed in to change notification settings - Fork 135
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
✨[RUM-5090] Collect resource protocol #3087
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3087 +/- ##
==========================================
- Coverage 93.68% 93.21% -0.47%
==========================================
Files 276 276
Lines 7628 7631 +3
Branches 1712 1714 +2
==========================================
- Hits 7146 7113 -33
- Misses 482 518 +36 ☔ View full report in Codecov by Sentry. |
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
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.
You'll need to add that field in rum-events-format (around here), get it merged, run rum-events-format:sync
in the SDK repo and update this PR
@@ -79,6 +79,7 @@ describe('resourceCollection', () => { | |||
download: jasmine.any(Object), | |||
first_byte: jasmine.any(Object), | |||
status_code: 200, | |||
request_protocol: 'HTTP/1.0', |
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.
💬 suggestion: rename request_protocol
to protocol
, as it is not only the request protocol but also the response and everything around :)
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.
Put it in staging before merging the PR, to make sure it works as expected there.
/to-staging |
Integrated commit sha: 86cfedc Co-authored-by: roman.gaignault <roman.gaignault@datadoghq.com>
🚂 Branch Integration: This commit was successfully integrated Commit 86cfedc81b has been merged into staging-43 in merge commit 6e41e1c15a. Check out the triggered pipeline on Gitlab 🦊 |
/to-staging |
🚂 Branch Integration: starting soon, median merge time is 10m Commit 10cc9416e0 will soon be integrated into staging-43. Use |
Integrated commit sha: 10cc941 Co-authored-by: roman.gaignault <roman.gaignault@datadoghq.com>
🚂 Branch Integration: This commit was successfully integrated Commit 10cc9416e0 has been merged into staging-43 in merge commit 92887b842b. Check out the triggered pipeline on Gitlab 🦊 |
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!
Motivation
Adding the network protocol information (e.g., http/1.1, h2) to resource events will provide deeper insights into how resources are fetched by the browser.
Changes
Utilized performanceTiming.nextHopProtocol to obtain the protocol information for same-origin requests.
Note: nextHopProtocol is only available for same-origin resources and might be restricted by the Timing-Allow-Origin header for cross-origin resources.
Testing
I have gone over the contributing documentation.