Skip to content

Conversation

@lforst
Copy link
Contributor

@lforst lforst commented Aug 29, 2023

No description provided.

github-actions bot and others added 8 commits August 28, 2023 13:12
[Gitflow] Merge master into develop
While looking into logger issues, I noticed that we fill console.xxx
multiple times. This PR changes that so that we use the console
instrumentation from utils in all cases.
…mentation (#8881)

In our auto instrumentation Vite plugin for SvelteKit, we read
`+(page|layout)(.server).(js|ts)` files' code to determine if we should
add our wrapper to the file or not. We previously didn't check for a
file id's existence before reading the file if the id matched that
certain pattern, wrongly assuming that these ids would always map to
actually existing files.

It seems like Vite plugins such as Houdini's plugin add file ids to the
build for files that actually don't exist (#8846, #8854) . When our
plugin's `load` hook was called for such an id, it then tried to access
and read the file which caused a build error.

This patch now adds a file existence guard to ensure we simply no-op for
these files.
Noticed that this is a bit tightly coupled in the browser client, and
could be simplified by using a hook.
This adds an `origin` to all spans, which defaults to `manual` but is
set to something more meaningful for all our auto instrumentation.

I tried to come up with reasonable origin names, I hope it makes sense
everywhere 😅 Also note that this now uses a new TS feature which seems
to be correctly transpiled to TS 3.8, as far as I can tell! 🎉

Closes #8510

---------

Co-authored-by: Luca Forstner <[email protected]>
Co-authored-by: Lukas Stracke <[email protected]>
@lforst lforst requested review from AbhiPrasad, Lms24 and mydea August 29, 2023 13:37
@github-actions
Copy link
Contributor

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 75.27 KB (+0.06% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 31.25 KB (+0.09% 🔺)
@sentry/browser - Webpack (gzipped) 21.86 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 69.79 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 28.2 KB (+0.08% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped) 20.19 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 220.3 KB (+0.13% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 85.13 KB (+0.33% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 59.84 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 31.1 KB (+0.09% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 75.29 KB (+0.06% 🔺)
@sentry/react - Webpack (gzipped) 21.89 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 93.13 KB (+0.06% 🔺)
@sentry/nextjs Client - Webpack (gzipped) 50.8 KB (+0.1% 🔺)

@AbhiPrasad
Copy link
Member

Why is this a minor? There are only fixes here.

Comment on lines +12 to +14
- ref: Use consistent console instrumentation (#8879)
- ref(browser): Refactor sentry breadcrumb to use hook (#8892)
- ref(tracing): Add `origin` to spans (#8765)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- ref: Use consistent console instrumentation (#8879)
- ref(browser): Refactor sentry breadcrumb to use hook (#8892)
- ref(tracing): Add `origin` to spans (#8765)
- ref(tracing): Add `origin` to spans (#8765)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to leave these in because they non-trivially touch our console instrumentation and somebody with issues might find the changelog entry useful (in case there are any issues).

But I can remove the entries if you feel strongly about it.

@lforst
Copy link
Contributor Author

lforst commented Aug 29, 2023

Why is this a minor? There are only fixes here.

@AbhiPrasad #8892 expands the API

@lforst lforst merged commit 6272f7c into master Aug 29, 2023
@lforst lforst deleted the prepare-release/7.66.0 branch August 29, 2023 14:45
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.

5 participants