Skip to content

feat(tracing): Try parameterizing URLs in default routing instrumentation #5411

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

Closed
wants to merge 1 commit into from
Closed

feat(tracing): Try parameterizing URLs in default routing instrumentation #5411

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
93 changes: 91 additions & 2 deletions packages/tracing/src/browser/router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,63 @@ import { addInstrumentationHandler, getGlobalObject, logger } from '@sentry/util

const global = getGlobalObject<Window>();

const PATHNAME_PARAMETER_PATTERNS = {
number: /^\d+$/,
'sha1-hash': /^[0-9a-f]{40}$/i,
'md-hash': /^[0-9a-f]{32}$/i,
uuid: /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i,
};

/**
* Tries to parameterize a provided pathname based on some heuristics.
*
* @param pathname - A pathname, usually obtained via `window.location.pathname`.
* @returns a parameterized version of the pathname alongside the values behind the parameters
*/
function extractPathnameParametersWithHeuristics(pathname: string): {
parameterizedPathname: string;
pathnameParameterValues: Record<string, string>;
hasParameters: boolean;
} {
/**
* Keeps track of the number of occurences of each pattern in the provided pathname.
*/
const patternCounts: Partial<Record<keyof typeof PATHNAME_PARAMETER_PATTERNS, number>> = {};

/**
* Keeps track of what found parameters in the URL evaluate to, e.g:
* { "uuid-1": "2778216e-b40c-46...", "uuid-2": "86024957-1789-47ec-be7..." }
*/
const pathnameParameterValues: Record<string, string> = {};
Copy link
Member Author

Choose a reason for hiding this comment

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

Question for reviewers: Do you think we actually need to include the parameters somewhere in the transaction data?

I am leaning towards no because we also include originalPathname in the transaction data which is enough for users to figure out what the parameters were.

Transaction data is currently also not queriable in discover so there wouldn't even be an immediate upside to including it in that regard.

Copy link
Member

Choose a reason for hiding this comment

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

Coming back to this even though the PR is closed (😿)

I think we don't include the parameters, it's un-needed bloat on the event JSON.


const parameterizedPathname = pathname
.split('/')
.map(originalPart => {
for (const patternName of Object.keys(
PATHNAME_PARAMETER_PATTERNS,
) as (keyof typeof PATHNAME_PARAMETER_PATTERNS)[]) {
if (originalPart.match(PATHNAME_PARAMETER_PATTERNS[patternName])) {
// Set patternCounts to 1 if it hasn't been set yet
patternCounts[patternName] = (patternCounts[patternName] || 0) + 1;

// Record what the parameter evaluated to
pathnameParameterValues[`${patternName}-${patternCounts[patternName]}`] = originalPart;

return `{${patternName}}`;
}
}

return originalPart;
})
.join('/');

return {
parameterizedPathname,
pathnameParameterValues,
hasParameters: Object.keys(pathnameParameterValues).length > 0,
};
}

