Skip to content

Commit 2919511

Browse files
mydeamydea
authored
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 #8535
1 parent 459c5d2 commit 2919511

File tree

6 files changed

+279
-15
lines changed

6 files changed

+279
-15
lines changed

‎packages/opentelemetry-node/src/spanprocessor.ts

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,9 @@ import type { DynamicSamplingContext, Span as SentrySpan, TraceparentData, Trans
77
import { isString, logger } from '@sentry/utils';
88

99
import { SENTRY_DYNAMIC_SAMPLING_CONTEXT_KEY, SENTRY_TRACE_PARENT_CONTEXT_KEY } from './constants';
10-
import { isSentryRequestSpan } from './utils/is-sentry-request';
11-
import { mapOtelStatus } from './utils/map-otel-status';
12-
import { parseSpanDescription } from './utils/parse-otel-span-description';
10+
import { isSentryRequestSpan } from './utils/isSentryRequest';
11+
import { mapOtelStatus } from './utils/mapOtelStatus';
12+
import { parseSpanDescription } from './utils/parseOtelSpanDescription';
1313

1414
export const SENTRY_SPAN_PROCESSOR_MAP: Map<SentrySpan['spanId'], SentrySpan> = new Map<
1515
SentrySpan['spanId'],
@@ -207,6 +207,8 @@ function getTraceData(otelSpan: OtelSpan, parentContext: Context): Partial<Trans
207207
function updateSpanWithOtelData(sentrySpan: SentrySpan, otelSpan: OtelSpan): void {
208208
const { attributes, kind } = otelSpan;
209209

210+
const { op, description, data } = parseSpanDescription(otelSpan);
211+
210212
sentrySpan.setStatus(mapOtelStatus(otelSpan));
211213
sentrySpan.setData('otel.kind', SpanKind[kind]);
212214

@@ -215,20 +217,34 @@ function updateSpanWithOtelData(sentrySpan: SentrySpan, otelSpan: OtelSpan): voi
215217
sentrySpan.setData(prop, value);
216218
});
217219

218-
const { op, description } = parseSpanDescription(otelSpan);
220+
if (data) {
221+
Object.keys(data).forEach(prop => {
222+
const value = data[prop];
223+
sentrySpan.setData(prop, value);
224+
});
225+
}
226+
219227
sentrySpan.op = op;
220228
sentrySpan.description = description;
221229
}
222230

223231
function updateTransactionWithOtelData(transaction: Transaction, otelSpan: OtelSpan): void {
232+
const { op, description, source, data } = parseSpanDescription(otelSpan);
233+
224234
transaction.setContext('otel', {
225235
attributes: otelSpan.attributes,
226236
resource: otelSpan.resource.attributes,
227237
});
228238

239+
if (data) {
240+
Object.keys(data).forEach(prop => {
241+
const value = data[prop];
242+
transaction.setData(prop, value);
243+
});
244+
}
245+
229246
transaction.setStatus(mapOtelStatus(otelSpan));
230247

231-
const { op, description, source } = parseSpanDescription(otelSpan);
232248
transaction.op = op;
233249
transaction.setName(description, source);
234250
}

‎packages/opentelemetry-node/src/utils/is-sentry-request.ts renamed to ‎packages/opentelemetry-node/src/utils/isSentryRequest.ts

File renamed without changes.

‎packages/opentelemetry-node/src/utils/map-otel-status.ts renamed to ‎packages/opentelemetry-node/src/utils/mapOtelStatus.ts

File renamed without changes.

‎packages/opentelemetry-node/src/utils/parse-otel-span-description.ts renamed to ‎packages/opentelemetry-node/src/utils/parseOtelSpanDescription.ts

Lines changed: 67 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,15 @@
1-
import type { AttributeValue } from '@opentelemetry/api';
1+
import type { Attributes, AttributeValue } from '@opentelemetry/api';
22
import { SpanKind } from '@opentelemetry/api';
33
import type { Span as OtelSpan } from '@opentelemetry/sdk-trace-base';
44
import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
55
import type { TransactionSource } from '@sentry/types';
6+
import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils';
67

