Skip to content

feat(sveltekit): Add partial instrumentation for client-side fetch - #7626

Merged
Lms24 merged 1 commit into
developfrom
lms/sveltekit-patch-client-load-fetch
Apr 5, 2023
Merged

feat(sveltekit): Add partial instrumentation for client-side fetch#7626
Lms24 merged 1 commit into
developfrom
lms/sveltekit-patch-client-load-fetch

Conversation

@Lms24

@Lms24Lms24 commented Mar 27, 2023•
edited
Loading

Copy link
Copy Markdown
Member

This PR adds partial instrumentation to the client-side fetch passed to the universal load functions. It enables distributed traces of fetch calls happening inside a load function.

Limitation: fetch requests made by SvelteKit (e.g. to call server-only load functions) are not touched by this instrumentation because we cannot access the Kit-internal fetch function at this time. Opened an issue on the SvelteKit repo to find a solution for this.

Anyway, here's a fancy screenshot of a successful trace:

image

My guess is that sveltejs/kit#9542 and sveltejs/kit#9530 are gonna take a little while longer to be resolved. I'd say, we add this in for the moment so that we at least get these traces connected. We should take it out/adjust whenever we have handleLoad or handleFetch available.

ref: #7413

@Lms24Lms24 changed the title feat(sveltekit): Add instrumentation for client-side fetchfeat(sveltekit): Add partial instrumentation for client-side fetchMar 27, 2023
@Lms24Lms24 mentioned this pull request Mar 27, 2023
Closed
@github-actions

github-actionsBot commented Mar 27, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
@sentry/browser - ES5 CDN Bundle (gzipped + minified)20.74 KB (+0.06% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)64.77 KB (+0.03% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)19.3 KB (+0.07% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)57.25 KB (+0.03% 🔺)
@sentry/browser - Webpack (gzipped + minified)20.85 KB (+0.06% 🔺)
@sentry/browser - Webpack (minified)68.21 KB (+0.03% 🔺)
@sentry/react - Webpack (gzipped + minified)20.88 KB (+0.06% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified)48.7 KB (+0.03% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)28.32 KB (+0.05% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)26.54 KB (+0.05% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified)44.88 KB (+0.01% 🔺)
@sentry/replay - Webpack (gzipped + minified)38.86 KB (0%)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)63.59 KB (+0.02% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified)56.63 KB (+0.03% 🔺)

AbhiPrasad
AbhiPrasad reviewed Mar 27, 2023
View reviewed changes
Comment threadpackages/utils/src/tracing.ts Outdated
@Lms24Lms24 mentioned this pull request Mar 28, 2023
Merged
@Lms24
Lms24force-pushed the lms/sveltekit-patch-client-load-fetch branch 2 times, most recently from 51eca27 to ab20d0bCompareMarch 31, 2023 10:36
github-advanced-security[bot]
github-advanced-securityAI found potential problems Apr 3, 2023
View reviewed changes
Comment threadpackages/utils/src/url.ts Fixed
github-advanced-security[bot]
github-advanced-securityAI found potential problems Apr 3, 2023
View reviewed changes
Comment threadpackages/utils/src/url.ts Fixed
@Lms24
Lms24force-pushed the lms/sveltekit-patch-client-load-fetch branch 2 times, most recently from ea8a521 to df10ac4CompareApril 3, 2023 12:48
Lms24
Lms24 commented Apr 3, 2023
View reviewed changes
Comment threadpackages/sveltekit/src/client/load.ts Outdated
...(urlObject.hash && { 'http.hash': urlObject.hash.substring(1) }),
};

// TODO: extract this to a util function (and use it in breadcrumbs integration as well)

@Lms24Lms24Apr 3, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Will do this in a separate PR as this spans multiple packages

Comment threadpackages/sveltekit/test/client/load.test.ts Outdated
Lms24
Lms24 commented Apr 3, 2023
View reviewed changes
Comment threadpackages/utils/src/url.ts
(host &&
host
// Always filter out authority
.replace(/^.*@/, '[filtered]:[filtered]@')

@Lms24Lms24Apr 3, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This ain't pretty, I know... turns out finding a proper authority regex that isn't ReDos vulnerable is very hard. I think we can leave this as is for now and maybe revisit whenever we sanitize URLs everywhere on the browser side.
(I'd argue that this function should nevertheless be a good base we can reuse later on).

@Lms24Lms24 mentioned this pull request Apr 3, 2023
Merged
@Lms24
Lms24 marked this pull request as ready for review April 3, 2023 14:23
@Lms24
Lms24 requested a review from AbhiPrasadApril 3, 2023 14:24
mydea
mydea reviewed Apr 4, 2023
View reviewed changes
Comment threadpackages/sveltekit/src/client/load.ts Outdated
mydea
mydea reviewed Apr 4, 2023
View reviewed changes
Comment threadpackages/sveltekit/src/client/load.ts Outdated
mydea
mydea reviewed Apr 4, 2023
View reviewed changes
Comment threadpackages/sveltekit/src/client/load.ts Outdated
AbhiPrasad
AbhiPrasad reviewed Apr 4, 2023
View reviewed changes
Comment threadpackages/sveltekit/src/client/load.ts
Comment threadpackages/tracing-internal/src/browser/request.ts Outdated
Comment threadpackages/sveltekit/src/client/load.ts
mydea
mydea reviewed Apr 4, 2023
View reviewed changes
Comment threadpackages/sveltekit/src/client/load.ts Outdated
@Lms24
Lms24force-pushed the lms/sveltekit-patch-client-load-fetch branch from 7cafcd7 to 5f05517CompareApril 4, 2023 11:41
@Lms24
Lms24 requested review from AbhiPrasad and mydeaApril 4, 2023 14:56
AbhiPrasad
AbhiPrasad approved these changes Apr 5, 2023
View reviewed changes

@AbhiPrasadAbhiPrasad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:shipit:

@Lms24
feat(sveltekit): Add instrumentation for client-side fetch
0314949
properly sanitize url, add url data
apply data to breadcrumbs
cleanup
set span status
add tests
sorry
fix authority url
"fix" authority regex again
cleanup
move addTracingHeadersToFetchRequest back to tracing-internal
cleanup
apply suggestions
re-activate request object test
adjust types after exporting
@Lms24
Lms24force-pushed the lms/sveltekit-patch-client-load-fetch branch from b7ce245 to 0314949CompareApril 5, 2023 11:23
@Lms24
Lms24 merged commit 105dcf1 into developApr 5, 2023
@Lms24
Lms24 deleted the lms/sveltekit-patch-client-load-fetch branch April 5, 2023 11:44
@Lms24Lms24 self-assigned this Apr 5, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mydeamydeamydea approved these changes
+1 more reviewer
@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@Lms24Lms24

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Lms24@mydea@AbhiPrasad@github-advanced-security