From 29195118575729b6bddbf1dd8f26bf30ecab9a63 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 17 Jul 2023 17:28:49 +0200 Subject: [PATCH] fix(otel): Use `HTTP_URL` attribute for client requests (#8539) Turns out, `HTTP_TARGET` is always the relative path, even for outgoing requests, which omits the host. So we handle this specifically now here. While at it (and as I'm working a bit in this codebase), I also renamed the files from kebap-case to camelCase, to align with the rest of the codebase better - unless there was a specific reason to use that here @AbhiPrasad ? Closes https://github.com/getsentry/sentry-javascript/issues/8535 --- .../opentelemetry-node/src/spanprocessor.ts | 26 +++- ...s-sentry-request.ts => isSentryRequest.ts} | 0 .../{map-otel-status.ts => mapOtelStatus.ts} | 0 ...ription.ts => parseOtelSpanDescription.ts} | 76 +++++++-- .../test/spanprocessor.test.ts | 48 +++++- .../utils/parseOtelSpanDescription.test.ts | 144 ++++++++++++++++++ 6 files changed, 279 insertions(+), 15 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} (58%) 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..980084a9adec 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'], @@ -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/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 58% rename from packages/opentelemetry-node/src/utils/parse-otel-span-description.ts rename to packages/opentelemetry-node/src/utils/parseOtelSpanDescription.ts index 396e37a29ced..bfef8a4a5885 100644 --- a/packages/opentelemetry-node/src/utils/parse-otel-span-description.ts +++ b/packages/opentelemetry-node/src/utils/parseOtelSpanDescription.ts @@ -1,13 +1,15 @@ -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, stripUrlQueryAndFragment } from '@sentry/utils'; interface SpanDescription { op: string | undefined; description: string; source: TransactionSource; + data?: Record; } /** @@ -87,21 +89,77 @@ function descriptionForHttpMethod(otelSpan: OtelSpan, httpMethod: AttributeValue break; } - const httpTarget = attributes[SemanticAttributes.HTTP_TARGET]; const httpRoute = attributes[SemanticAttributes.HTTP_ROUTE]; + const { urlPath, url, query, fragment } = getSanitizedUrl(attributes, kind); - // Ex. /api/users - const httpPath = httpRoute || httpTarget; - - if (!httpPath) { + if (!urlPath) { return { op: opParts.join('.'), description: name, source: 'custom' }; } // Ex. description="GET /api/users". - const description = `${httpMethod} ${httpPath}`; + const description = `${httpMethod} ${urlPath}`; // 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 || urlPath === '/' ? 'route' : 'url'; + + 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, +): { + 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 + const httpUrl = attributes[SemanticAttributes.HTTP_URL]; + // 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 { urlPath: httpRoute, url, query, fragment }; + } + + if (kind === SpanKind.SERVER && typeof httpTarget === 'string') { + return { urlPath: stripUrlQueryAndFragment(httpTarget), url, query, fragment }; + } + + 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 { urlPath: stripUrlQueryAndFragment(httpTarget), url, query, fragment }; + } - return { op: opParts.join('.'), description, source }; + 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 new file mode 100644 index 000000000000..b2d1b3654500 --- /dev/null +++ b/packages/opentelemetry-node/test/utils/parseOtelSpanDescription.test.ts @@ -0,0 +1,144 @@ +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, + { + urlPath: undefined, + url: undefined, + fragment: undefined, + query: 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, + { + urlPath: 'http://example.com/', + url: 'http://example.com/', + fragment: undefined, + query: '?what=true', + }, + ], + [ + '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, + { + urlPath: 'http://example.com/sub', + url: 'http://example.com/sub', + fragment: '#hash', + query: undefined, + }, + ], + [ + '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, + { + urlPath: '/my-route', + url: 'http://example.com/', + fragment: undefined, + query: '?what=true', + }, + ], + [ + '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, + { + urlPath: '/', + url: undefined, + fragment: undefined, + query: undefined, + }, + ], + [ + '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, + { + urlPath: '/', + url: 'http://example.com/', + fragment: undefined, + query: '?what=true', + }, + ], + [ + '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, + { + urlPath: '/sub', + url: 'http://example.com/', + fragment: undefined, + query: '?what=true', + }, + ], + [ + '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, + { + urlPath: '/my-route', + url: 'http://example.com/', + fragment: undefined, + query: '?what=true', + }, + ], + ])('%s', (_, attributes, kind, expected) => { + const actual = getSanitizedUrl(attributes, kind); + + expect(actual).toEqual(expected); + }); +});