78
interface SpanDescription {
89
op: string | undefined;
910
description: string;
1011
source: TransactionSource;
12+
data?: Record<string, string>;
1113
}
1214

1315
/**
@@ -87,21 +89,77 @@ function descriptionForHttpMethod(otelSpan: OtelSpan, httpMethod: AttributeValue
8789
break;
8890
}
8991

90-
const httpTarget = attributes[SemanticAttributes.HTTP_TARGET];
9192
const httpRoute = attributes[SemanticAttributes.HTTP_ROUTE];
93+
const { urlPath, url, query, fragment } = getSanitizedUrl(attributes, kind);
9294

93-
// Ex. /api/users
94-
const httpPath = httpRoute || httpTarget;
95-
96-
if (!httpPath) {
95+
if (!urlPath) {
9796
return { op: opParts.join('.'), description: name, source: 'custom' };
9897
}
9998

10099
// Ex. description="GET /api/users".
101-
const description = `${httpMethod} ${httpPath}`;
100+
const description = `${httpMethod} ${urlPath}`;
102101

103102
// If `httpPath` is a root path, then we can categorize the transaction source as route.
104-
const source: TransactionSource = httpRoute || httpPath === '/' ? 'route' : 'url';
103+
const source: TransactionSource = httpRoute || urlPath === '/' ? 'route' : 'url';
104+
105+
const data: Record<string, string> = {};
106+
107+
if (url) {
108+
data.url = url;
109+
}
110+
if (query) {
111+
data['http.query'] = query;
112+
}
113+
if (fragment) {
114+
data['http.fragment'] = fragment;
115+
}
116+
117+
return {
118+
op: opParts.join('.'),
119+
description,
120+
source,
121+
data,
122+
};
123+
}
124+
125+
/** Exported for tests only */
126+
export function getSanitizedUrl(
127+
attributes: Attributes,
128+
kind: SpanKind,
129+
): {
130+
url: string | undefined;
131+
urlPath: string | undefined;
132+
query: string | undefined;
133+
fragment: string | undefined;
134+
} {
135+
// This is the relative path of the URL, e.g. /sub
136+
const httpTarget = attributes[SemanticAttributes.HTTP_TARGET];
137+
// This is the full URL, including host & query params etc., e.g. https://example.com/sub?foo=bar
138+
const httpUrl = attributes[SemanticAttributes.HTTP_URL];
139+
// This is the normalized route name - may not always be available!
140+
const httpRoute = attributes[SemanticAttributes.HTTP_ROUTE];
141+
142+
const parsedUrl = typeof httpUrl === 'string' ? parseUrl(httpUrl) : undefined;
143+
const url = parsedUrl ? getSanitizedUrlString(parsedUrl) : undefined;
144+
const query = parsedUrl && parsedUrl.search ? parsedUrl.search : undefined;
145+
const fragment = parsedUrl && parsedUrl.hash ? parsedUrl.hash : undefined;
146+
147+
if (typeof httpRoute === 'string') {
148+
return { urlPath: httpRoute, url, query, fragment };
149+
}
150+
151+
if (kind === SpanKind.SERVER && typeof httpTarget === 'string') {
152+
return { urlPath: stripUrlQueryAndFragment(httpTarget), url, query, fragment };
153+
}
154+
155+
if (parsedUrl) {
156+
return { urlPath: url, url, query, fragment };
157+
}
158+
159+
// fall back to target even for client spans, if no URL is present
160+
if (typeof httpTarget === 'string') {
161+
return { urlPath: stripUrlQueryAndFragment(httpTarget), url, query, fragment };
162+
}
105163

106-
return { op: opParts.join('.'), description, source };
164+
return { urlPath: undefined, url, query, fragment };
107165
}

