Skip to content

Commit f760c24

Browse files
AbhiPrasadAbhiPrasad
committed
ref(opentelemetry): Remove usage of parseUrl 
1 parent f0fc41f commit f760c24

File tree

4 files changed

+38
-208
lines changed

4 files changed

+38
-208
lines changed

‎packages/opentelemetry/src/utils/getRequestSpanData.ts‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import {
66
SEMATTRS_HTTP_METHOD,
77
SEMATTRS_HTTP_URL,
88
} from '@opentelemetry/semantic-conventions';
9-
import { getSanitizedUrlString, parseUrl } from '@sentry/core';
9+
import { parseStringToURLObject, getSanitizedUrlStringFromUrlObject } from '@sentry/core';
1010
import type { SanitizedRequestData } from '@sentry/core';
1111

1212
import { spanHasAttributes } from './spanTypes';
@@ -40,15 +40,15 @@ export function getRequestSpanData(span: Span | ReadableSpan): Partial<Sanitized
4040

4141
try {
4242
if (typeof maybeUrlAttribute === 'string') {
43-
const url = parseUrl(maybeUrlAttribute);
44-
45-
data.url = getSanitizedUrlString(url);
46-
47-
if (url.search) {
48-
data['http.query'] = url.search;
49-
}
50-
if (url.hash) {
51-
data['http.fragment'] = url.hash;
43+
const parsedUrl = parseStringToURLObject(maybeUrlAttribute);
44+
if (parsedUrl) {
45+
data.url = getSanitizedUrlStringFromUrlObject(parsedUrl);
46+
if (parsedUrl.search) {
47+
data['http.query'] = parsedUrl.search;
48+
}
49+
if (parsedUrl.hash) {
50+
data['http.fragment'] = parsedUrl.hash;
51+
}
5252
}
5353
}
5454
} catch {

‎packages/opentelemetry/src/utils/parseSpanDescription.ts‎

Lines changed: 25 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,9 @@ import {
1919
SEMANTIC_ATTRIBUTE_SENTRY_OP,
2020
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
2121
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
22-
getSanitizedUrlString,
23-
parseUrl,
24-
stripUrlQueryAndFragment,
22+
parseStringToURLObject,
23+
getSanitizedUrlStringFromUrlObject,
24+
isURLObjectRelative,
2525
} from '@sentry/core';
2626
import { SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION } from '../semanticAttributes';
2727
import type { AbstractSpan } from '../types';
@@ -156,16 +156,16 @@ export function descriptionForHttpMethod(
156156
opParts.push('prefetch');
157157
}
158158

159-
const { urlPath, url, query, fragment, hasRoute } = getSanitizedUrl(attributes, kind);
159+
const [parsedUrl, httpRoute] = getParsedUrl(attributes);
160160

161-
if (!urlPath) {
161+
if (!parsedUrl) {
162162
return { ...getUserUpdatedNameAndSource(name, attributes), op: opParts.join('.') };
163163
}
164164

165165
const graphqlOperationsAttribute = attributes[SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION];
166166

167167
// Ex. GET /api/users
168-
const baseDescription = `${httpMethod} ${urlPath}`;
168+
const baseDescription = `${httpMethod} ${httpRoute || getSanitizedUrlStringFromUrlObject(parsedUrl)}`;
169169

170170
// When the http span has a graphql operation, append it to the description
171171
// We add these in the graphqlIntegration
@@ -174,18 +174,16 @@ export function descriptionForHttpMethod(
174174
: baseDescription;
175175

176176
// If `httpPath` is a root path, then we can categorize the transaction source as route.
177-
const inferredSource: TransactionSource = hasRoute || urlPath === '/' ? 'route' : 'url';
177+
const inferredSource: TransactionSource = httpRoute || parsedUrl.pathname === '/' ? 'route' : 'url';
178178

179179
const data: Record<string, string> = {};
180180

181-
if (url) {
182-
data.url = url;
181+
data.url = isURLObjectRelative(parsedUrl) ? parsedUrl.pathname : parsedUrl.toString();
182+
if (parsedUrl.search) {
183+
data['http.query'] = parsedUrl.search;
183184
}
184-
if (query) {
185-
data['http.query'] = query;
186-
}
187-
if (fragment) {
188-
data['http.fragment'] = fragment;
185+
if (parsedUrl.hash) {
186+
data['http.fragment'] = parsedUrl.hash;
189187
}
190188

191189
// If the span kind is neither client nor server, we use the original name
@@ -234,48 +232,31 @@ function getGraphqlOperationNamesFromAttribute(attr: AttributeValue): string {
234232
}
235233

236234
/** Exported for tests only */
237-
export function getSanitizedUrl(
235+
export function getParsedUrl(
238236
attributes: Attributes,
239-
kind: SpanKind,
240-
): {
241-
url: string | undefined;
242-
urlPath: string | undefined;
243-
query: string | undefined;
244-
fragment: string | undefined;
245-
hasRoute: boolean;
246-
} {
237+
): [parsedUrl: ReturnType<typeof parseStringToURLObject>, httpRoute: string | undefined] {
247238
// This is the relative path of the URL, e.g. /sub
248239
// eslint-disable-next-line deprecation/deprecation
249-
const httpTarget = attributes[SEMATTRS_HTTP_TARGET];
240+
const possibleRelativeUrl = attributes[SEMATTRS_HTTP_TARGET];
250241
// This is the full URL, including host & query params etc., e.g. https://example.com/sub?foo=bar
251242
// eslint-disable-next-line deprecation/deprecation
252-
const httpUrl = attributes[SEMATTRS_HTTP_URL] || attributes[ATTR_URL_FULL];
243+
const possibleFullUrl = attributes[SEMATTRS_HTTP_URL] || attributes[ATTR_URL_FULL];
253244
// This is the normalized route name - may not always be available!
254-
const httpRoute = attributes[ATTR_HTTP_ROUTE];
255-
256-
const parsedUrl = typeof httpUrl === 'string' ? parseUrl(httpUrl) : undefined;
257-
const url = parsedUrl ? getSanitizedUrlString(parsedUrl) : undefined;
258-
const query = parsedUrl?.search || undefined;
259-
const fragment = parsedUrl?.hash || undefined;
245+
const httpRoute = attributes[ATTR_HTTP_ROUTE] as string | undefined;
260246

261-
if (typeof httpRoute === 'string') {
262-
return { urlPath: httpRoute, url, query, fragment, hasRoute: true };
263-
}
264-
265-
if (kind === SpanKind.SERVER && typeof httpTarget === 'string') {
266-
return { urlPath: stripUrlQueryAndFragment(httpTarget), url, query, fragment, hasRoute: false };
267-
}
247+
const parsedHttpUrl = typeof possibleFullUrl === 'string' ? parseStringToURLObject(possibleFullUrl) : undefined;
248+
const parsedHttpTarget =
249+
typeof possibleRelativeUrl === 'string' ? parseStringToURLObject(possibleRelativeUrl) : undefined;
268250

269-
if (parsedUrl) {
270-
return { urlPath: url, url, query, fragment, hasRoute: false };
251+
if (parsedHttpUrl) {
252+
return [parsedHttpUrl, httpRoute];
271253
}
272254

273-
// fall back to target even for client spans, if no URL is present
274-
if (typeof httpTarget === 'string') {
275-
return { urlPath: stripUrlQueryAndFragment(httpTarget), url, query, fragment, hasRoute: false };
255+
if (parsedHttpTarget) {
256+
return [parsedHttpTarget, httpRoute];
276257
}
277258

278-
return { urlPath: undefined, url, query, fragment, hasRoute: false };
259+
return [undefined, httpRoute];
279260
}
280261

281262
/**

‎packages/opentelemetry/test/utils/getRequestSpanData.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ describe('getRequestSpanData', () => {
4242
const data = getRequestSpanData(span);
4343

4444
expect(data).toEqual({
45-
url: 'http://example.com',
45+
url: 'http://example.com/',
4646
'http.method': 'GET',
4747
'http.query': '?foo=bar',
4848
'http.fragment': '#baz',
@@ -58,7 +58,7 @@ describe('getRequestSpanData', () => {
5858
const data = getRequestSpanData(span);
5959

6060
expect(data).toEqual({
61-
url: 'http://example.com',
61+
url: 'http://example.com/',
6262
'http.method': 'GET',
6363
});
6464
});

‎packages/opentelemetry/test/utils/parseSpanDescription.test.ts‎

Lines changed: 1 addition & 152 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,7 @@ import {
66
SEMATTRS_DB_STATEMENT,
77
SEMATTRS_DB_SYSTEM,
88
SEMATTRS_FAAS_TRIGGER,
9-
SEMATTRS_HTTP_HOST,
109
SEMATTRS_HTTP_METHOD,
11-
SEMATTRS_HTTP_STATUS_CODE,
1210
SEMATTRS_HTTP_TARGET,
1311
SEMATTRS_HTTP_URL,
1412
SEMATTRS_MESSAGING_SYSTEM,
@@ -19,7 +17,6 @@ import { describe, expect, it } from 'vitest';
1917
import { SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core';
2018
import {
2119
descriptionForHttpMethod,
22-
getSanitizedUrl,
2320
getUserUpdatedNameAndSource,
2421
parseSpanDescription,
2522
} from '../../src/utils/parseSpanDescription';
@@ -390,7 +387,7 @@ describe('descriptionForHttpMethod', () => {
390387
SpanKind.SERVER,
391388
{
392389
op: 'http.server',
393-
description: 'POST /my-path',
390+
description: 'POST https://www.example.com/my-path',
394391
data: {
395392
url: 'https://www.example.com/my-path',
396393
},
@@ -507,154 +504,6 @@ describe('descriptionForHttpMethod', () => {
507504
});
508505
});
509506

510-
describe('getSanitizedUrl', () => {
511-
it.each([
512-
[
513-
'works without attributes',
514-
{},
515-
SpanKind.CLIENT,
516-
{
517-
urlPath: undefined,
518-
url: undefined,
519-
fragment: undefined,
520-
query: undefined,
521-
hasRoute: false,
522-
},
523-
],
524-
[
525-
'uses url without query for client request',
526-
{
527-
[SEMATTRS_HTTP_URL]: 'http://example.com/?what=true',
528-
[SEMATTRS_HTTP_METHOD]: 'GET',
529-
[SEMATTRS_HTTP_TARGET]: '/?what=true',
530-
[SEMATTRS_HTTP_HOST]: 'example.com:80',
531-
[SEMATTRS_HTTP_STATUS_CODE]: 200,
532-
},
533-
SpanKind.CLIENT,
534-
{
535-
urlPath: 'http://example.com/',
536-
url: 'http://example.com/',
537-
fragment: undefined,
538-
query: '?what=true',
539-
hasRoute: false,
540-
},
541-
],
542-
[
543-
'uses url without hash for client request',
544-
{
545-
[SEMATTRS_HTTP_URL]: 'http://example.com/sub#hash',
546-
[SEMATTRS_HTTP_METHOD]: 'GET',
547-
[SEMATTRS_HTTP_TARGET]: '/sub#hash',
548-
[SEMATTRS_HTTP_HOST]: 'example.com:80',
549-
[SEMATTRS_HTTP_STATUS_CODE]: 200,
550-
},
551-
SpanKind.CLIENT,
552-
{
553-
urlPath: 'http://example.com/sub',
554-
url: 'http://example.com/sub',
555-
fragment: '#hash',
556-
query: undefined,
557-
hasRoute: false,
558-
},
559-
],
560-
[
561-
'uses route if available for client request',
562-
{
563-
[SEMATTRS_HTTP_URL]: 'http://example.com/?what=true',
564-
[SEMATTRS_HTTP_METHOD]: 'GET',
565-
[SEMATTRS_HTTP_TARGET]: '/?what=true',
566-
[ATTR_HTTP_ROUTE]: '/my-route',
567-
[SEMATTRS_HTTP_HOST]: 'example.com:80',
568-
[SEMATTRS_HTTP_STATUS_CODE]: 200,
569-
},
570-
SpanKind.CLIENT,
571-
{
572-
urlPath: '/my-route',
573-
url: 'http://example.com/',
574-
fragment: undefined,
575-
query: '?what=true',
576-
hasRoute: true,
577-
},
578-
],
579-
[
580-
'falls back to target for client request if url not available',
581-
{
582-
[SEMATTRS_HTTP_METHOD]: 'GET',
583-
[SEMATTRS_HTTP_TARGET]: '/?what=true',
584-
[SEMATTRS_HTTP_HOST]: 'example.com:80',
585-
[SEMATTRS_HTTP_STATUS_CODE]: 200,
586-
},
587-
SpanKind.CLIENT,
588-
{
589-
urlPath: '/',
590-
url: undefined,
591-
fragment: undefined,
592-
query: undefined,
593-
hasRoute: false,
594-
},
595-
],
596-
[
597-
'uses target without query for server request',
598-
{
599-
[SEMATTRS_HTTP_URL]: 'http://example.com/?what=true',
600-
[SEMATTRS_HTTP_METHOD]: 'GET',
601-
[SEMATTRS_HTTP_TARGET]: '/?what=true',
602-
[SEMATTRS_HTTP_HOST]: 'example.com:80',
603-
[SEMATTRS_HTTP_STATUS_CODE]: 200,
604-
},
605-
SpanKind.SERVER,
606-
{
607-
urlPath: '/',
608-
url: 'http://example.com/',
609-
fragment: undefined,
610-
query: '?what=true',
611-
hasRoute: false,
612-
},
613-
],
614-
[
615-
'uses target without hash for server request',
616-
{
617-
[SEMATTRS_HTTP_URL]: 'http://example.com/?what=true',
618-
[SEMATTRS_HTTP_METHOD]: 'GET',
619-
[SEMATTRS_HTTP_TARGET]: '/sub#hash',
620-
[SEMATTRS_HTTP_HOST]: 'example.com:80',
621-
[SEMATTRS_HTTP_STATUS_CODE]: 200,
622-
},
623-
SpanKind.SERVER,
624-
{
625-
urlPath: '/sub',
626-
url: 'http://example.com/',
627-
fragment: undefined,
628-
query: '?what=true',
629-
hasRoute: false,
630-
},
631-
],
632-
[
633-
'uses route for server request if available',
634-
{
635-
[SEMATTRS_HTTP_URL]: 'http://example.com/?what=true',
636-
[SEMATTRS_HTTP_METHOD]: 'GET',
637-
[SEMATTRS_HTTP_TARGET]: '/?what=true',
638-
[ATTR_HTTP_ROUTE]: '/my-route',
639-
[SEMATTRS_HTTP_HOST]: 'example.com:80',
640-
[SEMATTRS_HTTP_STATUS_CODE]: 200,
641-
},
642-
SpanKind.SERVER,
643-
{
644-
urlPath: '/my-route',
645-
url: 'http://example.com/',
646-
fragment: undefined,
647-
query: '?what=true',
648-
hasRoute: true,
649-
},
650-
],
651-
])('%s', (_, attributes, kind, expected) => {
652-
const actual = getSanitizedUrl(attributes, kind);
653-
654-
expect(actual).toEqual(expected);
655-
});
656-
});
657-
658507
describe('getUserUpdatedNameAndSource', () => {
659508
it('returns param name if `SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME` attribute is not set', () => {
660509
expect(getUserUpdatedNameAndSource('base name', {})).toEqual({ description: 'base name', source: 'custom' });

0 commit comments

Comments
 (0)