Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(opentelemetry): Remove usage of parseUrl #15954

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
+38 −208
Open

ref(opentelemetry): Remove usage of parseUrl #15954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 10 additions & 10 deletions packages/opentelemetry/src/utils/getRequestSpanData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
SEMATTRS_HTTP_METHOD,
SEMATTRS_HTTP_URL,
} from '@opentelemetry/semantic-conventions';
import { getSanitizedUrlString, parseUrl } from '@sentry/core';
import { parseStringToURLObject, getSanitizedUrlStringFromUrlObject } from '@sentry/core';
import type { SanitizedRequestData } from '@sentry/core';

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

try {
if (typeof maybeUrlAttribute === 'string') {
const url = parseUrl(maybeUrlAttribute);

data.url = getSanitizedUrlString(url);

if (url.search) {
data['http.query'] = url.search;
}
if (url.hash) {
data['http.fragment'] = url.hash;
const parsedUrl = parseStringToURLObject(maybeUrlAttribute);
if (parsedUrl) {
data.url = getSanitizedUrlStringFromUrlObject(parsedUrl);
if (parsedUrl.search) {
data['http.query'] = parsedUrl.search;
}
if (parsedUrl.hash) {
data['http.fragment'] = parsedUrl.hash;
}
}
}
} catch {
Expand Down
69 changes: 25 additions & 44 deletions packages/opentelemetry/src/utils/parseSpanDescription.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ import {
SEMANTIC_ATTRIBUTE_SENTRY_OP,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
getSanitizedUrlString,
parseUrl,
stripUrlQueryAndFragment,
parseStringToURLObject,
getSanitizedUrlStringFromUrlObject,
isURLObjectRelative,
} from '@sentry/core';
import { SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION } from '../semanticAttributes';
import type { AbstractSpan } from '../types';
Expand Down Expand Up @@ -156,16 +156,16 @@ export function descriptionForHttpMethod(
opParts.push('prefetch');
}

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

if (!urlPath) {
if (!parsedUrl) {
return { ...getUserUpdatedNameAndSource(name, attributes), op: opParts.join('.') };
Copy link
Member

Choose a reason for hiding this comment

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

h: If we have an httpRoute but no parsedUrl we would return the original name here. I don't know if that is desired.

}

const graphqlOperationsAttribute = attributes[SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION];

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

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

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

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

if (url) {
data.url = url;
data.url = isURLObjectRelative(parsedUrl) ? parsedUrl.pathname : parsedUrl.toString();
if (parsedUrl.search) {
data['http.query'] = parsedUrl.search;
}
if (query) {
data['http.query'] = query;
}
if (fragment) {
data['http.fragment'] = fragment;
if (parsedUrl.hash) {
data['http.fragment'] = parsedUrl.hash;
}

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

/** Exported for tests only */
export function getSanitizedUrl(
export function getParsedUrl(
attributes: Attributes,
kind: SpanKind,
): {
url: string | undefined;
urlPath: string | undefined;
query: string | undefined;
fragment: string | undefined;
hasRoute: boolean;
} {
): [parsedUrl: ReturnType<typeof parseStringToURLObject>, httpRoute: string | undefined] {
Copy link
Member

Choose a reason for hiding this comment

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

can we get the httpRoute logic out of here somehow? It seems like all we do is read the http.route attribute and return it.

// This is the relative path of the URL, e.g. /sub
// eslint-disable-next-line deprecation/deprecation
const httpTarget = attributes[SEMATTRS_HTTP_TARGET];
const possibleRelativeUrl = attributes[SEMATTRS_HTTP_TARGET];
// This is the full URL, including host & query params etc., e.g. https://example.com/sub?foo=bar
// eslint-disable-next-line deprecation/deprecation
const httpUrl = attributes[SEMATTRS_HTTP_URL] || attributes[ATTR_URL_FULL];
const possibleFullUrl = attributes[SEMATTRS_HTTP_URL] || attributes[ATTR_URL_FULL];
// This is the normalized route name - may not always be available!
const httpRoute = attributes[ATTR_HTTP_ROUTE];

const parsedUrl = typeof httpUrl === 'string' ? parseUrl(httpUrl) : undefined;
const url = parsedUrl ? getSanitizedUrlString(parsedUrl) : undefined;
const query = parsedUrl?.search || undefined;
const fragment = parsedUrl?.hash || undefined;
const httpRoute = attributes[ATTR_HTTP_ROUTE] as string | undefined;

if (typeof httpRoute === 'string') {
return { urlPath: httpRoute, url, query, fragment, hasRoute: true };
}

if (kind === SpanKind.SERVER && typeof httpTarget === 'string') {
return { urlPath: stripUrlQueryAndFragment(httpTarget), url, query, fragment, hasRoute: false };
}
const parsedHttpUrl = typeof possibleFullUrl === 'string' ? parseStringToURLObject(possibleFullUrl) : undefined;
const parsedHttpTarget =
typeof possibleRelativeUrl === 'string' ? parseStringToURLObject(possibleRelativeUrl) : undefined;

if (parsedUrl) {
return { urlPath: url, url, query, fragment, hasRoute: false };
if (parsedHttpUrl) {
return [parsedHttpUrl, httpRoute];
}

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

return { urlPath: undefined, url, query, fragment, hasRoute: false };
return [undefined, httpRoute];
}

/**
Expand Down
4 changes: 2 additions & 2 deletions packages/opentelemetry/test/utils/getRequestSpanData.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ describe('getRequestSpanData', () => {
const data = getRequestSpanData(span);

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

expect(data).toEqual({
url: 'http://example.com',
url: 'http://example.com/',
'http.method': 'GET',
});
});
Expand Down
153 changes: 1 addition & 152 deletions packages/opentelemetry/test/utils/parseSpanDescription.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@ import {
SEMATTRS_DB_STATEMENT,
SEMATTRS_DB_SYSTEM,
SEMATTRS_FAAS_TRIGGER,
SEMATTRS_HTTP_HOST,
SEMATTRS_HTTP_METHOD,
SEMATTRS_HTTP_STATUS_CODE,
SEMATTRS_HTTP_TARGET,
SEMATTRS_HTTP_URL,
SEMATTRS_MESSAGING_SYSTEM,
Expand All @@ -19,7 +17,6 @@ import { describe, expect, it } from 'vitest';
import { SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core';
import {
descriptionForHttpMethod,
getSanitizedUrl,
getUserUpdatedNameAndSource,
parseSpanDescription,
} from '../../src/utils/parseSpanDescription';
Expand Down Expand Up @@ -390,7 +387,7 @@ describe('descriptionForHttpMethod', () => {
SpanKind.SERVER,
{
op: 'http.server',
description: 'POST /my-path',
description: 'POST https://www.example.com/my-path',
data: {
url: 'https://www.example.com/my-path',
},
Expand Down Expand Up @@ -507,154 +504,6 @@ describe('descriptionForHttpMethod', () => {
});
});

describe('getSanitizedUrl', () => {
it.each([
[
'works without attributes',
{},
SpanKind.CLIENT,
{
urlPath: undefined,
url: undefined,
fragment: undefined,
query: undefined,
hasRoute: false,
},
],
[
'uses url without query for client request',
{
[SEMATTRS_HTTP_URL]: 'http://example.com/?what=true',
[SEMATTRS_HTTP_METHOD]: 'GET',
[SEMATTRS_HTTP_TARGET]: '/?what=true',
[SEMATTRS_HTTP_HOST]: 'example.com:80',
[SEMATTRS_HTTP_STATUS_CODE]: 200,
},
SpanKind.CLIENT,
{
urlPath: 'http://example.com/',
url: 'http://example.com/',
fragment: undefined,
query: '?what=true',
hasRoute: false,
},
],
[
'uses url without hash for client request',
{
[SEMATTRS_HTTP_URL]: 'http://example.com/sub#hash',
[SEMATTRS_HTTP_METHOD]: 'GET',
[SEMATTRS_HTTP_TARGET]: '/sub#hash',
[SEMATTRS_HTTP_HOST]: 'example.com:80',
[SEMATTRS_HTTP_STATUS_CODE]: 200,
},
SpanKind.CLIENT,
{
urlPath: 'http://example.com/sub',
url: 'http://example.com/sub',
fragment: '#hash',
query: undefined,
hasRoute: false,
},
],
[
'uses route if available for client request',
{
[SEMATTRS_HTTP_URL]: 'http://example.com/?what=true',
[SEMATTRS_HTTP_METHOD]: 'GET',
[SEMATTRS_HTTP_TARGET]: '/?what=true',
[ATTR_HTTP_ROUTE]: '/my-route',
[SEMATTRS_HTTP_HOST]: 'example.com:80',
[SEMATTRS_HTTP_STATUS_CODE]: 200,
},
SpanKind.CLIENT,
{
urlPath: '/my-route',
url: 'http://example.com/',
fragment: undefined,
query: '?what=true',
hasRoute: true,
},
],
[
'falls back to target for client request if url not available',
{
[SEMATTRS_HTTP_METHOD]: 'GET',
[SEMATTRS_HTTP_TARGET]: '/?what=true',
[SEMATTRS_HTTP_HOST]: 'example.com:80',
[SEMATTRS_HTTP_STATUS_CODE]: 200,
},
SpanKind.CLIENT,
{
urlPath: '/',
url: undefined,
fragment: undefined,
query: undefined,
hasRoute: false,
},
],
[
'uses target without query for server request',
{
[SEMATTRS_HTTP_URL]: 'http://example.com/?what=true',
[SEMATTRS_HTTP_METHOD]: 'GET',
[SEMATTRS_HTTP_TARGET]: '/?what=true',
[SEMATTRS_HTTP_HOST]: 'example.com:80',
[SEMATTRS_HTTP_STATUS_CODE]: 200,
},
SpanKind.SERVER,
{
urlPath: '/',
url: 'http://example.com/',
fragment: undefined,
query: '?what=true',
hasRoute: false,
},
],
[
'uses target without hash for server request',
{
[SEMATTRS_HTTP_URL]: 'http://example.com/?what=true',
[SEMATTRS_HTTP_METHOD]: 'GET',
[SEMATTRS_HTTP_TARGET]: '/sub#hash',
[SEMATTRS_HTTP_HOST]: 'example.com:80',
[SEMATTRS_HTTP_STATUS_CODE]: 200,
},
SpanKind.SERVER,
{
urlPath: '/sub',
url: 'http://example.com/',
fragment: undefined,
query: '?what=true',
hasRoute: false,
},
],
[
'uses route for server request if available',
{
[SEMATTRS_HTTP_URL]: 'http://example.com/?what=true',
[SEMATTRS_HTTP_METHOD]: 'GET',
[SEMATTRS_HTTP_TARGET]: '/?what=true',
[ATTR_HTTP_ROUTE]: '/my-route',
[SEMATTRS_HTTP_HOST]: 'example.com:80',
[SEMATTRS_HTTP_STATUS_CODE]: 200,
},
SpanKind.SERVER,
{
urlPath: '/my-route',
url: 'http://example.com/',
fragment: undefined,
query: '?what=true',
hasRoute: true,
},
],
])('%s', (_, attributes, kind, expected) => {
const actual = getSanitizedUrl(attributes, kind);

expect(actual).toEqual(expected);
});
});

describe('getUserUpdatedNameAndSource', () => {
it('returns param name if `SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME` attribute is not set', () => {
expect(getUserUpdatedNameAndSource('base name', {})).toEqual({ description: 'base name', source: 'custom' });
Expand Down
Loading