-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(core): Apply request data to segment spans in span streaming #20654
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
Changes from all commits
3ec6240
4c78699
862e0b2
fd48621
c40c729
0620a89
ad85ed6
70496a8
5a4788e
0cab628
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 |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| import * as Sentry from '@sentry/node'; | ||
| import { loggingTransport } from '@sentry-internal/node-integration-tests'; | ||
|
|
||
| Sentry.init({ | ||
| dsn: 'https://public@dsn.ingest.sentry.io/1337', | ||
| release: '1.0', | ||
| tracesSampleRate: 1.0, | ||
| transport: loggingTransport, | ||
| traceLifecycle: 'stream', | ||
| sendDefaultPii: true, | ||
| integrations: defaults => defaults.filter(i => i.name !== 'RequestData'), | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| import * as Sentry from '@sentry/node'; | ||
| import { loggingTransport } from '@sentry-internal/node-integration-tests'; | ||
|
|
||
| Sentry.init({ | ||
| dsn: 'https://public@dsn.ingest.sentry.io/1337', | ||
| release: '1.0', | ||
| tracesSampleRate: 1.0, | ||
| transport: loggingTransport, | ||
| traceLifecycle: 'stream', | ||
| sendDefaultPii: true, | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| import * as Sentry from '@sentry/node'; | ||
| import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests'; | ||
| import express from 'express'; | ||
|
|
||
| const app = express(); | ||
|
|
||
| app.get('/test', (_req, res) => { | ||
| res.send({ response: 'ok' }); | ||
| }); | ||
|
|
||
| Sentry.setupExpressErrorHandler(app); | ||
|
|
||
| startExpressServerAndSendPortToRunner(app); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| import { afterAll, describe, expect } from 'vitest'; | ||
| import { cleanupChildProcesses, createEsmAndCjsTests } from '../../../utils/runner'; | ||
|
|
||
| describe('requestData-streamed', () => { | ||
| afterAll(() => { | ||
| cleanupChildProcesses(); | ||
| }); | ||
|
|
||
| createEsmAndCjsTests(__dirname, 'server.mjs', 'instrument.mjs', (createRunner, test) => { | ||
| test('applies request data attributes to the segment span', async () => { | ||
| const runner = createRunner() | ||
| .expect({ | ||
| span: container => { | ||
| const serverSpan = container.items.find(item => item.is_segment); | ||
|
|
||
| expect(serverSpan).toBeDefined(); | ||
|
|
||
| expect(serverSpan?.attributes?.['url.full']).toEqual({ | ||
| type: 'string', | ||
| value: expect.stringContaining('/test?foo=bar'), | ||
| }); | ||
|
|
||
| expect(serverSpan?.attributes?.['http.request.method']).toEqual({ | ||
| type: 'string', | ||
| value: 'GET', | ||
| }); | ||
|
|
||
| expect(serverSpan?.attributes?.['url.query']).toEqual({ | ||
| type: 'string', | ||
| value: 'foo=bar', | ||
| }); | ||
|
|
||
| expect(serverSpan?.attributes?.['http.request.header.host']).toEqual({ | ||
| type: 'string', | ||
| value: expect.any(String), | ||
| }); | ||
|
|
||
| expect(serverSpan?.attributes?.['user.ip_address']).toEqual({ | ||
| type: 'string', | ||
| value: expect.any(String), | ||
| }); | ||
| }, | ||
| }) | ||
| .start(); | ||
|
|
||
| await runner.makeRequest('get', '/test?foo=bar'); | ||
|
|
||
| await runner.completed(); | ||
| }); | ||
| }); | ||
|
|
||
| createEsmAndCjsTests(__dirname, 'server.mjs', 'instrument-without-request-data.mjs', (createRunner, test) => { | ||
| test('does not apply request data attributes when requestDataIntegration is removed', async () => { | ||
| const runner = createRunner() | ||
| .expect({ | ||
| span: container => { | ||
| const serverSpan = container.items.find(item => item.is_segment); | ||
|
|
||
| expect(serverSpan).toBeDefined(); | ||
|
|
||
| // url.query and user.ip_address are only set by applyScopeToSegmentSpan | ||
| // (not by OTel instrumentation), so they should be absent when the integration is removed | ||
| expect(serverSpan?.attributes?.['url.query']).toBeUndefined(); | ||
| expect(serverSpan?.attributes?.['user.ip_address']).toBeUndefined(); | ||
| }, | ||
| }) | ||
| .start(); | ||
|
|
||
| await runner.makeRequest('get', '/test?foo=bar'); | ||
|
|
||
| await runner.completed(); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,14 @@ | ||
| import { getIsolationScope } from '../currentScopes'; | ||
| import { defineIntegration } from '../integration'; | ||
| import { SEMANTIC_ATTRIBUTE_USER_IP_ADDRESS } from '../semanticAttributes'; | ||
| import type { Event } from '../types-hoist/event'; | ||
| import type { IntegrationFn } from '../types-hoist/integration'; | ||
| import type { RequestEventData } from '../types-hoist/request'; | ||
| import type { QueryParams, RequestEventData } from '../types-hoist/request'; | ||
| import type { StreamedSpanJSON } from '../types-hoist/span'; | ||
| import { parseCookie } from '../utils/cookie'; | ||
| import { httpHeadersToSpanAttributes } from '../utils/request'; | ||
| import { getClientIPAddress, ipHeaderNames } from '../vendor/getIpAddress'; | ||
| import { safeSetSpanJSONAttributes } from '../tracing/spans/captureSpan'; | ||
|
|
||
| interface RequestDataIncludeOptions { | ||
| cookies?: boolean; | ||
|
|
@@ -55,6 +60,22 @@ const _requestDataIntegration = ((options: RequestDataIntegrationOptions = {}) = | |
|
|
||
| return event; | ||
| }, | ||
| processSegmentSpan(span, client) { | ||
| const { sdkProcessingMetadata = {} } = getIsolationScope().getScopeData(); | ||
| const { normalizedRequest, ipAddress } = sdkProcessingMetadata; | ||
|
|
||
| if (!normalizedRequest) { | ||
| return; | ||
| } | ||
|
|
||
| const { sendDefaultPii } = client.getOptions(); | ||
| const includeWithDefaultPiiApplied: RequestDataIncludeOptions = { | ||
| ...include, | ||
| ip: include.ip ?? sendDefaultPii, | ||
| }; | ||
|
|
||
| addNormalizedRequestDataToSpan(span, normalizedRequest, ipAddress, includeWithDefaultPiiApplied, sendDefaultPii); | ||
| }, | ||
| }; | ||
| }) satisfies IntegrationFn; | ||
|
|
||
|
|
@@ -91,6 +112,60 @@ function addNormalizedRequestDataToEvent( | |
| } | ||
| } | ||
|
|
||
| function addNormalizedRequestDataToSpan( | ||
| span: StreamedSpanJSON, | ||
| normalizedRequest: RequestEventData, | ||
| ipAddress: string | undefined, | ||
| include: RequestDataIncludeOptions, | ||
| sendDefaultPii: boolean | undefined, | ||
| ): void { | ||
| const requestData = extractNormalizedRequestData(normalizedRequest, include); | ||
| const attributes: Record<string, unknown> = {}; | ||
|
|
||
| if (requestData.url) { | ||
| attributes['url.full'] = requestData.url; | ||
| } | ||
|
|
||
| if (requestData.method) { | ||
| attributes['http.request.method'] = requestData.method; | ||
| } | ||
|
|
||
| if (requestData.query_string) { | ||
| attributes['url.query'] = normalizeQueryString(requestData.query_string); | ||
| } | ||
|
|
||
| safeSetSpanJSONAttributes(span, attributes); | ||
|
|
||
| // Process cookies before headers so normalizedRequest.cookies takes precedence | ||
| // over the raw cookie header (matching the processEvent path). | ||
| if (requestData.cookies && Object.keys(requestData.cookies).length > 0) { | ||
| const cookieString = Object.entries(requestData.cookies) | ||
| .map(([name, value]) => `${name}=${value}`) | ||
| .join('; '); | ||
| const cookieAttributes = httpHeadersToSpanAttributes({ cookie: cookieString }, sendDefaultPii ?? false, 'request'); | ||
| safeSetSpanJSONAttributes(span, cookieAttributes); | ||
| } | ||
|
|
||
| if (requestData.headers) { | ||
| const headerAttributes = httpHeadersToSpanAttributes(requestData.headers, sendDefaultPii ?? false, 'request'); | ||
| safeSetSpanJSONAttributes(span, headerAttributes); | ||
| } | ||
|
|
||
| if (requestData.data != null) { | ||
| const serialized = typeof requestData.data === 'string' ? requestData.data : JSON.stringify(requestData.data); | ||
| if (serialized) { | ||
| safeSetSpanJSONAttributes(span, { 'http.request.body.data': serialized }); | ||
| } | ||
| } | ||
|
|
||
| if (include.ip) { | ||
| const ip = (normalizedRequest.headers && getClientIPAddress(normalizedRequest.headers)) || ipAddress || undefined; | ||
| if (ip) { | ||
| safeSetSpanJSONAttributes(span, { [SEMANTIC_ATTRIBUTE_USER_IP_ADDRESS]: ip }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function extractNormalizedRequestData( | ||
| normalizedRequest: RequestEventData, | ||
| include: RequestDataIncludeOptions, | ||
|
|
@@ -101,13 +176,10 @@ function extractNormalizedRequestData( | |
| if (include.headers) { | ||
| requestData.headers = headers; | ||
|
|
||
| // Remove the Cookie header in case cookie data should not be included in the event | ||
|
Member
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. l: why remove the comments here? |
||
| if (!include.cookies) { | ||
| delete (headers as { cookie?: string }).cookie; | ||
| } | ||
|
|
||
| // Remove IP headers in case IP data should not be included in the event. | ||
| // Match case-insensitively — same as getClientIPAddress — so lowercase keys are stripped too. | ||
| if (!include.ip) { | ||
| const ipHeaderNamesLower = new Set(ipHeaderNames.map(name => name.toLowerCase())); | ||
| for (const key of Object.keys(headers)) { | ||
|
|
@@ -140,3 +212,14 @@ function extractNormalizedRequestData( | |
|
|
||
| return requestData; | ||
| } | ||
|
|
||
| function normalizeQueryString(queryString: QueryParams): string | undefined { | ||
| if (typeof queryString === 'string') { | ||
| return queryString || undefined; | ||
| } | ||
|
|
||
| const pairs = Array.isArray(queryString) ? queryString : Object.entries(queryString); | ||
| const result = pairs.map(([key, value]) => `${key}=${value}`).join('&'); | ||
|
|
||
| return result || undefined; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,10 +97,27 @@ export function captureSpan(span: Span, client: Client): SerializedStreamedSpanW | |
| } | ||
|
|
||
| function applyScopeToSegmentSpan(_segmentSpanJSON: StreamedSpanJSON, _scopeData: ScopeData): void { | ||
| // TODO: Apply all scope and request data from auto instrumentation (contexts, request) to segment span | ||
| // TODO: Apply contexts data from auto instrumentation to segment span | ||
|
Member
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. l: are the changes here just leftovers or intentional?
Member
Author
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. leftover |
||
| // This will follow in a separate PR | ||
| } | ||
|
|
||
| /** | ||
| * Safely set attributes on a span JSON. | ||
| * If an attribute already exists, it will not be overwritten. | ||
| */ | ||
| export function safeSetSpanJSONAttributes( | ||
| spanJSON: StreamedSpanJSON, | ||
| newAttributes: RawAttributes<Record<string, unknown>>, | ||
| ): void { | ||
| const originalAttributes = spanJSON.attributes ?? (spanJSON.attributes = {}); | ||
|
|
||
| Object.entries(newAttributes).forEach(([key, value]) => { | ||
| if (value != null && !(key in originalAttributes)) { | ||
| originalAttributes[key] = value; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| function applyCommonSpanAttributes( | ||
| spanJSON: StreamedSpanJSON, | ||
| serializedSegmentSpan: StreamedSpanJSON, | ||
|
|
@@ -145,23 +162,6 @@ export function applyBeforeSendSpanCallback( | |
| return modifedSpan; | ||
| } | ||
|
|
||
| /** | ||
| * Safely set attributes on a span JSON. | ||
| * If an attribute already exists, it will not be overwritten. | ||
| */ | ||
| export function safeSetSpanJSONAttributes( | ||
| spanJSON: StreamedSpanJSON, | ||
| newAttributes: RawAttributes<Record<string, unknown>>, | ||
| ): void { | ||
| const originalAttributes = spanJSON.attributes ?? (spanJSON.attributes = {}); | ||
|
|
||
| Object.entries(newAttributes).forEach(([key, value]) => { | ||
| if (value != null && !(key in originalAttributes)) { | ||
| originalAttributes[key] = value; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| // OTel SpanKind values (numeric to avoid importing from @opentelemetry/api) | ||
| const SPAN_KIND_SERVER = 1; | ||
| const SPAN_KIND_CLIENT = 2; | ||
|
|
||
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.
l: any reason not to aggregate all attributes and then call
safeSetSpanJSONAttributesonce instead of splitting this up?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.
To set cookies before headers for precedence