Skip to content

Commit

Permalink
feat(core): Deprecate parseUrl
Browse files Browse the repository at this point in the history
  • Loading branch information
lforst committed Nov 21, 2024
1 parent 2435b87 commit 26f38eb
Show file tree
Hide file tree
Showing 13 changed files with 107 additions and 87 deletions.
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);
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;
}
} 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);

// 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);

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

0 comments on commit 26f38eb

Please sign in to comment.