Skip to content
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

ref(opentelemetry): Remove usage of parseUrl #15954

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

AbhiPrasad
Copy link
Member

resolves #15939

parseUrl is expensive, so let's refactor to remove it. This makes the change for the opentelemetry package.

Removal of getSanitizedUrlString and stripUrlQueryAndFragment should also help with performance as we no longer have the js regex to compute.

@AbhiPrasad AbhiPrasad requested review from mydea and a team April 2, 2025 09:28
@AbhiPrasad AbhiPrasad self-assigned this Apr 2, 2025
@AbhiPrasad AbhiPrasad requested review from lforst and removed request for a team April 2, 2025 09:28
Copy link
Contributor

github-actions bot commented Apr 2, 2025

size-limit report 📦

Path Size % Change Change
@sentry/browser 23.2 KB - -
@sentry/browser - with treeshaking flags 23.02 KB - -
@sentry/browser (incl. Tracing) 36.76 KB - -
@sentry/browser (incl. Tracing, Replay) 73.92 KB - -
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 67.32 KB - -
@sentry/browser (incl. Tracing, Replay with Canvas) 78.59 KB - -
@sentry/browser (incl. Tracing, Replay, Feedback) 91.16 KB - -
@sentry/browser (incl. Feedback) 40.33 KB - -
@sentry/browser (incl. sendFeedback) 27.83 KB - -
@sentry/browser (incl. FeedbackAsync) 32.63 KB - -
@sentry/react 25 KB - -
@sentry/react (incl. Tracing) 38.68 KB - -
@sentry/vue 27.41 KB - -
@sentry/vue (incl. Tracing) 38.48 KB - -
@sentry/svelte 23.23 KB - -
CDN Bundle 24.44 KB - -
CDN Bundle (incl. Tracing) 36.78 KB - -
CDN Bundle (incl. Tracing, Replay) 71.8 KB - -
CDN Bundle (incl. Tracing, Replay, Feedback) 76.99 KB - -
CDN Bundle - uncompressed 71.24 KB - -
CDN Bundle (incl. Tracing) - uncompressed 108.76 KB - -
CDN Bundle (incl. Tracing, Replay) - uncompressed 220.05 KB - -
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 232.62 KB - -
@sentry/nextjs (client) 39.96 KB - -
@sentry/sveltekit (client) 37.2 KB - -
@sentry/node 143.01 KB +0.09% +128 B 🔺
@sentry/node - without tracing 96.2 KB +0.13% +120 B 🔺
@sentry/aws-serverless 120.56 KB +0.11% +130 B 🔺

View base workflow run

Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

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

Love this! One of the changes looks a tiny bit sus/buggy.

fragment: string | undefined;
hasRoute: boolean;
} {
): [parsedUrl: ReturnType<typeof parseStringToURLObject>, httpRoute: string | undefined] {
Copy link
Member

Choose a reason for hiding this comment

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

can we get the httpRoute logic out of here somehow? It seems like all we do is read the http.route attribute and return it.


if (!urlPath) {
if (!parsedUrl) {
return { ...getUserUpdatedNameAndSource(name, attributes), op: opParts.join('.') };
Copy link
Member

Choose a reason for hiding this comment

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

h: If we have an httpRoute but no parsedUrl we would return the original name here. I don't know if that is desired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove parseUrl from packages/opentelemetry
2 participants