-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(core): Add server.address
to browser http.client
spans
#11634
Changes from all commits
e86eeac
c4a5142
b8187ea
77effb7
c93409c
4f86bf2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -208,7 +208,7 @@ module.exports = [ | |
'tls', | ||
], | ||
gzip: true, | ||
limit: '150 KB', | ||
limit: '160 KB', | ||
}, | ||
]; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import * as Sentry from '@sentry/browser'; | ||
|
||
window.Sentry = Sentry; | ||
|
||
Sentry.init({ | ||
dsn: 'https://public@dsn.ingest.sentry.io/1337', | ||
integrations: [Sentry.browserTracingIntegration()], | ||
tracesSampleRate: 1, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
fetch('/test-req/0').then( | ||
fetch('/test-req/1', { headers: { 'X-Test-Header': 'existing-header' } }).then(fetch('/test-req/2')), | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
import { expect } from '@playwright/test'; | ||
|
||
import { TEST_HOST, sentryTest } from '../../../../utils/fixtures'; | ||
import { | ||
envelopeRequestParser, | ||
shouldSkipTracingTest, | ||
waitForTransactionRequestOnUrl, | ||
} from '../../../../utils/helpers'; | ||
|
||
sentryTest('should create spans for fetch requests', async ({ getLocalTestUrl, page }) => { | ||
if (shouldSkipTracingTest()) { | ||
sentryTest.skip(); | ||
} | ||
|
||
const url = await getLocalTestUrl({ testDir: __dirname }); | ||
const req = await waitForTransactionRequestOnUrl(page, url); | ||
const tracingEvent = envelopeRequestParser(req); | ||
|
||
const requestSpans = tracingEvent.spans?.filter(({ op }) => op === 'http.client'); | ||
|
||
expect(requestSpans).toHaveLength(3); | ||
|
||
requestSpans?.forEach((span, index) => | ||
expect(span).toMatchObject({ | ||
description: `GET /test-req/${index}`, | ||
parent_span_id: tracingEvent.contexts?.trace?.span_id, | ||
span_id: expect.any(String), | ||
start_timestamp: expect.any(Number), | ||
timestamp: expect.any(Number), | ||
trace_id: tracingEvent.contexts?.trace?.trace_id, | ||
data: { | ||
'http.method': 'GET', | ||
'http.url': `${TEST_HOST}/test-req/${index}`, | ||
url: `/test-req/${index}`, | ||
'server.address': 'sentry-test.io', | ||
type: 'fetch', | ||
}, | ||
}), | ||
); | ||
}); | ||
|
||
sentryTest('should attach `sentry-trace` header to fetch requests', async ({ getLocalTestUrl, page }) => { | ||
if (shouldSkipTracingTest()) { | ||
sentryTest.skip(); | ||
} | ||
|
||
const url = await getLocalTestUrl({ testDir: __dirname }); | ||
|
||
const requests = ( | ||
await Promise.all([ | ||
page.goto(url), | ||
Promise.all([0, 1, 2].map(idx => page.waitForRequest(`${TEST_HOST}/test-req/${idx}`))), | ||
]) | ||
)[1]; | ||
|
||
expect(requests).toHaveLength(3); | ||
|
||
const request1 = requests[0]; | ||
const requestHeaders1 = request1.headers(); | ||
expect(requestHeaders1).toMatchObject({ | ||
'sentry-trace': expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/), | ||
baggage: expect.any(String), | ||
}); | ||
|
||
const request2 = requests[1]; | ||
const requestHeaders2 = request2.headers(); | ||
expect(requestHeaders2).toMatchObject({ | ||
'sentry-trace': expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/), | ||
baggage: expect.any(String), | ||
'x-test-header': 'existing-header', | ||
}); | ||
|
||
const request3 = requests[2]; | ||
const requestHeaders3 = request3.headers(); | ||
expect(requestHeaders3).toMatchObject({ | ||
'sentry-trace': expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/), | ||
baggage: expect.any(String), | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import * as Sentry from '@sentry/browser'; | ||
|
||
window.Sentry = Sentry; | ||
|
||
Sentry.init({ | ||
dsn: 'https://public@dsn.ingest.sentry.io/1337', | ||
integrations: [Sentry.browserTracingIntegration()], | ||
tracesSampleRate: 1, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
const xhr_1 = new XMLHttpRequest(); | ||
xhr_1.open('GET', '/test-req/0'); | ||
xhr_1.send(); | ||
|
||
const xhr_2 = new XMLHttpRequest(); | ||
xhr_2.open('GET', '/test-req/1'); | ||
xhr_2.setRequestHeader('X-Test-Header', 'existing-header'); | ||
xhr_2.send(); | ||
|
||
const xhr_3 = new XMLHttpRequest(); | ||
xhr_3.open('GET', '/test-req/2'); | ||
xhr_3.send(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
import { expect } from '@playwright/test'; | ||
|
||
import { TEST_HOST, sentryTest } from '../../../../utils/fixtures'; | ||
import { | ||
envelopeRequestParser, | ||
shouldSkipTracingTest, | ||
waitForTransactionRequestOnUrl, | ||
} from '../../../../utils/helpers'; | ||
|
||
sentryTest('should create spans for xhr requests', async ({ getLocalTestUrl, page }) => { | ||
if (shouldSkipTracingTest()) { | ||
sentryTest.skip(); | ||
} | ||
|
||
const url = await getLocalTestUrl({ testDir: __dirname }); | ||
const req = await waitForTransactionRequestOnUrl(page, url); | ||
const tracingEvent = envelopeRequestParser(req); | ||
|
||
const requestSpans = tracingEvent.spans?.filter(({ op }) => op === 'http.client'); | ||
|
||
expect(requestSpans).toHaveLength(3); | ||
|
||
requestSpans?.forEach((span, index) => | ||
expect(span).toMatchObject({ | ||
description: `GET /test-req/${index}`, | ||
parent_span_id: tracingEvent.contexts?.trace?.span_id, | ||
span_id: expect.any(String), | ||
start_timestamp: expect.any(Number), | ||
timestamp: expect.any(Number), | ||
trace_id: tracingEvent.contexts?.trace?.trace_id, | ||
data: { | ||
'http.method': 'GET', | ||
'http.url': `${TEST_HOST}/test-req/${index}`, | ||
url: `/test-req/${index}`, | ||
'server.address': 'sentry-test.io', | ||
type: 'xhr', | ||
}, | ||
}), | ||
); | ||
}); | ||
|
||
sentryTest('should attach `sentry-trace` header to xhr requests', async ({ getLocalTestUrl, page }) => { | ||
if (shouldSkipTracingTest()) { | ||
sentryTest.skip(); | ||
} | ||
|
||
const url = await getLocalTestUrl({ testDir: __dirname }); | ||
|
||
const requests = ( | ||
await Promise.all([ | ||
page.goto(url), | ||
Promise.all([0, 1, 2].map(idx => page.waitForRequest(`${TEST_HOST}/test-req/${idx}`))), | ||
]) | ||
)[1]; | ||
|
||
expect(requests).toHaveLength(3); | ||
|
||
const request1 = requests[0]; | ||
const requestHeaders1 = request1.headers(); | ||
expect(requestHeaders1).toMatchObject({ | ||
'sentry-trace': expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/), | ||
baggage: expect.any(String), | ||
}); | ||
|
||
const request2 = requests[1]; | ||
const requestHeaders2 = request2.headers(); | ||
expect(requestHeaders2).toMatchObject({ | ||
'sentry-trace': expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/), | ||
baggage: expect.any(String), | ||
'x-test-header': 'existing-header', | ||
}); | ||
|
||
const request3 = requests[2]; | ||
const requestHeaders3 = request3.headers(); | ||
expect(requestHeaders3).toMatchObject({ | ||
'sentry-trace': expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/), | ||
baggage: expect.any(String), | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ import { | |
addXhrInstrumentationHandler, | ||
} from '@sentry-internal/browser-utils'; | ||
import { | ||
SEMANTIC_ATTRIBUTE_SENTRY_OP, | ||
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, | ||
SentryNonRecordingSpan, | ||
getActiveSpan, | ||
|
@@ -26,6 +27,7 @@ import { | |
browserPerformanceTimeOrigin, | ||
dynamicSamplingContextToSentryBaggageHeader, | ||
generateSentryTraceHeader, | ||
parseUrl, | ||
stringMatchesSomePattern, | ||
} from '@sentry/utils'; | ||
import { WINDOW } from '../helpers'; | ||
|
@@ -115,6 +117,18 @@ export function instrumentOutgoingRequests(_options?: Partial<RequestInstrumenta | |
if (traceFetch) { | ||
addFetchInstrumentationHandler(handlerData => { | ||
const createdSpan = instrumentFetchRequest(handlerData, shouldCreateSpan, shouldAttachHeadersWithTargets, spans); | ||
// We cannot use `window.location` in the generic fetch instrumentation, | ||
// but we need it for reliable `server.address` attribute. | ||
// so we extend this in here | ||
if (createdSpan) { | ||
const fullUrl = getFullURL(handlerData.fetchData.url); | ||
const host = fullUrl ? parseUrl(fullUrl).host : undefined; | ||
createdSpan.setAttributes({ | ||
'http.url': fullUrl, | ||
'server.address': host, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A small ask, but could you add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sadly it's not so easy to get the port, as most URLs don't have it and we just don't really know. We could "guess" it based on the protocol but this seems tricky to me? Anyhow, we can track this in a separate issue if we need this, I'd say! |
||
}); | ||
} | ||
|
||
if (enableHTTPTimings && createdSpan) { | ||
addHTTPTimings(createdSpan); | ||
} | ||
|
@@ -310,17 +324,22 @@ export function xhrCallback( | |
|
||
const hasParent = !!getActiveSpan(); | ||
|
||
const fullUrl = getFullURL(sentryXhrData.url); | ||
const host = fullUrl ? parseUrl(fullUrl).host : undefined; | ||
|
||
const span = | ||
shouldCreateSpanResult && hasParent | ||
? startInactiveSpan({ | ||
name: `${sentryXhrData.method} ${sentryXhrData.url}`, | ||
attributes: { | ||
type: 'xhr', | ||
'http.method': sentryXhrData.method, | ||
'http.url': fullUrl, | ||
url: sentryXhrData.url, | ||
'server.address': host, | ||
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.browser', | ||
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.client', | ||
}, | ||
op: 'http.client', | ||
gggritso marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}) | ||
: new SentryNonRecordingSpan(); | ||
|
||
|
@@ -381,3 +400,14 @@ function setHeaderOnXhr( | |
// Error: InvalidStateError: Failed to execute 'setRequestHeader' on 'XMLHttpRequest': The object's state must be OPENED. | ||
} | ||
} | ||
|
||
function getFullURL(url: string): string | undefined { | ||
try { | ||
// By adding a base URL to new URL(), this will also work for relative urls | ||
// If `url` is a full URL, the base URL is ignored anyhow | ||
const parsed = new URL(url, WINDOW.location.origin); | ||
return parsed.href; | ||
} catch { | ||
return undefined; | ||
} | ||
} |
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.
@gggritso regarding port, you can see here that the port is part of the server.address if it is in the URL (=non-standard)
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.
Roger, thanks!