‎packages/opentelemetry-node/test/spanprocessor.test.ts

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -434,10 +434,19 @@ describe('SentrySpanProcessor', () => {
434434
child.setAttribute(SemanticAttributes.HTTP_METHOD, 'GET');
435435
child.setAttribute(SemanticAttributes.HTTP_ROUTE, '/my/route/{id}');
436436
child.setAttribute(SemanticAttributes.HTTP_TARGET, '/my/route/123');
437+
child.setAttribute(SemanticAttributes.HTTP_URL, 'http://example.com/my/route/123');
437438

438439
child.end();
439440

440441
expect(sentrySpan?.description).toBe('GET /my/route/{id}');
442+
expect(sentrySpan?.data).toEqual({
443+
'http.method': 'GET',
444+
'http.route': '/my/route/{id}',
445+
'http.target': '/my/route/123',
446+
'http.url': 'http://example.com/my/route/123',
447+
'otel.kind': 'INTERNAL',
448+
url: 'http://example.com/my/route/123',
449+
});
441450

442451
parentOtelSpan.end();
443452
});
@@ -453,10 +462,47 @@ describe('SentrySpanProcessor', () => {
453462

454463
child.setAttribute(SemanticAttributes.HTTP_METHOD, 'GET');
455464
child.setAttribute(SemanticAttributes.HTTP_TARGET, '/my/route/123');
465+
child.setAttribute(SemanticAttributes.HTTP_URL, 'http://example.com/my/route/123');
456466

457467
child.end();
458468

459-
expect(sentrySpan?.description).toBe('GET /my/route/123');
469+
expect(sentrySpan?.description).toBe('GET http://example.com/my/route/123');
470+
expect(sentrySpan?.data).toEqual({
471+
'http.method': 'GET',
472+
'http.target': '/my/route/123',
473+
'http.url': 'http://example.com/my/route/123',
474+
'otel.kind': 'INTERNAL',
475+
url: 'http://example.com/my/route/123',
476+
});
477+
478+
parentOtelSpan.end();
479+
});
480+
});
481+
});
482+
483+
it('Adds query & hash data based on HTTP_URL', async () => {
484+
const tracer = provider.getTracer('default');
485+
486+
tracer.startActiveSpan('GET /users', parentOtelSpan => {
487+
tracer.startActiveSpan('HTTP GET', child => {
488+
const sentrySpan = getSpanForOtelSpan(child);
489+
490+
child.setAttribute(SemanticAttributes.HTTP_METHOD, 'GET');
491+
child.setAttribute(SemanticAttributes.HTTP_TARGET, '/my/route/123');
492+
child.setAttribute(SemanticAttributes.HTTP_URL, 'http://example.com/my/route/123?what=123#myHash');
493+
494+
child.end();
495+
496+
expect(sentrySpan?.description).toBe('GET http://example.com/my/route/123');
497+
expect(sentrySpan?.data).toEqual({
498+
'http.method': 'GET',
499+
'http.target': '/my/route/123',
500+
'http.url': 'http://example.com/my/route/123?what=123#myHash',
501+
'otel.kind': 'INTERNAL',
502+
url: 'http://example.com/my/route/123',
503+
'http.query': '?what=123',
504+
'http.fragment': '#myHash',
505+
});
460506

461507
parentOtelSpan.end();
462508
});

‎packages/opentelemetry-node/test/utils/parseOtelSpanDescription.test.ts

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
import { SpanKind } from '@opentelemetry/api';
2+
import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
3+
4+
import { getSanitizedUrl } from '../../src/utils/parseOtelSpanDescription';
5+
6+
describe('getSanitizedUrl', () => {
7+
it.each([
8+
[
9+
'works without attributes',
10+
{},
11+
SpanKind.CLIENT,
12+
{
13+
urlPath: undefined,
14+
url: undefined,
15+
fragment: undefined,
16+
query: undefined,
17+
},
18+
],
19+
[
20+
'uses url without query for client request',
21+
{
22+
[SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true',
23+
[SemanticAttributes.HTTP_METHOD]: 'GET',
24+
[SemanticAttributes.HTTP_TARGET]: '/?what=true',
25+
[SemanticAttributes.HTTP_HOST]: 'example.com:80',
26+
[SemanticAttributes.HTTP_STATUS_CODE]: 200,
27+
},
28+
SpanKind.CLIENT,
29+
{
30+
urlPath: 'http://example.com/',
31+
url: 'http://example.com/',
32+
fragment: undefined,
33+
query: '?what=true',
34+
},
35+
],
36+
[
37+
'uses url without hash for client request',
38+
{
39+
[SemanticAttributes.HTTP_URL]: 'http://example.com/sub#hash',
40+
[SemanticAttributes.HTTP_METHOD]: 'GET',
41+
[SemanticAttributes.HTTP_TARGET]: '/sub#hash',
42+
[SemanticAttributes.HTTP_HOST]: 'example.com:80',
43+
[SemanticAttributes.HTTP_STATUS_CODE]: 200,
44+
},
45+
SpanKind.CLIENT,
46+
{
47+
urlPath: 'http://example.com/sub',
48+
url: 'http://example.com/sub',
49+
fragment: '#hash',
50+
query: undefined,
51+
},
52+
],
53+
[
54+
'uses route if available for client request',
55+
{
56+
[SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true',
57+
[SemanticAttributes.HTTP_METHOD]: 'GET',
58+
[SemanticAttributes.HTTP_TARGET]: '/?what=true',
59+
[SemanticAttributes.HTTP_ROUTE]: '/my-route',
60+
[SemanticAttributes.HTTP_HOST]: 'example.com:80',
61+
[SemanticAttributes.HTTP_STATUS_CODE]: 200,
62+
},
63+
SpanKind.CLIENT,
64+
{
65+
urlPath: '/my-route',
66+
url: 'http://example.com/',
67+
fragment: undefined,
68+
query: '?what=true',
69+
},
70+
],
71+
[
72+
'falls back to target for client request if url not available',
73+
{
74+
[SemanticAttributes.HTTP_METHOD]: 'GET',
75+
[SemanticAttributes.HTTP_TARGET]: '/?what=true',
76+
[SemanticAttributes.HTTP_HOST]: 'example.com:80',
77+
[SemanticAttributes.HTTP_STATUS_CODE]: 200,
78+
},
79+
SpanKind.CLIENT,
80+
{
81+
urlPath: '/',
82+
url: undefined,
83+
fragment: undefined,
84+
query: undefined,
85+
},
86+
],
87+
[
88+
'uses target without query for server request',
89+
{
90+
[SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true',
91+
[SemanticAttributes.HTTP_METHOD]: 'GET',
92+
[SemanticAttributes.HTTP_TARGET]: '/?what=true',
93+
[SemanticAttributes.HTTP_HOST]: 'example.com:80',
94+
[SemanticAttributes.HTTP_STATUS_CODE]: 200,
95+
},
96+
SpanKind.SERVER,
97+
{
98+
urlPath: '/',
99+
url: 'http://example.com/',
100+
fragment: undefined,
101+
query: '?what=true',
102+
},
103+
],
104+
[
105+
'uses target without hash for server request',
106+
{
107+
[SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true',
108+
[SemanticAttributes.HTTP_METHOD]: 'GET',
109+
[SemanticAttributes.HTTP_TARGET]: '/sub#hash',
110+
[SemanticAttributes.HTTP_HOST]: 'example.com:80',
111+
[SemanticAttributes.HTTP_STATUS_CODE]: 200,
112+
},
113+
SpanKind.SERVER,
114+
{
115+
urlPath: '/sub',
116+
url: 'http://example.com/',
117+
fragment: undefined,
118+
query: '?what=true',
119+
},
120+
],
121+
[
122+
'uses route for server request if available',
123+
{
124+
[SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true',
125+
[SemanticAttributes.HTTP_METHOD]: 'GET',
126+
[SemanticAttributes.HTTP_TARGET]: '/?what=true',
127+
[SemanticAttributes.HTTP_ROUTE]: '/my-route',
128+
[SemanticAttributes.HTTP_HOST]: 'example.com:80',
129+
[SemanticAttributes.HTTP_STATUS_CODE]: 200,
130+
},
131+
SpanKind.SERVER,
132+
{
133+
urlPath: '/my-route',
134+
url: 'http://example.com/',
135+
fragment: undefined,
136+
query: '?what=true',
137+
},
138+
],
139+
])('%s', (_, attributes, kind, expected) => {
140+
const actual = getSanitizedUrl(attributes, kind);
141+
142+
expect(actual).toEqual(expected);
143+
});
144+
});

0 commit comments

Comments
 (0)