From 9339e1b0ef438fed2d1e5f9b49a117316ecabca6 Mon Sep 17 00:00:00 2001 From: Kev <6111995+k-fish@users.noreply.github.com> Date: Tue, 11 Jul 2023 11:10:57 -0400 Subject: [PATCH 1/9] fix(tracing): Improve network.protocol.version Protocols are from https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids which don't exactly match the OSI. We can consider adding a lookup table later if people are finding this insufficient. --- packages/tracing-internal/src/browser/request.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index 071b2146bb16..00c7011fe16b 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -183,8 +183,8 @@ function addHTTPTimings(span: Span): void { } function resourceTimingEntryToSpanData(resourceTiming: PerformanceResourceTiming): [string, string | number][] { - const version = resourceTiming.nextHopProtocol.split('/')[1] || 'none'; - + const version = resourceTiming.nextHopProtocol.split('/')[1] || resourceTiming.nextHopProtocol.split('h')[1] || 'unknown'; + const timingSpanData: [string, string | number][] = []; if (version) { timingSpanData.push(['network.protocol.version', version]); From 5823ee7e66c56cbbb77a2af7e6519aca1992f4fa Mon Sep 17 00:00:00 2001 From: Kev <6111995+k-fish@users.noreply.github.com> Date: Tue, 11 Jul 2023 11:14:53 -0400 Subject: [PATCH 2/9] Also add protocol to disambiguate SPDY --- packages/tracing-internal/src/browser/request.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index 00c7011fe16b..d2c16e74059c 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -183,12 +183,12 @@ function addHTTPTimings(span: Span): void { } function resourceTimingEntryToSpanData(resourceTiming: PerformanceResourceTiming): [string, string | number][] { + const name = resourceTiming.nextHopProtocol.split('/')[0].toLowerCase() || (resourceTiming.nextHopProtocol.startsWith('h') && 'http') || 'unknown'; const version = resourceTiming.nextHopProtocol.split('/')[1] || resourceTiming.nextHopProtocol.split('h')[1] || 'unknown'; const timingSpanData: [string, string | number][] = []; - if (version) { - timingSpanData.push(['network.protocol.version', version]); - } + + timingSpanData.push(['network.protocol.version', version], ['network.protocol.name', name]); if (!browserPerformanceTimeOrigin) { return timingSpanData; From f0cdc056055f775c38a9652e1f11b9bbb8b5e69d Mon Sep 17 00:00:00 2001 From: k-fish Date: Fri, 14 Jul 2023 11:28:48 -0400 Subject: [PATCH 3/9] Clean up and add thorough tests --- .../tracing-internal/src/browser/request.ts | 19 +++++-- .../test/browser/request.test.ts | 53 ++++++++++++++++++- 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index d2c16e74059c..9c437e0f27b2 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -182,12 +182,23 @@ function addHTTPTimings(span: Span): void { }); } +/** + * Converts ALPN protocol ids to name and version. + * + * (https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids) + * @param nextHopProtocol PerformanceResourceTiming.nextHopProtocol + */ +export function extractNetworkProtocol(nextHopProtocol: string): { name: string; version: string } { + const name = nextHopProtocol.split('/')[0].toLowerCase() || (nextHopProtocol.startsWith('h') && 'http') || 'unknown'; + const version = nextHopProtocol.split('/')[1] || nextHopProtocol.split('h')[1] || 'unknown'; + return { name, version }; +} + function resourceTimingEntryToSpanData(resourceTiming: PerformanceResourceTiming): [string, string | number][] { - const name = resourceTiming.nextHopProtocol.split('/')[0].toLowerCase() || (resourceTiming.nextHopProtocol.startsWith('h') && 'http') || 'unknown'; - const version = resourceTiming.nextHopProtocol.split('/')[1] || resourceTiming.nextHopProtocol.split('h')[1] || 'unknown'; - + const { name, version } = extractNetworkProtocol(resourceTiming.nextHopProtocol); + const timingSpanData: [string, string | number][] = []; - + timingSpanData.push(['network.protocol.version', version], ['network.protocol.name', name]); if (!browserPerformanceTimeOrigin) { diff --git a/packages/tracing-internal/test/browser/request.test.ts b/packages/tracing-internal/test/browser/request.test.ts index 9c8307e97fd7..3d43929d1ca0 100644 --- a/packages/tracing-internal/test/browser/request.test.ts +++ b/packages/tracing-internal/test/browser/request.test.ts @@ -7,7 +7,13 @@ import type { Transaction } from '../../../tracing/src'; import { addExtensionMethods, Span, spanStatusfromHttpCode } from '../../../tracing/src'; import { getDefaultBrowserClientOptions } from '../../../tracing/test/testutils'; import type { FetchData, XHRData } from '../../src/browser/request'; -import { fetchCallback, instrumentOutgoingRequests, shouldAttachHeaders, xhrCallback } from '../../src/browser/request'; +import { + extractNetworkProtocol, + fetchCallback, + instrumentOutgoingRequests, + shouldAttachHeaders, + xhrCallback, +} from '../../src/browser/request'; import { TestClient } from '../utils/TestClient'; beforeAll(() => { @@ -388,6 +394,51 @@ describe('callbacks', () => { }); }); +describe('HTTPTimings', () => { + describe('Extracting version from ALPN protocol', () => { + const nextHopToNetworkVersion = { + 'http/0.9': { name: 'http', version: '0.9' }, + 'http/1.0': { name: 'http', version: '1.0' }, + 'http/1.1': { name: 'http', version: '1.1' }, + 'spdy/1': { name: 'spdy', version: '1' }, + 'spdy/2': { name: 'spdy', version: '2' }, + 'spdy/3': { name: 'spdy', version: '3' }, + 'stun.turn': { name: 'stun.turn', version: 'none' }, + 'stun.nat-discovery': { name: 'stun.nat-discovery', version: 'none' }, + h2: { name: 'http', version: '2' }, + h2c: { name: 'http', version: '2c' }, + webrtc: { name: 'webrtc', version: 'none' }, + 'c-webrtc': { name: 'c-webrtc', version: 'none' }, + ftp: { name: 'ftp', version: 'none' }, + imap: { name: 'imap', version: 'none' }, + pop3: { name: 'pop', version: '3' }, + managesieve: { name: 'managesieve', version: 'none' }, + coap: { name: 'coap', version: 'none' }, + 'xmpp-client': { name: 'xmpp-client', version: 'none' }, + 'xmpp-server': { name: 'xmpp-server', version: 'none' }, + 'acme-tls/1': { name: 'acme-tls', version: '1' }, + mqtt: { name: 'mqtt', version: 'none' }, + dot: { name: 'dot', version: 'none' }, + 'ntske/1': { name: 'ntske', version: '1' }, + sunrpc: { name: 'sunrpc', version: 'none' }, + h3: { name: 'http', version: '3' }, + smb: { name: 'smb', version: 'none' }, + irc: { name: 'irc', version: 'none' }, + nntp: { name: 'nntp', version: 'none' }, + nnsp: { name: 'nnsp', version: 'none' }, + doq: { name: 'doq', version: 'none' }, + 'sip/2': { name: 'sip', version: '2' }, + 'tds/8.0': { name: 'tds', version: '8.0' }, + dicom: { name: 'dicom', version: 'none' }, + }; + + const protocols = Object.keys(nextHopToNetworkVersion); + for (const protocol of protocols) { + expect(extractNetworkProtocol(protocol)).toMatchObject(nextHopToNetworkVersion[protocol]); + } + }); +}); + describe('shouldAttachHeaders', () => { describe('should prefer `tracePropagationTargets` over defaults', () => { it('should return `true` if the url matches the new tracePropagationTargets', () => { From f833e5b7e5c786029f6165449b6d1eb506a9f77e Mon Sep 17 00:00:00 2001 From: k-fish Date: Fri, 14 Jul 2023 12:14:44 -0400 Subject: [PATCH 4/9] Fix any type --- packages/tracing-internal/test/browser/request.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/tracing-internal/test/browser/request.test.ts b/packages/tracing-internal/test/browser/request.test.ts index 3d43929d1ca0..4af9f288b3df 100644 --- a/packages/tracing-internal/test/browser/request.test.ts +++ b/packages/tracing-internal/test/browser/request.test.ts @@ -434,7 +434,8 @@ describe('HTTPTimings', () => { const protocols = Object.keys(nextHopToNetworkVersion); for (const protocol of protocols) { - expect(extractNetworkProtocol(protocol)).toMatchObject(nextHopToNetworkVersion[protocol]); + const expected: { name: string; version: string } = nextHopToNetworkVersion[protocol]; + expect(extractNetworkProtocol(protocol)).toMatchObject(expected); } }); }); From f99dc65989ddd5ec1dcdb49398901ea138bb1d2e Mon Sep 17 00:00:00 2001 From: k-fish Date: Fri, 14 Jul 2023 14:32:11 -0400 Subject: [PATCH 5/9] Use type throughout --- packages/tracing-internal/test/browser/request.test.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/tracing-internal/test/browser/request.test.ts b/packages/tracing-internal/test/browser/request.test.ts index 4af9f288b3df..100703a1a969 100644 --- a/packages/tracing-internal/test/browser/request.test.ts +++ b/packages/tracing-internal/test/browser/request.test.ts @@ -394,9 +394,14 @@ describe('callbacks', () => { }); }); +interface ProtocolInfo { + name: string; + version: string; +} + describe('HTTPTimings', () => { describe('Extracting version from ALPN protocol', () => { - const nextHopToNetworkVersion = { + const nextHopToNetworkVersion: Record = { 'http/0.9': { name: 'http', version: '0.9' }, 'http/1.0': { name: 'http', version: '1.0' }, 'http/1.1': { name: 'http', version: '1.1' }, @@ -434,7 +439,7 @@ describe('HTTPTimings', () => { const protocols = Object.keys(nextHopToNetworkVersion); for (const protocol of protocols) { - const expected: { name: string; version: string } = nextHopToNetworkVersion[protocol]; + const expected: ProtocolInfo = nextHopToNetworkVersion[protocol]; expect(extractNetworkProtocol(protocol)).toMatchObject(expected); } }); From 138da56dcac2afa61ae5ee5b4bfe3c5316971918 Mon Sep 17 00:00:00 2001 From: k-fish Date: Fri, 14 Jul 2023 15:13:37 -0400 Subject: [PATCH 6/9] Switch unknown to real values --- .../test/browser/request.test.ts | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/packages/tracing-internal/test/browser/request.test.ts b/packages/tracing-internal/test/browser/request.test.ts index 100703a1a969..992b50768428 100644 --- a/packages/tracing-internal/test/browser/request.test.ts +++ b/packages/tracing-internal/test/browser/request.test.ts @@ -408,33 +408,33 @@ describe('HTTPTimings', () => { 'spdy/1': { name: 'spdy', version: '1' }, 'spdy/2': { name: 'spdy', version: '2' }, 'spdy/3': { name: 'spdy', version: '3' }, - 'stun.turn': { name: 'stun.turn', version: 'none' }, - 'stun.nat-discovery': { name: 'stun.nat-discovery', version: 'none' }, + 'stun.turn': { name: 'stun.turn', version: 'unknown' }, + 'stun.nat-discovery': { name: 'stun.nat-discovery', version: 'unknown' }, h2: { name: 'http', version: '2' }, h2c: { name: 'http', version: '2c' }, - webrtc: { name: 'webrtc', version: 'none' }, - 'c-webrtc': { name: 'c-webrtc', version: 'none' }, - ftp: { name: 'ftp', version: 'none' }, - imap: { name: 'imap', version: 'none' }, + webrtc: { name: 'webrtc', version: 'unknown' }, + 'c-webrtc': { name: 'c-webrtc', version: 'unknown' }, + ftp: { name: 'ftp', version: 'unknown' }, + imap: { name: 'imap', version: 'unknown' }, pop3: { name: 'pop', version: '3' }, - managesieve: { name: 'managesieve', version: 'none' }, - coap: { name: 'coap', version: 'none' }, - 'xmpp-client': { name: 'xmpp-client', version: 'none' }, - 'xmpp-server': { name: 'xmpp-server', version: 'none' }, + managesieve: { name: 'managesieve', version: 'unknown' }, + coap: { name: 'coap', version: 'unknown' }, + 'xmpp-client': { name: 'xmpp-client', version: 'unknown' }, + 'xmpp-server': { name: 'xmpp-server', version: 'unknown' }, 'acme-tls/1': { name: 'acme-tls', version: '1' }, - mqtt: { name: 'mqtt', version: 'none' }, - dot: { name: 'dot', version: 'none' }, + mqtt: { name: 'mqtt', version: 'unknown' }, + dot: { name: 'dot', version: 'unknown' }, 'ntske/1': { name: 'ntske', version: '1' }, - sunrpc: { name: 'sunrpc', version: 'none' }, + sunrpc: { name: 'sunrpc', version: 'unknown' }, h3: { name: 'http', version: '3' }, - smb: { name: 'smb', version: 'none' }, - irc: { name: 'irc', version: 'none' }, - nntp: { name: 'nntp', version: 'none' }, - nnsp: { name: 'nnsp', version: 'none' }, - doq: { name: 'doq', version: 'none' }, + smb: { name: 'smb', version: 'unknown' }, + irc: { name: 'irc', version: 'unknown' }, + nntp: { name: 'nntp', version: 'unknown' }, + nnsp: { name: 'nnsp', version: 'unknown' }, + doq: { name: 'doq', version: 'unknown' }, 'sip/2': { name: 'sip', version: '2' }, 'tds/8.0': { name: 'tds', version: '8.0' }, - dicom: { name: 'dicom', version: 'none' }, + dicom: { name: 'dicom', version: 'unknown' }, }; const protocols = Object.keys(nextHopToNetworkVersion); From ee069949a14a8880d4ce8a0b12a57b56d0e70972 Mon Sep 17 00:00:00 2001 From: k-fish Date: Mon, 17 Jul 2023 09:44:10 -0400 Subject: [PATCH 7/9] Add more complete protocol splitting --- .../tracing-internal/src/browser/request.ts | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index 9c437e0f27b2..7942df7f459c 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -189,8 +189,23 @@ function addHTTPTimings(span: Span): void { * @param nextHopProtocol PerformanceResourceTiming.nextHopProtocol */ export function extractNetworkProtocol(nextHopProtocol: string): { name: string; version: string } { - const name = nextHopProtocol.split('/')[0].toLowerCase() || (nextHopProtocol.startsWith('h') && 'http') || 'unknown'; - const version = nextHopProtocol.split('/')[1] || nextHopProtocol.split('h')[1] || 'unknown'; + let name = 'unknown'; + let version = 'unknown'; + let _name = ''; + for (const char of nextHopProtocol) { + // http/1.1 etc. + if (char === '/') { + [name, version] = nextHopProtocol.split('/'); + break; + } + // h2, h3 etc. + if (!isNaN(Number(char))) { + name = _name === 'h' ? 'http' : _name; + version = nextHopProtocol.split(_name)[1]; + break; + } + _name += char; + } return { name, version }; } From 3b27b6f41f9ae15a73ee742d7f0b88fba89cb773 Mon Sep 17 00:00:00 2001 From: k-fish Date: Mon, 17 Jul 2023 09:50:30 -0400 Subject: [PATCH 8/9] Fix for versionless protocols --- packages/tracing-internal/src/browser/request.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index 7942df7f459c..5294b06c98bb 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -206,6 +206,10 @@ export function extractNetworkProtocol(nextHopProtocol: string): { name: string; } _name += char; } + if (_name === nextHopProtocol) { + // webrtc, ftp, etc. + name = _name; + } return { name, version }; } From 1ee7589db85d2aaf848bed834f0116bba20a0d53 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 17 Jul 2023 13:00:14 -0400 Subject: [PATCH 9/9] skip flaky remix test --- .../remix/test/integration/test/client/pageload.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/remix/test/integration/test/client/pageload.test.ts b/packages/remix/test/integration/test/client/pageload.test.ts index 7c49e4ac9c8c..59a8e331668e 100644 --- a/packages/remix/test/integration/test/client/pageload.test.ts +++ b/packages/remix/test/integration/test/client/pageload.test.ts @@ -4,7 +4,12 @@ import { getFirstSentryEnvelopeRequest } from './utils/helpers'; import { test, expect } from '@playwright/test'; import { Event } from '@sentry/types'; -test('should add `pageload` transaction on load.', async ({ page }) => { +test('should add `pageload` transaction on load.', async ({ page, browserName }) => { + // This test is flaky on firefox + if (browserName === 'firefox') { + test.skip(); + } + const envelope = await getFirstSentryEnvelopeRequest(page, '/'); expect(envelope.contexts?.trace.op).toBe('pageload');