Skip to content

feat(core): Deprecate parseUrl #14404

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.

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions packages/browser-utils/src/instrument/xhr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export function instrumentXHR(): void {
// open() should always be called with two or more arguments
// But to be on the safe side, we actually validate this and bail out if we don't have a method & url
const method = isString(xhrOpenArgArray[0]) ? xhrOpenArgArray[0].toUpperCase() : undefined;
const url = parseUrl(xhrOpenArgArray[1]);
const url = ensureUrlIsString(xhrOpenArgArray[1]);

if (!method || !url) {
return originalOpen.apply(xhrOpenThisArg, xhrOpenArgArray);
Expand Down Expand Up @@ -140,7 +140,7 @@ export function instrumentXHR(): void {
});
}

function parseUrl(url: string | unknown): string | undefined {
function ensureUrlIsString(url: string | unknown): string | undefined {
if (isString(url)) {
return url;
}
Expand Down
20 changes: 12 additions & 8 deletions packages/browser-utils/src/metrics/browserMetrics.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* eslint-disable max-lines */
import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, getActiveSpan } from '@sentry/core';
import { setMeasurement } from '@sentry/core';
import { browserPerformanceTimeOrigin, getComponentName, htmlTreeAsString, logger, parseUrl } from '@sentry/core';
import { browserPerformanceTimeOrigin, getComponentName, htmlTreeAsString, logger } from '@sentry/core';
import type { Measurements, Span, SpanAttributes, StartSpanOptions } from '@sentry/types';

import { spanToJSON } from '@sentry/core';
Expand Down Expand Up @@ -545,8 +545,6 @@ export function _addResourceSpans(
return;
}

const parsedUrl = parseUrl(resourceUrl);

const attributes: SpanAttributes = {
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.resource.browser.metrics',
};
Expand All @@ -561,12 +559,18 @@ export function _addResourceSpans(
if ('renderBlockingStatus' in entry) {
attributes['resource.render_blocking_status'] = entry.renderBlockingStatus;
}
if (parsedUrl.protocol) {
attributes['url.scheme'] = parsedUrl.protocol.split(':').pop(); // the protocol returned by parseUrl includes a :, but OTEL spec does not, so we remove it.
}

if (parsedUrl.host) {
attributes['server.address'] = parsedUrl.host;
try {
// The URL constructor can throw when there is no protocol or host.
const parsedUrl = new URL(resourceUrl);
Copy link
Member

Choose a reason for hiding this comment

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

can resourceUrl be a path-only, without a host? 🤔 just double checking.

if (parsedUrl.protocol) {
Copy link
Member

Choose a reason for hiding this comment

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

can protocol be empty?

attributes['url.scheme'] = parsedUrl.protocol.split(':').pop(); // the protocol returned by parseUrl includes a :, but OTEL spec does not, so we remove it.
}
if (parsedUrl.host) {
Copy link
Member

Choose a reason for hiding this comment

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

can host even be empty? 🤔

attributes['server.address'] = parsedUrl.host;
}
} catch {
// noop
}

attributes['url.same_origin'] = resourceUrl.includes(WINDOW.location.origin);
Expand Down
23 changes: 13 additions & 10 deletions packages/browser/src/integrations/breadcrumbs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import {
getEventDescription,
htmlTreeAsString,
logger,
parseUrl,
safeJoin,
severityLevelFromString,
} from '@sentry/core';
Expand Down Expand Up @@ -328,6 +327,9 @@ function _getFetchBreadcrumbHandler(client: Client): (handlerData: HandlerDataFe
};
}

// Just a dummy url base for the `URL` constructor.
const DUMMY_URL_BASE = 'a://';

/**
* Creates breadcrumbs from history API calls
*/
Expand All @@ -337,24 +339,25 @@ function _getHistoryBreadcrumbHandler(client: Client): (handlerData: HandlerData
return;
}

const currentUrl = new URL(WINDOW.location.href);

let from: string | undefined = handlerData.from;
let to: string | undefined = handlerData.to;
const parsedLoc = parseUrl(WINDOW.location.href);
let parsedFrom = from ? parseUrl(from) : undefined;
const parsedTo = parseUrl(to);
let parsedFrom = from ? new URL(from, DUMMY_URL_BASE) : undefined;
const parsedTo = new URL(to, DUMMY_URL_BASE);
Copy link
Member

Choose a reason for hiding this comment

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

we should always try-catch this, to avoid stuff blowing up IMHO!


// Initial pushState doesn't provide `from` information
if (!parsedFrom || !parsedFrom.path) {
parsedFrom = parsedLoc;
if (!parsedFrom || !parsedFrom.pathname) {
parsedFrom = currentUrl;
}

// Use only the path component of the URL if the URL matches the current
// document (almost all the time when using pushState)
if (parsedLoc.protocol === parsedTo.protocol && parsedLoc.host === parsedTo.host) {
to = parsedTo.relative;
if (currentUrl.origin === parsedTo.origin) {
to = `${parsedTo.pathname}${parsedTo.search}${parsedTo.hash}`;
}
if (parsedLoc.protocol === parsedFrom.protocol && parsedLoc.host === parsedFrom.host) {
from = parsedFrom.relative;
if (currentUrl.origin === parsedFrom.origin) {
from = `${parsedTo.pathname}${parsedTo.search}${parsedTo.hash}`;
}

addBreadcrumb({
Expand Down
45 changes: 23 additions & 22 deletions packages/browser/src/tracing/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
getDynamicSamplingContextFromClient,
getDynamicSamplingContextFromSpan,
getIsolationScope,
getSanitizedUrlString,
hasTracingEnabled,
instrumentFetchRequest,
setHttpStatus,
Expand Down Expand Up @@ -173,15 +174,19 @@ export function instrumentOutgoingRequests(client: Client, _options?: Partial<Re
responseToSpanId.set(handlerData.response, handlerData.fetchData.__span);
}

// We cannot use `window.location` in the generic fetch instrumentation,
// but we need it for reliable `server.address` attribute.
// so we extend this in here
if (createdSpan) {
const fullUrl = getFullURL(handlerData.fetchData.url);
const host = fullUrl ? parseUrl(fullUrl).host : undefined;
let parsedUrl;
try {
// By adding a base URL to new URL(), this will also work for relative urls
// If `url` is a full URL, the base URL is ignored anyhow
parsedUrl = new URL(handlerData.fetchData.url, WINDOW.location.origin);
} catch {
// noop
}

createdSpan.setAttributes({
'http.url': fullUrl,
'server.address': host,
'http.url': parsedUrl ? getSanitizedUrlString(parsedUrl) : undefined,
'server.address': parsedUrl ? parsedUrl.host : undefined,
});
}

Expand Down Expand Up @@ -347,6 +352,7 @@ export function shouldAttachHeaders(
*
* @returns Span if a span was created, otherwise void.
*/
// eslint-disable-next-line complexity
export function xhrCallback(
handlerData: HandlerDataXhr,
shouldCreateSpan: (url: string) => boolean,
Expand Down Expand Up @@ -378,8 +384,14 @@ export function xhrCallback(
return undefined;
}

const fullUrl = getFullURL(sentryXhrData.url);
const host = fullUrl ? parseUrl(fullUrl).host : undefined;
let parsedUrl;
try {
// By adding a base URL to new URL(), this will also work for relative urls
// If `url` is a full URL, the base URL is ignored anyhow
parsedUrl = new URL(sentryXhrData.url, WINDOW.location.origin);
} catch {
// noop
}

const hasParent = !!getActiveSpan();

Expand All @@ -390,9 +402,9 @@ export function xhrCallback(
attributes: {
type: 'xhr',
'http.method': sentryXhrData.method,
'http.url': fullUrl,
'http.url': parsedUrl ? getSanitizedUrlString(parsedUrl) : undefined,
url: sentryXhrData.url,
'server.address': host,
'server.address': parsedUrl ? parsedUrl.host : undefined,
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.browser',
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.client',
},
Expand Down Expand Up @@ -455,14 +467,3 @@ function setHeaderOnXhr(
// Error: InvalidStateError: Failed to execute 'setRequestHeader' on 'XMLHttpRequest': The object's state must be OPENED.
}
}

function getFullURL(url: string): string | undefined {
try {
// By adding a base URL to new URL(), this will also work for relative urls
// If `url` is a full URL, the base URL is ignored anyhow
const parsed = new URL(url, WINDOW.location.origin);
return parsed.href;
} catch {
return undefined;
}
}
15 changes: 8 additions & 7 deletions packages/bun/src/integrations/bunserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {
startSpan,
withIsolationScope,
} from '@sentry/core';
import { extractQueryParamsFromUrl, getSanitizedUrlString, parseUrl } from '@sentry/core';
import type { IntegrationFn, RequestEventData, SpanAttributes } from '@sentry/types';

const INTEGRATION_NAME = 'BunServer';
Expand Down Expand Up @@ -50,6 +49,9 @@ export function instrumentBunServe(): void {
});
}

// Just a dummy url base for the `URL` constructor.
const DUMMY_URL_BASE = 'a://';

/**
* Instruments Bun.serve `fetch` option to automatically create spans and capture errors.
*/
Expand All @@ -63,24 +65,23 @@ function instrumentBunServeOptions(serveOptions: Parameters<typeof Bun.serve>[0]
return fetchTarget.apply(fetchThisArg, fetchArgs);
}

const parsedUrl = parseUrl(request.url);
const attributes: SpanAttributes = {
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.bun.serve',
[SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD]: request.method || 'GET',
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
};

const parsedUrl = new URL(request.url, DUMMY_URL_BASE);
Copy link
Member

Choose a reason for hiding this comment

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

try-catch, maybe?


if (parsedUrl.search) {
attributes['http.query'] = parsedUrl.search;
}

const url = getSanitizedUrlString(parsedUrl);

isolationScope.setSDKProcessingMetadata({
normalizedRequest: {
url,
url: `${parsedUrl.pathname}${parsedUrl.search}`,
method: request.method,
headers: request.headers.toJSON(),
query_string: extractQueryParamsFromUrl(url),
} satisfies RequestEventData,
});

Expand All @@ -91,7 +92,7 @@ function instrumentBunServeOptions(serveOptions: Parameters<typeof Bun.serve>[0]
{
attributes,
op: 'http.server',
name: `${request.method} ${parsedUrl.path || '/'}`,
name: `${request.method} ${parsedUrl.pathname || '/'}`,
},
async span => {
try {
Expand Down
22 changes: 8 additions & 14 deletions packages/core/src/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import {
} from './utils-hoist/baggage';
import { isInstanceOf } from './utils-hoist/is';
import { generateSentryTraceHeader } from './utils-hoist/tracing';
import { parseUrl } from './utils-hoist/url';
import { hasTracingEnabled } from './utils/hasTracingEnabled';
import { getActiveSpan, spanToTraceHeader } from './utils/spanUtils';

Expand Down Expand Up @@ -68,8 +67,12 @@ export function instrumentFetchRequest(

const { method, url } = handlerData.fetchData;

const fullUrl = getFullURL(url);
const host = fullUrl ? parseUrl(fullUrl).host : undefined;
let parsedUrl;
try {
parsedUrl = new URL(url);
} catch {
// noop
}

const hasParent = !!getActiveSpan();

Expand All @@ -81,8 +84,8 @@ export function instrumentFetchRequest(
url,
type: 'fetch',
'http.method': method,
'http.url': fullUrl,
'server.address': host,
'http.url': parsedUrl ? parsedUrl.href : undefined,
'server.address': parsedUrl ? parsedUrl.hostname : undefined,
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: spanOrigin,
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.client',
},
Expand Down Expand Up @@ -227,15 +230,6 @@ export function addTracingHeadersToFetchRequest(
}
}

function getFullURL(url: string): string | undefined {
try {
const parsed = new URL(url);
return parsed.href;
} catch {
return undefined;
}
}

function endSpan(span: Span, handlerData: HandlerDataFetch): void {
if (handlerData.response) {
setHttpStatus(span, handlerData.response.status);
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/utils-hoist/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ export {
parseBaggageHeader,
} from './baggage';

// eslint-disable-next-line deprecation/deprecation
export { getNumberOfUrlSegments, getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from './url';
export { makeFifoCache } from './cache';
export { eventFromMessage, eventFromUnknownInput, exceptionFromError, parseStackFrames } from './eventbuilder';
Expand Down
10 changes: 8 additions & 2 deletions packages/core/src/utils-hoist/url.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
type PartialURL = {
host?: string;
path?: string;
pathname?: string;
protocol?: string;
relative?: string;
search?: string;
Expand All @@ -13,6 +14,8 @@ type PartialURL = {
* // intentionally using regex and not <a/> href parsing trick because React Native and other
* // environments where DOM might not be available
* @returns parsed URL object
*
* @deprecated This function is deprecated and will be removed in the next major version. Use `new URL()` instead.
*/
export function parseUrl(url: string): PartialURL {
if (!url) {
Expand Down Expand Up @@ -61,7 +64,10 @@ export function getNumberOfUrlSegments(url: string): number {
* see: https://develop.sentry.dev/sdk/data-handling/#structuring-data
*/
export function getSanitizedUrlString(url: PartialURL): string {
const { protocol, host, path } = url;
const { protocol, host, path, pathname } = url;

// This is the compatibility layer between PartialURL and URL
const prioritizedPathArg = pathname || path;

const filteredHost =
(host &&
Expand All @@ -74,5 +80,5 @@ export function getSanitizedUrlString(url: PartialURL): string {
.replace(/(:443)$/, '')) ||
'';

return `${protocol ? `${protocol}://` : ''}${filteredHost}${path}`;
return `${protocol ? `${protocol}://` : ''}${filteredHost}${prioritizedPathArg}`;
}
1 change: 1 addition & 0 deletions packages/core/test/utils-hoist/url.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ describe('getSanitizedUrlString', () => {
['url with port 443', 'http://172.31.12.144:443/test', 'http://172.31.12.144/test'],
['url with IP and port 80', 'http://172.31.12.144:80/test', 'http://172.31.12.144/test'],
])('returns a sanitized URL for a %s', (_, rawUrl: string, sanitizedURL: string) => {
// eslint-disable-next-line deprecation/deprecation
const urlObject = parseUrl(rawUrl);
expect(getSanitizedUrlString(urlObject)).toEqual(sanitizedURL);
});
Expand Down
15 changes: 7 additions & 8 deletions packages/node/src/integrations/http/SentryHttpInstrumentation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import {
getSanitizedUrlString,
httpRequestToRequestData,
logger,
parseUrl,
stripUrlQueryAndFragment,
withIsolationScope,
} from '@sentry/core';
Expand Down Expand Up @@ -311,20 +310,20 @@ function addRequestBreadcrumb(request: http.ClientRequest, response: http.Incomi
function getBreadcrumbData(request: http.ClientRequest): Partial<SanitizedRequestData> {
try {
// `request.host` does not contain the port, but the host header does
const host = request.getHeader('host') || request.host;
const hostHeader = request.getHeader('host');
const host = typeof hostHeader === 'string' ? hostHeader : request.host;
const url = new URL(request.path, `${request.protocol}//${host}`);
const parsedUrl = parseUrl(url.toString());

const data: Partial<SanitizedRequestData> = {
url: getSanitizedUrlString(parsedUrl),
url: getSanitizedUrlString(url),
'http.method': request.method || 'GET',
};

if (parsedUrl.search) {
data['http.query'] = parsedUrl.search;
if (url.search) {
data['http.query'] = url.search;
}
if (parsedUrl.hash) {
data['http.fragment'] = parsedUrl.hash;
if (url.hash) {
data['http.fragment'] = url.hash;
}

return data;
Expand Down
Loading
Loading