Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ test('Should trace outgoing fetch requests inside middleware and create breadcru
'sentry.origin': 'auto.http.node_fetch',
'server.address': 'localhost',
'server.port': 3030,
'url.domain': 'localhost',
'url.full': 'http://localhost:3030/',
'url.path': '/',
'url.scheme': 'http',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,12 @@ test('Sends streamed spans for an errored route', async ({ baseURL }) => {

test('Outgoing fetch spans are streamed', async ({ baseURL }) => {
const fetchSpanPromise = waitForStreamedSpan('node-express-streaming', span => {
return getSpanOp(span) === 'http.client' && !span.is_segment && span.name.includes('localhost:3030/test-success');
// Streamed `http.client` names are the request method alone, so match on `url.full` instead.
return (
getSpanOp(span) === 'http.client' &&
!span.is_segment &&
String(span.attributes['url.full']?.value ?? '').includes('localhost:3030/test-success')
);
});

await fetch(`${baseURL}/test-outgoing-fetch`);
Expand All @@ -96,7 +101,12 @@ test.skip('Outgoing fetch spans include response headers when headersToSpanAttri
baseURL,
}) => {
const fetchSpanPromise = waitForStreamedSpan('node-express-streaming', span => {
return getSpanOp(span) === 'http.client' && !span.is_segment && span.name.includes('localhost:3030/test-success');
// Streamed `http.client` names are the request method alone, so match on `url.full` instead.
return (
getSpanOp(span) === 'http.client' &&
!span.is_segment &&
String(span.attributes['url.full']?.value ?? '').includes('localhost:3030/test-success')
);
});

await fetch(`${baseURL}/test-outgoing-fetch`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ describe('http.client span with streaming enabled', () => {
);

expect(httpClientSpan).toBeDefined();
expect(httpClientSpan?.name).toMatch(/^GET .*\/external$/);
// The URL path is high cardinality, so a streamed span name keeps only the domain.
expect(httpClientSpan?.name).toBe('GET localhost');
},
})
.start();
Expand Down
23 changes: 15 additions & 8 deletions packages/core/src/integrations/http/get-outgoing-span-data.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import type { Span, SpanAttributes } from '../../types/span';
import { getClient } from '../../currentScopes';
import { SEMANTIC_ATTRIBUTE_SENTRY_OP } from '../../semanticAttributes';
import { hasSpanStreamingEnabled } from '../../tracing/spans/hasSpanStreamingEnabled';
import { HTTP_SPAN_NAME_FALLBACK } from '../../tracing/spans/spanNames';
import { filterCollectedUrl } from '../../utils/data-collection/filterCollectedUrl';
import { getContentLengthFromHeaders } from '../../utils/request';
import { getHttpSpanDetailsFromUrlObject, parseStringToURLObject } from '../../utils/url';
import { getHttpSpanDetailsFromUrlObject, isURLObjectRelative, parseStringToURLObject } from '../../utils/url';
import type { HttpClientRequest, HttpIncomingMessage } from './types';
import { getRequestUrlFromClientRequest } from './get-request-url';
import type { StartSpanOptions } from '../../types/startSpanOptions';
Expand All @@ -29,17 +32,21 @@ import {
*/
export function getOutgoingRequestSpanData(request: HttpClientRequest): StartSpanOptions {
const url = getRequestUrlFromClientRequest(request);
const [name, attributes] = getHttpSpanDetailsFromUrlObject(
parseStringToURLObject(url),
'client',
'auto.http.client',
request,
);
const urlObject = parseStringToURLObject(url);
const [name, attributes] = getHttpSpanDetailsFromUrlObject(urlObject, 'client', 'auto.http.client', request);

const userAgent = request.getHeader('user-agent');

// With span streaming, span names have to be low cardinality, so the URL path is dropped and only the
// domain is kept. Outgoing requests have no route to parameterize.
const client = getClient();
const method = request.method?.toUpperCase();
const domain = urlObject && !isURLObjectRelative(urlObject) ? urlObject.hostname : undefined;
const streamedName = method ? (domain ? `${method} ${domain}` : method) : HTTP_SPAN_NAME_FALLBACK;
const spanName = !!client && hasSpanStreamingEnabled(client) ? streamedName : name;

return {
name,
name: spanName,
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.client',
[SENTRY_KIND]: 'client',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { describe, expect, it, vi } from 'vitest';
import { afterEach, describe, expect, it, vi } from 'vitest';
import type { Client } from '../../../../src/client';
import * as currentScopes from '../../../../src/currentScopes';
import {
getOutgoingRequestSpanData,
setIncomingResponseSpanData,
Expand Down Expand Up @@ -76,6 +78,37 @@ describe('getOutgoingRequestSpanData', () => {
expect(result.name).toMatch(/^POST /);
});

describe('with span streaming enabled', () => {
afterEach(() => {
vi.restoreAllMocks();
});

function mockStreamingClient(): void {
vi.spyOn(currentScopes, 'getClient').mockReturnValue({
getOptions: () => ({ traceLifecycle: 'stream' }),
getDataCollectionOptions: () => ({ urlQueryParams: true }),
} as unknown as Client);
}

it('drops the URL path but keeps the domain', () => {
mockStreamingClient();
const result = getOutgoingRequestSpanData(makeMockRequest({ method: 'post' }));
expect(result.name).toBe('POST example.com');
});

it('falls back to `HTTP` when the request has no method', () => {
mockStreamingClient();
const result = getOutgoingRequestSpanData(makeMockRequest({ method: undefined }));
expect(result.name).toBe('HTTP');
});

it('still records the URL on `url.full`', () => {
mockStreamingClient();
const result = getOutgoingRequestSpanData(makeMockRequest());
expect(result.attributes![URL_FULL]).toBe('http://example.com/api/test');
});
});

it('includes URL_FULL, HTTP_REQUEST_METHOD, URL_PATH, and server endpoint attributes', () => {
const result = getOutgoingRequestSpanData(makeMockRequest());
expect(result.attributes).toMatchObject({
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import type * as common from '@google-cloud/common';
import { HTTP_REQUEST_METHOD, SENTRY_OP, SERVER_ADDRESS, URL_FULL } from '@sentry/conventions/attributes';
import { HTTP_REQUEST_METHOD, SENTRY_OP, SERVER_ADDRESS, URL_DOMAIN, URL_FULL } from '@sentry/conventions/attributes';
import { HTTP_CLIENT } from '@sentry/conventions/op';
import type { Client, IntegrationFn } from '@sentry/core';
import {
defineIntegration,
fill,
getClient,
hasSpanStreamingEnabled,
isURLObjectRelative,
parseStringToURLObject,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
Expand Down Expand Up @@ -56,16 +57,25 @@ export const googleCloudHttpIntegration = defineIntegration(_googleCloudHttpInte
function wrapRequestFunction(orig: RequestFunction): RequestFunction {
return function (this: common.Service, reqOpts: RequestOptions, callback: ResponseCallback): void {
const httpMethod = reqOpts.method || 'GET';
const span = SETUP_CLIENTS.has(getClient() as Client)
const client = getClient();
const serverAddress = getServerAddress(this.apiEndpoint);
// Span names must not contain a query string, and callers can pass any URI they want. With span
// streaming they have to be low cardinality on top of that, so the URI is dropped entirely and only
// the API endpoint is kept — `reqOpts.uri` is a path with no route to parameterize.
const streamedName = serverAddress ? `${httpMethod} ${serverAddress}` : httpMethod;
const span = SETUP_CLIENTS.has(client as Client)
? startInactiveSpan({
// Span names must not contain a query string, and callers can pass any URI they want.
name: `${httpMethod} ${stripUrlQueryAndFragment(reqOpts.uri)}`,
name:
!!client && hasSpanStreamingEnabled(client)
? streamedName
: `${httpMethod} ${stripUrlQueryAndFragment(reqOpts.uri)}`,
onlyIfParent: true,
attributes: {
[SENTRY_OP]: HTTP_CLIENT,
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.serverless',
[HTTP_REQUEST_METHOD]: httpMethod,
[SERVER_ADDRESS]: getServerAddress(this.apiEndpoint),
[SERVER_ADDRESS]: serverAddress,
[URL_DOMAIN]: serverAddress,
Comment thread
chargome marked this conversation as resolved.
[URL_FULL]: filterCollectedUrl(reqOpts.uri),
},
})
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { BigQuery } from '@google-cloud/bigquery';
import { HTTP_REQUEST_METHOD, SENTRY_OP, SERVER_ADDRESS, URL_FULL } from '@sentry/conventions/attributes';
import { HTTP_REQUEST_METHOD, SENTRY_OP, SERVER_ADDRESS, URL_DOMAIN, URL_FULL } from '@sentry/conventions/attributes';
import { HTTP_CLIENT } from '@sentry/conventions/op';
import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core';
import { createTransport, NodeClient, setCurrentClient } from '@sentry/node';
Expand Down Expand Up @@ -34,8 +34,19 @@ describe('GoogleCloudHttp tracing', () => {
stackParser: () => [],
});

// `traceLifecycle` defaults to `'stream'`, so `mockClient` exercises the low-cardinality names.
const staticClient = new NodeClient({
tracesSampleRate: 1.0,
integrations: [],
traceLifecycle: 'static',
dsn: 'https://withAWSServices@domain/123',
transport: () => createTransport({ recordDroppedEvent: () => undefined }, _ => Promise.resolve({})),
stackParser: () => [],
});

const integration = googleCloudHttpIntegration();
mockClient.addIntegration(integration);
staticClient.addIntegration(googleCloudHttpIntegration());

beforeEach(() => {
nock('https://www.googleapis.com')
Expand Down Expand Up @@ -78,32 +89,32 @@ describe('GoogleCloudHttp tracing', () => {
const resp = await bigquery.query('SELECT true AS foo');
expect(resp).toEqual([[{ foo: true }]]);
expect(mockStartInactiveSpan).toBeCalledWith({
name: 'POST /jobs',
name: 'POST bigquery.googleapis.com',
onlyIfParent: true,
attributes: {
[SENTRY_OP]: HTTP_CLIENT,
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.serverless',
[HTTP_REQUEST_METHOD]: 'POST',
[SERVER_ADDRESS]: 'bigquery.googleapis.com',
[URL_DOMAIN]: 'bigquery.googleapis.com',
[URL_FULL]: '/jobs',
},
});
expect(mockStartInactiveSpan).toBeCalledWith({
name: expect.stringMatching(/^GET \/queries\/.+/),
name: 'GET bigquery.googleapis.com',
onlyIfParent: true,
attributes: {
[SENTRY_OP]: HTTP_CLIENT,
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.serverless',
[HTTP_REQUEST_METHOD]: 'GET',
[SERVER_ADDRESS]: 'bigquery.googleapis.com',
[URL_DOMAIN]: 'bigquery.googleapis.com',
[URL_FULL]: expect.stringMatching(/^\/queries\/.+/),
},
});
});

// Span names follow `METHOD scheme://host/path`, so a query string must never reach the name,
// whatever the caller passes as `uri`.
test('strips the query string from the span name', async () => {
async function requestDatasetsWithQueryString(): Promise<void> {
nock('https://bigquery.googleapis.com')
.get('/bigquery/v2/projects/project-id/datasets')
.query(true)
Expand All @@ -115,6 +126,21 @@ describe('GoogleCloudHttp tracing', () => {
(err: unknown) => (err ? reject(err) : resolve()),
);
});
}

// With span streaming the URI does not reach the name at all, so neither can the query string.
test('names the span after the method and the API endpoint', async () => {
await requestDatasetsWithQueryString();

expect(mockStartInactiveSpan).toBeCalledWith(expect.objectContaining({ name: 'GET bigquery.googleapis.com' }));
});

// Span names follow `METHOD scheme://host/path`, so a query string must never reach the name,
// whatever the caller passes as `uri`.
test('strips the query string from the span name with `traceLifecycle: "static"`', async () => {
setCurrentClient(staticClient);

await requestDatasetsWithQueryString();

expect(mockStartInactiveSpan).toBeCalledWith(expect.objectContaining({ name: 'GET /datasets' }));
const names = mockStartInactiveSpan.mock.calls.map(([args]) => (args as { name: string }).name);
Expand Down
16 changes: 14 additions & 2 deletions packages/node/src/integrations/http/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,14 @@
import type { ClientRequest, RequestOptions } from 'node:http';
import type { Span } from '@sentry/core';
import { URL_FULL } from '@sentry/conventions/attributes';
import { defineIntegration, getRequestUrlFromClientRequest, hasSpansEnabled, stripDataUrlContent } from '@sentry/core';
import {
defineIntegration,
getClient,
getRequestUrlFromClientRequest,
hasSpansEnabled,
hasSpanStreamingEnabled,
stripDataUrlContent,
} from '@sentry/core';
import type { NodeClient } from '../../sdk/client';
import type { HttpServerIntegrationOptions } from './httpServerIntegration';
import { httpServerIntegration } from './httpServerIntegration';
Expand Down Expand Up @@ -109,7 +116,12 @@ export const httpIntegration = defineIntegration((options: HttpOptions = {}) =>
const url = getRequestUrlFromClientRequest(request);
if (url.startsWith('data:')) {
const sanitizedUrl = stripDataUrlContent(url);
span.updateName(`${request.method || 'GET'} ${sanitizedUrl}`);
// With span streaming the span already carries a low-cardinality name, so it must not be
// renamed back to something containing the URL.
const client = getClient();
if (!client || !hasSpanStreamingEnabled(client)) {
span.updateName(`${request.method || 'GET'} ${sanitizedUrl}`);
}
span.setAttributes({
[URL_FULL]: sanitizedUrl,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
getSanitizedUrlString,
getSpanStatusFromHttpCode,
hasSpanStreamingEnabled,
HTTP_SPAN_NAME_FALLBACK,
isTracingSuppressed,
LRUMap,
parseUrl,
Expand All @@ -49,6 +50,7 @@ import {
SENTRY_OP,
SERVER_ADDRESS,
SERVER_PORT,
URL_DOMAIN,
URL_FRAGMENT,
URL_FULL,
URL_PATH,
Expand Down Expand Up @@ -220,6 +222,7 @@ function onRequestCreated(config: NodeFetchOptions, { request }: RequestMessage)
[HTTP_REQUEST_METHOD]: requestMethod,
[ATTR_HTTP_REQUEST_METHOD_ORIGINAL]: request.method,
[URL_FULL]: filterCollectedUrl(requestUrl.toString()),
[URL_DOMAIN]: requestUrl.hostname || undefined,
[URL_PATH]: requestUrl.pathname,
[URL_QUERY]: filterCollectedUrlQuery(getUrlQuery(requestUrl.search)),
[URL_FRAGMENT]: getUrlFragment(requestUrl.hash),
Comment on lines 222 to 228

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The streaming path for undici/fetch does not strip the internal SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME attribute for data: URLs, leading to a potential high-cardinality transaction name in Sentry.
Severity: LOW

Suggested Fix

Before serializing the span for streaming, delete the SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME attribute from the span's attributes. This would mirror the cleanup logic already present in the static (non-streaming) path's SentrySpan._convertSpanToTransaction() function.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: packages/node/src/integrations/node-fetch/undici-instrumentation.ts#L222-L228

Potential issue: When span streaming is enabled for `undici`/`fetch` and a `data:` URL
is used, the internal attribute `SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME` is not
removed from the streamed span's attributes. The static (non-streaming) path correctly
deletes this attribute before sending the transaction, but the streaming path does not.
This inconsistency causes the attribute to be leaked, which can result in the Sentry
backend using a high-cardinality name for the transaction instead of the intended
low-cardinality one.

Did we get this right? 👍 / 👎 to inform future reviews.

Expand Down Expand Up @@ -262,14 +265,21 @@ function onRequestCreated(config: NodeFetchOptions, { request }: RequestMessage)
// when an OpenTelemetry SDK tracer provider is set up, so we enforce it here too, which covers
// SDKs that don't use an OpenTelemetry tracer provider at all.
const isDataUrl = url.startsWith('data:');
const spanName =
requestMethod === '_OTHER'
? 'HTTP'
: isDataUrl
? `${request.method || 'GET'} ${stripDataUrlContent(url)}`
: `${requestMethod} ${getSanitizedUrlString(parseUrl(requestUrl.toString()))}`;

const client = getClient();

let spanName: string;
if (requestMethod === '_OTHER') {
spanName = HTTP_SPAN_NAME_FALLBACK;
} else if (!!client && hasSpanStreamingEnabled(client)) {
// With span streaming, span names have to be low cardinality, so the URL path is dropped and only
// the domain is kept. Outgoing requests have no route to parameterize, and data URLs have no domain.
spanName = requestUrl.hostname ? `${requestMethod} ${requestUrl.hostname}` : requestMethod;
} else if (isDataUrl) {
spanName = `${request.method || 'GET'} ${stripDataUrlContent(url)}`;
} else {
spanName = `${requestMethod} ${getSanitizedUrlString(parseUrl(requestUrl.toString()))}`;
}

const span = startInactiveSpan({
name: spanName,
attributes,
Expand Down
Loading
Loading