/**
* Default function implementing pageload and navigation transactions
*/
Expand All @@ -20,9 +77,25 @@ export function instrumentRoutingWithDefaults<T extends Transaction>(

let activeTransaction: T | undefined;
if (startTransactionOnPageLoad) {
const { parameterizedPathname, pathnameParameterValues, hasParameters } = extractPathnameParametersWithHeuristics(
global.location.pathname,
);

activeTransaction = customStartTransaction({
name: global.location.pathname,
name: parameterizedPathname,
op: 'pageload',
data: hasParameters
? {
params: pathnameParameterValues,
originalPathname: global.location.pathname,
}
: undefined,
// For now, we will not define the transaction source as 'route' - even when we find a parameterizable
// pattern in the URL. Reason for that is, that we might run into URLs which both have parts that we identify
// as parameters, as well as parts that we didn't identify as such, even though they would be.
// An example: 'https://sentry.io/organization/some-random-org/user/14'
// Here we would identify '14' as parameter but not 'some-random-org'. To be on the safe side regarding
// transaction name cardinality, we still fall back to the source being 'url'.
metadata: { source: 'url' },
});
}
Expand Down Expand Up @@ -50,9 +123,25 @@ export function instrumentRoutingWithDefaults<T extends Transaction>(
// If there's an open transaction on the scope, we need to finish it before creating an new one.
activeTransaction.finish();
}

const { parameterizedPathname, pathnameParameterValues, hasParameters } =
extractPathnameParametersWithHeuristics(global.location.pathname);

activeTransaction = customStartTransaction({
name: global.location.pathname,
name: parameterizedPathname,
op: 'navigation',
data: hasParameters
? {
params: pathnameParameterValues,
originalPathname: global.location.pathname,
}
: undefined,
// For now, we will not define the transaction source as 'route' - even when we find a parameterizable
// pattern in the URL. Reason for that is, that we might run into URLs which both have parts that we identify
// as parameters, as well as parts that we didn't identify as such, even though they would be.
// An example: 'https://sentry.io/organization/some-random-org/user/14'
// Here we would identify '14' as parameter but not 'some-random-org'. To be on the safe side regarding
// transaction name cardinality, we still fall back to the source being 'url'.
metadata: { source: 'url' },
});
}
Expand Down
125 changes: 121 additions & 4 deletions packages/tracing/test/browser/router.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,15 @@ jest.mock('@sentry/utils', () => {
};
});

const DEFAULT_PAGE_URL = 'https://www.example.com/';

const dom = new JSDOM(undefined, { url: DEFAULT_PAGE_URL });

