ref(browser): Add protocol attributes to resource spans #15161
Conversation
| @@ -131,6 +131,7 @@ describe('_addResourceSpans', () => { | |||
| encodedBodySize: 256, | |||
| decodedBodySize: 256, | |||
| renderBlockingStatus: 'non-blocking', | |||
| nextHopProtocol: 'http/1.1', | |||
There was a problem hiding this comment.
This prop is needed even if the metadata is not asserted, otherwise this line throws due to undefined value:
There was a problem hiding this comment.
Yeah I was a bit worried about this but I think it's okay. MDN classifies the nextHopProtocol prop as widely available and we previously treated it as such when adding timing info to fetch request spans. So it's okay to add this I think. If we get reports, we can still safeguard usage later.
Lms24
force-pushed
the
update/metadata-resource-span
branch
from
590a5ed to
857cc4b
Compare
There was a problem hiding this comment.
Hey @Zen-cronic, once again, thanks for contributing! :D
I took the liberty to refactor the unit tests a bit and make the integration test for resource spans a bit more robust (see comments).
| @@ -131,6 +131,7 @@ describe('_addResourceSpans', () => { | |||
| encodedBodySize: 256, | |||
| decodedBodySize: 256, | |||
| renderBlockingStatus: 'non-blocking', | |||
| nextHopProtocol: 'http/1.1', | |||
There was a problem hiding this comment.
Yeah I was a bit worried about this but I think it's okay. MDN classifies the nextHopProtocol prop as widely available and we previously treated it as such when adding timing info to fetch request spans. So it's okay to add this I think. If we get reports, we can still safeguard usage later.
| ['sip/2', { name: 'sip', version: '2' }], | ||
| ['tds/8.0', { name: 'tds', version: '8.0' }], | ||
| ['dicom', { name: 'dicom', version: 'unknown' }], | ||
| ['', { name: '', version: 'unknown' }], |
There was a problem hiding this comment.
I added this last case here because according to MDN, nextHopProtocol will be empty for cross-origin resource requests where the responses don't contain a Timing-Allow-Origin header.
I was briefly debating changing the implementation to {name: 'unknown', version: 'unknown'} for consistency but then realized that this doesn't let us distinguish between the cross origin case and when we simply don't recognize the protocol. So let's leave it as-is for now.
| 'network.protocol.name': 'unknown', | ||
| 'network.protocol.version': 'unknown', |
There was a problem hiding this comment.
I was hoping this would actually be populated but looks like PlayWright/Chromium Headless emits a '' despite me returning a Timing-Allow-Origin: * header. Same goes for the deliveryType property which follows a similar pattern
There was a problem hiding this comment.
gotcha, i tried asserting the network.protocol.name and version here as well. But they came out to be unkown. And the properties were more comprehensively tested in the unit test, so i didn't add them here.
thanks for updating the integration test for better coverage!
There was a problem hiding this comment.
Yeah, I'm also not super happy about this but it looks like some browser or playwright bugs might be responsible for the ''. Coincidentally, we opened #15164 where it seems like at least some changes were made to cross origin performance entry data. So maybe this actually solves itself.
|
hey @Lms24 , anytime! I've been working on the graphqlClientIntegration for a while now, and i got somewhat familiar with the |
|
FYI, I'm waiting on #15164 before merging this to see if the integration tests need some adjustments afterwards. |
- Relocated `extractNetworkProtocol` util to `browser-utils`. - Modified `browserMetrics` test to assert the new metadata. Signed-off-by: Kaung Zin Hein <[email protected]>
Lms24
force-pushed
the
update/metadata-resource-span
branch
from
d1323e9 to
b75eb03
Compare
Lms24
changed the title
This PR adds the external contributor to the CHANGELOG.md file, so that they are credited for their contribution. See #15161 --------- Co-authored-by: Lms24 <[email protected]> Co-authored-by: Lukas Stracke <[email protected]>
Resolves #12395
Changes made:
extractNetworkProtocolutil and its unit test tobrowser-utils.browserMetricstest to assert the new metadata.yarn lint) & (yarn test).