From eaec0f58cff7eb3bc4514e70d51aaebf611a465d Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 14 Jul 2023 14:34:46 +0200 Subject: [PATCH 1/3] fix(otel): Use `HTTP_URL` for client requests Turns out, `HTTP_TARGET` is always the relative path, even for outgoing requests, which omits the host. So we handle this specifically now here. --- .../opentelemetry-node/src/spanprocessor.ts | 6 +- ...s-sentry-request.ts => isSentryRequest.ts} | 0 .../{map-otel-status.ts => mapOtelStatus.ts} | 0 ...ription.ts => parseOtelSpanDescription.ts} | 49 +++++++-- .../utils/parseOtelSpanDescription.test.ts | 99 +++++++++++++++++++ 5 files changed, 143 insertions(+), 11 deletions(-) rename packages/opentelemetry-node/src/utils/{is-sentry-request.ts => isSentryRequest.ts} (100%) rename packages/opentelemetry-node/src/utils/{map-otel-status.ts => mapOtelStatus.ts} (100%) rename packages/opentelemetry-node/src/utils/{parse-otel-span-description.ts => parseOtelSpanDescription.ts} (68%) create mode 100644 packages/opentelemetry-node/test/utils/parseOtelSpanDescription.test.ts diff --git a/packages/opentelemetry-node/src/spanprocessor.ts b/packages/opentelemetry-node/src/spanprocessor.ts index d6719fd682ad..ca5609597686 100644 --- a/packages/opentelemetry-node/src/spanprocessor.ts +++ b/packages/opentelemetry-node/src/spanprocessor.ts @@ -7,9 +7,9 @@ import type { DynamicSamplingContext, Span as SentrySpan, TraceparentData, Trans import { isString, logger } from '@sentry/utils'; import { SENTRY_DYNAMIC_SAMPLING_CONTEXT_KEY, SENTRY_TRACE_PARENT_CONTEXT_KEY } from './constants'; -import { isSentryRequestSpan } from './utils/is-sentry-request'; -import { mapOtelStatus } from './utils/map-otel-status'; -import { parseSpanDescription } from './utils/parse-otel-span-description'; +import { isSentryRequestSpan } from './utils/isSentryRequest'; +import { mapOtelStatus } from './utils/mapOtelStatus'; +import { parseSpanDescription } from './utils/parseOtelSpanDescription'; export const SENTRY_SPAN_PROCESSOR_MAP: Map = new Map< SentrySpan['spanId'], diff --git a/packages/opentelemetry-node/src/utils/is-sentry-request.ts b/packages/opentelemetry-node/src/utils/isSentryRequest.ts similarity index 100% rename from packages/opentelemetry-node/src/utils/is-sentry-request.ts rename to packages/opentelemetry-node/src/utils/isSentryRequest.ts diff --git a/packages/opentelemetry-node/src/utils/map-otel-status.ts b/packages/opentelemetry-node/src/utils/mapOtelStatus.ts similarity index 100% rename from packages/opentelemetry-node/src/utils/map-otel-status.ts rename to packages/opentelemetry-node/src/utils/mapOtelStatus.ts diff --git a/packages/opentelemetry-node/src/utils/parse-otel-span-description.ts b/packages/opentelemetry-node/src/utils/parseOtelSpanDescription.ts similarity index 68% rename from packages/opentelemetry-node/src/utils/parse-otel-span-description.ts rename to packages/opentelemetry-node/src/utils/parseOtelSpanDescription.ts index 396e37a29ced..7c6c3a4d4020 100644 --- a/packages/opentelemetry-node/src/utils/parse-otel-span-description.ts +++ b/packages/opentelemetry-node/src/utils/parseOtelSpanDescription.ts @@ -1,8 +1,9 @@ -import type { AttributeValue } from '@opentelemetry/api'; +import type { Attributes, AttributeValue } from '@opentelemetry/api'; import { SpanKind } from '@opentelemetry/api'; import type { Span as OtelSpan } from '@opentelemetry/sdk-trace-base'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; import type { TransactionSource } from '@sentry/types'; +import { getSanitizedUrlString, parseUrl } from '@sentry/utils'; interface SpanDescription { op: string | undefined; @@ -87,21 +88,53 @@ function descriptionForHttpMethod(otelSpan: OtelSpan, httpMethod: AttributeValue break; } - const httpTarget = attributes[SemanticAttributes.HTTP_TARGET]; const httpRoute = attributes[SemanticAttributes.HTTP_ROUTE]; + const url = getSanitizedUrl(attributes, kind); - // Ex. /api/users - const httpPath = httpRoute || httpTarget; - - if (!httpPath) { + if (!url) { return { op: opParts.join('.'), description: name, source: 'custom' }; } // Ex. description="GET /api/users". - const description = `${httpMethod} ${httpPath}`; + const description = `${httpMethod} ${url}`; // If `httpPath` is a root path, then we can categorize the transaction source as route. - const source: TransactionSource = httpRoute || httpPath === '/' ? 'route' : 'url'; + const source: TransactionSource = httpRoute || url === '/' ? 'route' : 'url'; return { op: opParts.join('.'), description, source }; } + +/** Exported for tests only */ +export function getSanitizedUrl(attributes: Attributes, kind: SpanKind): string | undefined { + // This is the relative path of the URL, e.g. /sub + const httpTarget = attributes[SemanticAttributes.HTTP_TARGET]; + // This is the full URL, including host & query params etc., e.g. https://example.com/sub?foo=bar + const httpUrl = attributes[SemanticAttributes.HTTP_URL]; + // This is the normalized route name - may not always be available! + const httpRoute = attributes[SemanticAttributes.HTTP_ROUTE]; + + if (typeof httpRoute === 'string') { + return httpRoute; + } + + if (kind === SpanKind.SERVER && typeof httpTarget === 'string') { + return normalizeTarget(httpTarget); + } + + if (typeof httpUrl === 'string') { + const url = parseUrl(httpUrl); + return getSanitizedUrlString(url); + } + + // fall back to target even for client spans, if no URL is present + if (typeof httpTarget === 'string') { + return normalizeTarget(httpTarget); + } + + return undefined; +} + +// Remove trailing ? and # from the target +function normalizeTarget(httpTarget: string): string { + return httpTarget.replace(/([?#].*)$/, ''); +} diff --git a/packages/opentelemetry-node/test/utils/parseOtelSpanDescription.test.ts b/packages/opentelemetry-node/test/utils/parseOtelSpanDescription.test.ts new file mode 100644 index 000000000000..7b02dad6ccdb --- /dev/null +++ b/packages/opentelemetry-node/test/utils/parseOtelSpanDescription.test.ts @@ -0,0 +1,99 @@ +import { SpanKind } from '@opentelemetry/api'; +import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; + +import { getSanitizedUrl } from '../../src/utils/parseOtelSpanDescription'; + +describe('getSanitizedUrl', () => { + it.each([ + ['works without attributes', {}, SpanKind.CLIENT, undefined], + [ + 'uses url without query for client request', + { + [SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true', + [SemanticAttributes.HTTP_METHOD]: 'GET', + [SemanticAttributes.HTTP_TARGET]: '/?what=true', + [SemanticAttributes.HTTP_HOST]: 'example.com:80', + [SemanticAttributes.HTTP_STATUS_CODE]: 200, + }, + SpanKind.CLIENT, + 'http://example.com/', + ], + [ + 'uses url without hash for client request', + { + [SemanticAttributes.HTTP_URL]: 'http://example.com/sub#hash', + [SemanticAttributes.HTTP_METHOD]: 'GET', + [SemanticAttributes.HTTP_TARGET]: '/sub#hash', + [SemanticAttributes.HTTP_HOST]: 'example.com:80', + [SemanticAttributes.HTTP_STATUS_CODE]: 200, + }, + SpanKind.CLIENT, + 'http://example.com/sub', + ], + [ + 'uses route if available for client request', + { + [SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true', + [SemanticAttributes.HTTP_METHOD]: 'GET', + [SemanticAttributes.HTTP_TARGET]: '/?what=true', + [SemanticAttributes.HTTP_ROUTE]: '/my-route', + [SemanticAttributes.HTTP_HOST]: 'example.com:80', + [SemanticAttributes.HTTP_STATUS_CODE]: 200, + }, + SpanKind.CLIENT, + '/my-route', + ], + [ + 'falls back to target for client request if url not available', + { + [SemanticAttributes.HTTP_METHOD]: 'GET', + [SemanticAttributes.HTTP_TARGET]: '/?what=true', + [SemanticAttributes.HTTP_HOST]: 'example.com:80', + [SemanticAttributes.HTTP_STATUS_CODE]: 200, + }, + SpanKind.CLIENT, + '/', + ], + [ + 'uses target without query for server request', + { + [SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true', + [SemanticAttributes.HTTP_METHOD]: 'GET', + [SemanticAttributes.HTTP_TARGET]: '/?what=true', + [SemanticAttributes.HTTP_HOST]: 'example.com:80', + [SemanticAttributes.HTTP_STATUS_CODE]: 200, + }, + SpanKind.SERVER, + '/', + ], + [ + 'uses target without hash for server request', + { + [SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true', + [SemanticAttributes.HTTP_METHOD]: 'GET', + [SemanticAttributes.HTTP_TARGET]: '/sub#hash', + [SemanticAttributes.HTTP_HOST]: 'example.com:80', + [SemanticAttributes.HTTP_STATUS_CODE]: 200, + }, + SpanKind.SERVER, + '/sub', + ], + [ + 'uses route for server request if available', + { + [SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true', + [SemanticAttributes.HTTP_METHOD]: 'GET', + [SemanticAttributes.HTTP_TARGET]: '/?what=true', + [SemanticAttributes.HTTP_ROUTE]: '/my-route', + [SemanticAttributes.HTTP_HOST]: 'example.com:80', + [SemanticAttributes.HTTP_STATUS_CODE]: 200, + }, + SpanKind.SERVER, + '/my-route', + ], + ])('%s', (_, attributes, kind, expected) => { + const actual = getSanitizedUrl(attributes, kind); + + expect(actual).toEqual(expected); + }); +}); From c75ef77a2247b362aeceea62aef21d0467d57ae5 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 17 Jul 2023 10:19:05 +0200 Subject: [PATCH 2/3] use existing util --- .../src/utils/parseOtelSpanDescription.ts | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/packages/opentelemetry-node/src/utils/parseOtelSpanDescription.ts b/packages/opentelemetry-node/src/utils/parseOtelSpanDescription.ts index 7c6c3a4d4020..947577f2f528 100644 --- a/packages/opentelemetry-node/src/utils/parseOtelSpanDescription.ts +++ b/packages/opentelemetry-node/src/utils/parseOtelSpanDescription.ts @@ -3,7 +3,7 @@ import { SpanKind } from '@opentelemetry/api'; import type { Span as OtelSpan } from '@opentelemetry/sdk-trace-base'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; import type { TransactionSource } from '@sentry/types'; -import { getSanitizedUrlString, parseUrl } from '@sentry/utils'; +import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils'; interface SpanDescription { op: string | undefined; @@ -118,7 +118,7 @@ export function getSanitizedUrl(attributes: Attributes, kind: SpanKind): string } if (kind === SpanKind.SERVER && typeof httpTarget === 'string') { - return normalizeTarget(httpTarget); + return stripUrlQueryAndFragment(httpTarget); } if (typeof httpUrl === 'string') { @@ -128,13 +128,8 @@ export function getSanitizedUrl(attributes: Attributes, kind: SpanKind): string // fall back to target even for client spans, if no URL is present if (typeof httpTarget === 'string') { - return normalizeTarget(httpTarget); + return stripUrlQueryAndFragment(httpTarget); } return undefined; } - -// Remove trailing ? and # from the target -function normalizeTarget(httpTarget: string): string { - return httpTarget.replace(/([?#].*)$/, ''); -} From 714d56db52720a929768d57f0c23e962f2e952fa Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 17 Jul 2023 11:30:54 +0200 Subject: [PATCH 3/3] ensure we set data for http spans --- .../opentelemetry-node/src/spanprocessor.ts | 20 +++++- .../src/utils/parseOtelSpanDescription.ts | 56 +++++++++++++---- .../test/spanprocessor.test.ts | 48 ++++++++++++++- .../utils/parseOtelSpanDescription.test.ts | 61 ++++++++++++++++--- 4 files changed, 161 insertions(+), 24 deletions(-) diff --git a/packages/opentelemetry-node/src/spanprocessor.ts b/packages/opentelemetry-node/src/spanprocessor.ts index ca5609597686..980084a9adec 100644 --- a/packages/opentelemetry-node/src/spanprocessor.ts +++ b/packages/opentelemetry-node/src/spanprocessor.ts @@ -207,6 +207,8 @@ function getTraceData(otelSpan: OtelSpan, parentContext: Context): Partial { + const value = data[prop]; + sentrySpan.setData(prop, value); + }); + } + sentrySpan.op = op; sentrySpan.description = description; } function updateTransactionWithOtelData(transaction: Transaction, otelSpan: OtelSpan): void { + const { op, description, source, data } = parseSpanDescription(otelSpan); + transaction.setContext('otel', { attributes: otelSpan.attributes, resource: otelSpan.resource.attributes, }); + if (data) { + Object.keys(data).forEach(prop => { + const value = data[prop]; + transaction.setData(prop, value); + }); + } + transaction.setStatus(mapOtelStatus(otelSpan)); - const { op, description, source } = parseSpanDescription(otelSpan); transaction.op = op; transaction.setName(description, source); } diff --git a/packages/opentelemetry-node/src/utils/parseOtelSpanDescription.ts b/packages/opentelemetry-node/src/utils/parseOtelSpanDescription.ts index 947577f2f528..bfef8a4a5885 100644 --- a/packages/opentelemetry-node/src/utils/parseOtelSpanDescription.ts +++ b/packages/opentelemetry-node/src/utils/parseOtelSpanDescription.ts @@ -9,6 +9,7 @@ interface SpanDescription { op: string | undefined; description: string; source: TransactionSource; + data?: Record; } /** @@ -89,23 +90,48 @@ function descriptionForHttpMethod(otelSpan: OtelSpan, httpMethod: AttributeValue } const httpRoute = attributes[SemanticAttributes.HTTP_ROUTE]; - const url = getSanitizedUrl(attributes, kind); + const { urlPath, url, query, fragment } = getSanitizedUrl(attributes, kind); - if (!url) { + if (!urlPath) { return { op: opParts.join('.'), description: name, source: 'custom' }; } // Ex. description="GET /api/users". - const description = `${httpMethod} ${url}`; + const description = `${httpMethod} ${urlPath}`; // If `httpPath` is a root path, then we can categorize the transaction source as route. - const source: TransactionSource = httpRoute || url === '/' ? 'route' : 'url'; + const source: TransactionSource = httpRoute || urlPath === '/' ? 'route' : 'url'; - return { op: opParts.join('.'), description, source }; + const data: Record = {}; + + if (url) { + data.url = url; + } + if (query) { + data['http.query'] = query; + } + if (fragment) { + data['http.fragment'] = fragment; + } + + return { + op: opParts.join('.'), + description, + source, + data, + }; } /** Exported for tests only */ -export function getSanitizedUrl(attributes: Attributes, kind: SpanKind): string | undefined { +export function getSanitizedUrl( + attributes: Attributes, + kind: SpanKind, +): { + url: string | undefined; + urlPath: string | undefined; + query: string | undefined; + fragment: string | undefined; +} { // This is the relative path of the URL, e.g. /sub const httpTarget = attributes[SemanticAttributes.HTTP_TARGET]; // This is the full URL, including host & query params etc., e.g. https://example.com/sub?foo=bar @@ -113,23 +139,27 @@ export function getSanitizedUrl(attributes: Attributes, kind: SpanKind): string // This is the normalized route name - may not always be available! const httpRoute = attributes[SemanticAttributes.HTTP_ROUTE]; + const parsedUrl = typeof httpUrl === 'string' ? parseUrl(httpUrl) : undefined; + const url = parsedUrl ? getSanitizedUrlString(parsedUrl) : undefined; + const query = parsedUrl && parsedUrl.search ? parsedUrl.search : undefined; + const fragment = parsedUrl && parsedUrl.hash ? parsedUrl.hash : undefined; + if (typeof httpRoute === 'string') { - return httpRoute; + return { urlPath: httpRoute, url, query, fragment }; } if (kind === SpanKind.SERVER && typeof httpTarget === 'string') { - return stripUrlQueryAndFragment(httpTarget); + return { urlPath: stripUrlQueryAndFragment(httpTarget), url, query, fragment }; } - if (typeof httpUrl === 'string') { - const url = parseUrl(httpUrl); - return getSanitizedUrlString(url); + if (parsedUrl) { + return { urlPath: url, url, query, fragment }; } // fall back to target even for client spans, if no URL is present if (typeof httpTarget === 'string') { - return stripUrlQueryAndFragment(httpTarget); + return { urlPath: stripUrlQueryAndFragment(httpTarget), url, query, fragment }; } - return undefined; + return { urlPath: undefined, url, query, fragment }; } diff --git a/packages/opentelemetry-node/test/spanprocessor.test.ts b/packages/opentelemetry-node/test/spanprocessor.test.ts index cde0c7338d00..38c72ef5d3b8 100644 --- a/packages/opentelemetry-node/test/spanprocessor.test.ts +++ b/packages/opentelemetry-node/test/spanprocessor.test.ts @@ -434,10 +434,19 @@ describe('SentrySpanProcessor', () => { child.setAttribute(SemanticAttributes.HTTP_METHOD, 'GET'); child.setAttribute(SemanticAttributes.HTTP_ROUTE, '/my/route/{id}'); child.setAttribute(SemanticAttributes.HTTP_TARGET, '/my/route/123'); + child.setAttribute(SemanticAttributes.HTTP_URL, 'http://example.com/my/route/123'); child.end(); expect(sentrySpan?.description).toBe('GET /my/route/{id}'); + expect(sentrySpan?.data).toEqual({ + 'http.method': 'GET', + 'http.route': '/my/route/{id}', + 'http.target': '/my/route/123', + 'http.url': 'http://example.com/my/route/123', + 'otel.kind': 'INTERNAL', + url: 'http://example.com/my/route/123', + }); parentOtelSpan.end(); }); @@ -453,10 +462,47 @@ describe('SentrySpanProcessor', () => { child.setAttribute(SemanticAttributes.HTTP_METHOD, 'GET'); child.setAttribute(SemanticAttributes.HTTP_TARGET, '/my/route/123'); + child.setAttribute(SemanticAttributes.HTTP_URL, 'http://example.com/my/route/123'); child.end(); - expect(sentrySpan?.description).toBe('GET /my/route/123'); + expect(sentrySpan?.description).toBe('GET http://example.com/my/route/123'); + expect(sentrySpan?.data).toEqual({ + 'http.method': 'GET', + 'http.target': '/my/route/123', + 'http.url': 'http://example.com/my/route/123', + 'otel.kind': 'INTERNAL', + url: 'http://example.com/my/route/123', + }); + + parentOtelSpan.end(); + }); + }); + }); + + it('Adds query & hash data based on HTTP_URL', async () => { + const tracer = provider.getTracer('default'); + + tracer.startActiveSpan('GET /users', parentOtelSpan => { + tracer.startActiveSpan('HTTP GET', child => { + const sentrySpan = getSpanForOtelSpan(child); + + child.setAttribute(SemanticAttributes.HTTP_METHOD, 'GET'); + child.setAttribute(SemanticAttributes.HTTP_TARGET, '/my/route/123'); + child.setAttribute(SemanticAttributes.HTTP_URL, 'http://example.com/my/route/123?what=123#myHash'); + + child.end(); + + expect(sentrySpan?.description).toBe('GET http://example.com/my/route/123'); + expect(sentrySpan?.data).toEqual({ + 'http.method': 'GET', + 'http.target': '/my/route/123', + 'http.url': 'http://example.com/my/route/123?what=123#myHash', + 'otel.kind': 'INTERNAL', + url: 'http://example.com/my/route/123', + 'http.query': '?what=123', + 'http.fragment': '#myHash', + }); parentOtelSpan.end(); }); diff --git a/packages/opentelemetry-node/test/utils/parseOtelSpanDescription.test.ts b/packages/opentelemetry-node/test/utils/parseOtelSpanDescription.test.ts index 7b02dad6ccdb..b2d1b3654500 100644 --- a/packages/opentelemetry-node/test/utils/parseOtelSpanDescription.test.ts +++ b/packages/opentelemetry-node/test/utils/parseOtelSpanDescription.test.ts @@ -5,7 +5,17 @@ import { getSanitizedUrl } from '../../src/utils/parseOtelSpanDescription'; describe('getSanitizedUrl', () => { it.each([ - ['works without attributes', {}, SpanKind.CLIENT, undefined], + [ + 'works without attributes', + {}, + SpanKind.CLIENT, + { + urlPath: undefined, + url: undefined, + fragment: undefined, + query: undefined, + }, + ], [ 'uses url without query for client request', { @@ -16,7 +26,12 @@ describe('getSanitizedUrl', () => { [SemanticAttributes.HTTP_STATUS_CODE]: 200, }, SpanKind.CLIENT, - 'http://example.com/', + { + urlPath: 'http://example.com/', + url: 'http://example.com/', + fragment: undefined, + query: '?what=true', + }, ], [ 'uses url without hash for client request', @@ -28,7 +43,12 @@ describe('getSanitizedUrl', () => { [SemanticAttributes.HTTP_STATUS_CODE]: 200, }, SpanKind.CLIENT, - 'http://example.com/sub', + { + urlPath: 'http://example.com/sub', + url: 'http://example.com/sub', + fragment: '#hash', + query: undefined, + }, ], [ 'uses route if available for client request', @@ -41,7 +61,12 @@ describe('getSanitizedUrl', () => { [SemanticAttributes.HTTP_STATUS_CODE]: 200, }, SpanKind.CLIENT, - '/my-route', + { + urlPath: '/my-route', + url: 'http://example.com/', + fragment: undefined, + query: '?what=true', + }, ], [ 'falls back to target for client request if url not available', @@ -52,7 +77,12 @@ describe('getSanitizedUrl', () => { [SemanticAttributes.HTTP_STATUS_CODE]: 200, }, SpanKind.CLIENT, - '/', + { + urlPath: '/', + url: undefined, + fragment: undefined, + query: undefined, + }, ], [ 'uses target without query for server request', @@ -64,7 +94,12 @@ describe('getSanitizedUrl', () => { [SemanticAttributes.HTTP_STATUS_CODE]: 200, }, SpanKind.SERVER, - '/', + { + urlPath: '/', + url: 'http://example.com/', + fragment: undefined, + query: '?what=true', + }, ], [ 'uses target without hash for server request', @@ -76,7 +111,12 @@ describe('getSanitizedUrl', () => { [SemanticAttributes.HTTP_STATUS_CODE]: 200, }, SpanKind.SERVER, - '/sub', + { + urlPath: '/sub', + url: 'http://example.com/', + fragment: undefined, + query: '?what=true', + }, ], [ 'uses route for server request if available', @@ -89,7 +129,12 @@ describe('getSanitizedUrl', () => { [SemanticAttributes.HTTP_STATUS_CODE]: 200, }, SpanKind.SERVER, - '/my-route', + { + urlPath: '/my-route', + url: 'http://example.com/', + fragment: undefined, + query: '?what=true', + }, ], ])('%s', (_, attributes, kind, expected) => { const actual = getSanitizedUrl(attributes, kind);