describe('instrumentRoutingWithDefaults', () => {
const mockFinish = jest.fn();
const customStartTransaction = jest.fn().mockReturnValue({ finish: mockFinish });
beforeEach(() => {
const dom = new JSDOM();
dom.reconfigure({ url: DEFAULT_PAGE_URL });
// @ts-ignore need to override global document
global.document = dom.window.document;
// @ts-ignore need to override global document
Expand All @@ -43,7 +47,7 @@ describe('instrumentRoutingWithDefaults', () => {
instrumentRoutingWithDefaults(customStartTransaction);
expect(customStartTransaction).toHaveBeenCalledTimes(1);
expect(customStartTransaction).toHaveBeenLastCalledWith({
name: 'blank',
name: '/',
op: 'pageload',
metadata: { source: 'url' },
});
Expand All @@ -54,6 +58,119 @@ describe('instrumentRoutingWithDefaults', () => {
expect(customStartTransaction).toHaveBeenCalledTimes(0);
});

it.each([
['https://example.com', '/', undefined],
['https://example.com/', '/', undefined],
['https://example.com/home', '/home', undefined],
['https://example.com/organization/some-org-slug', '/organization/some-org-slug', undefined],
// numbers
[
'https://example.com/organization/01337',
'/organization/{number}',
{ params: { 'number-1': '01337' }, originalPathname: '/organization/01337' },
],
[
'https://example.com/organization/01337/user/42',
'/organization/{number}/user/{number}',
{ params: { 'number-1': '01337', 'number-2': '42' }, originalPathname: '/organization/01337/user/42' },
],
[
'https://example.com/organization/01337/user/42/',
'/organization/{number}/user/{number}/',
{ params: { 'number-1': '01337', 'number-2': '42' }, originalPathname: '/organization/01337/user/42/' },
],
// SHA
[
'https://example.com/organization/3da541559918a808c2402bba5012f6c60b27661c',
'/organization/{sha1-hash}',
{
params: { 'sha1-hash-1': '3da541559918a808c2402bba5012f6c60b27661c' },
originalPathname: '/organization/3da541559918a808c2402bba5012f6c60b27661c',
},
],
[
'https://example.com/organization/3da541559918a808c2402bba5012f6c60b27661c/user/01ce26dc69094af9246ea7e7ce9970aff2b81cc9/',
'/organization/{sha1-hash}/user/{sha1-hash}/',
{
params: {
'sha1-hash-1': '3da541559918a808c2402bba5012f6c60b27661c',
'sha1-hash-2': '01ce26dc69094af9246ea7e7ce9970aff2b81cc9',
},
originalPathname:
'/organization/3da541559918a808c2402bba5012f6c60b27661c/user/01ce26dc69094af9246ea7e7ce9970aff2b81cc9/',
},
],
// MD
[
'https://example.com/organization/1bee69a46ba811185c194762abaeae90',
'/organization/{md-hash}',
{
params: { 'md-hash-1': '1bee69a46ba811185c194762abaeae90' },
originalPathname: '/organization/1bee69a46ba811185c194762abaeae90',
},
],
[
'https://example.com/organization/1bee69a46ba811185c194762abaeae90/user/b86e130ce7028da59e672d56ad0113df/',
'/organization/{md-hash}/user/{md-hash}/',
{
params: {
'md-hash-1': '1bee69a46ba811185c194762abaeae90',
'md-hash-2': 'b86e130ce7028da59e672d56ad0113df',
},
originalPathname: '/organization/1bee69a46ba811185c194762abaeae90/user/b86e130ce7028da59e672d56ad0113df/',
},
],
// UUID
[
'https://example.com/organization/7591173e-01c7-11ed-b939-0242ac120002',
'/organization/{uuid}',
{
params: { 'uuid-1': '7591173e-01c7-11ed-b939-0242ac120002' },
originalPathname: '/organization/7591173e-01c7-11ed-b939-0242ac120002',
},
],
[
'https://example.com/organization/7591173e-01c7-11ed-b939-0242ac120002/user/908607df-e5cb-4cc4-b61a-d6a534c43ec7/',
'/organization/{uuid}/user/{uuid}/',
{
params: {
'uuid-1': '7591173e-01c7-11ed-b939-0242ac120002',
'uuid-2': '908607df-e5cb-4cc4-b61a-d6a534c43ec7',
},
originalPathname:
'/organization/7591173e-01c7-11ed-b939-0242ac120002/user/908607df-e5cb-4cc4-b61a-d6a534c43ec7/',
},
],
// mixed
[
'https://example.com/organization/0012301/user/908607df-e5cb-4cc4-b61a-d6a534c43ec7/setting/424242/value/1bee69a46ba811185c194762abaeae90',
'/organization/{number}/user/{uuid}/setting/{number}/value/{md-hash}',
{
params: {
'md-hash-1': '1bee69a46ba811185c194762abaeae90',
'number-1': '0012301',
'number-2': '424242',
'uuid-1': '908607df-e5cb-4cc4-b61a-d6a534c43ec7',
},
originalPathname:
'/organization/0012301/user/908607df-e5cb-4cc4-b61a-d6a534c43ec7/setting/424242/value/1bee69a46ba811185c194762abaeae90',
},
],
])(
'should start a transaction with parameterization (url = %s)',
(simulatedUrl, expectedTransactionName, expectedTransactionData) => {
dom.reconfigure({ url: simulatedUrl });
instrumentRoutingWithDefaults(customStartTransaction);
expect(customStartTransaction).toHaveBeenCalledTimes(1);
expect(customStartTransaction).toHaveBeenLastCalledWith({
name: expectedTransactionName,
op: 'pageload',
data: expectedTransactionData,
metadata: { source: 'url' },
});
},
);

describe('navigation transaction', () => {
beforeEach(() => {
mockChangeHistory = () => undefined;
Expand All @@ -63,7 +180,7 @@ describe('instrumentRoutingWithDefaults', () => {
it('it is not created automatically', () => {
instrumentRoutingWithDefaults(customStartTransaction);
expect(customStartTransaction).not.toHaveBeenLastCalledWith({
name: 'blank',
name: '/',
op: 'navigation',
metadata: { source: 'url' },
});
Expand All @@ -76,7 +193,7 @@ describe('instrumentRoutingWithDefaults', () => {

expect(customStartTransaction).toHaveBeenCalledTimes(2);
expect(customStartTransaction).toHaveBeenLastCalledWith({
name: 'blank',
name: '/',
op: 'navigation',
metadata: { source: 'url' },
});
Expand Down