Repository navigation
fix(core): Deduplicate native HTTP breadcrumbs for URLs with a query string #6881
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
Changes from all commits
3ae4a05
2de5669
7ab4ac9
aaae161
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -119,19 +119,25 @@ function deduplicateNativeHttpBreadcrumbs(nativeBreadcrumbs: Breadcrumb[], jsBre | |
| } | ||
|
|
||
| const consumedJsIndices = new Set<number>(); | ||
| const jsUrls = jsHttpBreadcrumbs.map(b => normalizeJsHttpUrl(b.data?.url)); | ||
|
|
||
| return nativeBreadcrumbs.filter(nativeBreadcrumb => { | ||
| if (nativeBreadcrumb.type !== 'http' || nativeBreadcrumb.category !== 'http') { | ||
| return true; | ||
| } | ||
|
|
||
| const nativeUrl = normalizeNativeHttpUrl(nativeBreadcrumb.data?.url); | ||
|
|
||
| const matchIndex = jsHttpBreadcrumbs.findIndex((jsBreadcrumb, index) => { | ||
| if (consumedJsIndices.has(index)) { | ||
| return false; | ||
| } | ||
|
|
||
| const jsUrl = jsUrls[index]; | ||
| const sameMethod = nativeBreadcrumb.data?.method === jsBreadcrumb.data?.method; | ||
| const sameUrl = nativeBreadcrumb.data?.url === jsBreadcrumb.data?.url; | ||
| const sameUrl = | ||
| nativeBreadcrumb.data?.url === jsBreadcrumb.data?.url || | ||
| (nativeUrl !== undefined && jsUrl !== undefined && isSameHttpUrl(nativeUrl, jsUrl)); | ||
| const nativeStatus = nativeBreadcrumb.data?.status_code; | ||
| const jsStatus = jsBreadcrumb.data?.status_code; | ||
| const sameStatus = | ||
|
|
@@ -155,3 +161,56 @@ function deduplicateNativeHttpBreadcrumbs(nativeBreadcrumbs: Breadcrumb[], jsBre | |
| return true; | ||
| }); | ||
| } | ||
|
|
||
| /* | ||
| * The JS breadcrumb keeps the raw request URL. The native one is sanitized: credentials are replaced, | ||
| * the fragment is removed, the query is moved to `data["http.query"]` or kept filtered, and the URL | ||
| * is percent-decoded. The query is ignored because native may filter or drop it. | ||
| */ | ||
|
|
||
| /** Returns the decoded JS URL without query, fragment or trailing slash. */ | ||
| function normalizeJsHttpUrl(url: unknown): string | undefined { | ||
| if (typeof url !== 'string') { | ||
| return undefined; | ||
| } | ||
|
|
||
| // Cut on the raw URL, where `?` and `#` are always delimiters. | ||
| const queryOrFragmentStart = url.search(/[?#]/); | ||
| const normalized = decodeUrl( | ||
| normalizeUrlAuthority(queryOrFragmentStart === -1 ? url : url.slice(0, queryOrFragmentStart)), | ||
| ); | ||
|
|
||
| return normalized.length > 1 && normalized.endsWith('/') ? normalized.slice(0, -1) : normalized; | ||
| } | ||
|
|
||
| /** Returns the decoded native URL. It may still contain a (filtered) query. */ | ||
| function normalizeNativeHttpUrl(url: unknown): string | undefined { | ||
| return typeof url === 'string' ? decodeUrl(normalizeUrlAuthority(url)) : undefined; | ||
| } | ||
|
|
||
| /** Matches the native URL to the JS one, with or without a trailing slash or query. */ | ||
| function isSameHttpUrl(nativeUrl: string, jsUrl: string): boolean { | ||
| return ( | ||
| nativeUrl === jsUrl || | ||
| nativeUrl === `${jsUrl}/` || | ||
| nativeUrl.startsWith(`${jsUrl}?`) || | ||
| nativeUrl.startsWith(`${jsUrl}/?`) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a chance the question mark will be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is already covered since it is decoded at this point. Also checked with the |
||
| ); | ||
| } | ||
|
|
||
| /** Removes credentials and lowercases the scheme and host. */ | ||
| function normalizeUrlAuthority(url: string): string { | ||
| return url.replace(/^([a-zA-Z][a-zA-Z0-9+.-]*:\/\/)([^/?#]*)/, (_match, scheme: string, authority: string) => { | ||
| const host = authority.slice(authority.lastIndexOf('@') + 1); | ||
| return `${scheme}${host}`.toLowerCase(); | ||
| }); | ||
| } | ||
|
|
||
| function decodeUrl(url: string): string { | ||
| try { | ||
| return decodeURIComponent(url); | ||
| } catch { | ||
| // Malformed percent-encoding, compare as is. | ||
| return url; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isnt this check redundant since we do an equal check on line 139?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question ๐ I rechecked and line 139 compares the raw URLs while this compares the normalised URLs