feat: Add Sentry to the bundler plugins #224

nicholas-codecov · 2024-12-06T14:34:37Z

Description

This PR adds in the first iteration of Sentry to the bundler plugins, however, this PR does not include adding Sentry to the bundler-analyzer as that will come in a following PR as that requires a bit more work to sorted and this PR was getting big enough as it is.

Ticket: codecov/engineering-team#954

Notable Changes

Create Sentry helper utils and telemetry unplugin plugin - Followed along from: feat: Update Sentry telemetry to v8 getsentry/sentry-javascript-bundler-plugins#604
Add some spans to the Output to track some initial data points
Add telemetry plugin to all bundler plugins
- Disabled telemetry from running on the integration tests
Fix for NextJS plugin, where we weren't using the correct webpack as we should have been doing.
- This was included as it was causing type issues
Create/Update tests

codecov-notifications · 2024-12-06T14:38:33Z

Codecov Report

Attention: Patch coverage is 58.53270% with 260 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
packages/astro-plugin/src/index.ts	0.00%	27 Missing ⚠️
packages/remix-vite-plugin/src/index.ts	0.00%	27 Missing ⚠️
packages/solidstart-plugin/src/index.ts	0.00%	27 Missing ⚠️
packages/sveltekit-plugin/src/index.ts	0.00%	27 Missing ⚠️
packages/nuxt-plugin/src/index.ts	0.00%	26 Missing ⚠️
packages/vite-plugin/src/index.ts	0.00%	23 Missing ⚠️
packages/nextjs-webpack-plugin/src/index.ts	0.00%	20 Missing ⚠️
packages/rollup-plugin/src/index.ts	0.00%	18 Missing ⚠️
packages/webpack-plugin/src/index.ts	0.00%	18 Missing ⚠️
...ckages/bundler-plugin-core/src/sentry/telemetry.ts	84.82%	17 Missing ⚠️
... and 12 more

Components	Coverage Δ
Plugin core	`98.20% <93.25%> (-0.49%)`	⬇️
Rollup plugin	`8.69% <17.39%> (-0.89%)`	⬇️
Vite plugin	`8.69% <14.28%> (-1.13%)`	⬇️
Webpack plugin	`57.69% <16.00%> (-3.65%)`	⬇️

📢 Thoughts on this report? Let us know!

codecov · 2024-12-06T14:38:35Z

Codecov Report

Attention: Patch coverage is 58.53270% with 260 lines in your changes missing coverage. Please review.

Project coverage is 81.06%. Comparing base (0542128) to head (e167fcb).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
packages/astro-plugin/src/index.ts	0.00%	27 Missing ⚠️
packages/remix-vite-plugin/src/index.ts	0.00%	27 Missing ⚠️
packages/solidstart-plugin/src/index.ts	0.00%	27 Missing ⚠️
packages/sveltekit-plugin/src/index.ts	0.00%	27 Missing ⚠️
packages/nuxt-plugin/src/index.ts	0.00%	26 Missing ⚠️
packages/vite-plugin/src/index.ts	0.00%	23 Missing ⚠️
packages/nextjs-webpack-plugin/src/index.ts	0.00%	20 Missing ⚠️
packages/rollup-plugin/src/index.ts	0.00%	18 Missing ⚠️
packages/webpack-plugin/src/index.ts	0.00%	18 Missing ⚠️
...ckages/bundler-plugin-core/src/sentry/telemetry.ts	84.82%	17 Missing ⚠️
... and 12 more

Additional details and impacted files

Components	Coverage Δ
Plugin core	`98.20% <93.25%> (-0.49%)`	⬇️
Rollup plugin	`8.69% <17.39%> (-0.89%)`	⬇️
Vite plugin	`8.69% <14.28%> (-1.13%)`	⬇️
Webpack plugin	`57.69% <16.00%> (-3.65%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

codecov-staging · 2024-12-06T14:59:01Z

Bundle Report

Changes will increase total bundle size by 10.36kB (0.17%) ⬆️. This is within the configured threshold ✅

Detailed changes

Bundle name	Size	Change
@codecov/bundler-plugin-core-cjs	64.28kB	5.25kB (8.89%) ⬆️
@codecov/bundler-plugin-core-esm	14.37kB	1.94kB (15.6%) ⬆️
@codecov/vite-plugin-cjs	3.14kB	303 bytes (10.67%) ⬆️
@codecov/nuxt-plugin-esm	855 bytes	372 bytes (-30.32%) ⬇️
@codecov/nextjs-webpack-plugin-esm	2.3kB	429 bytes (22.88%) ⬆️
@codecov/webpack-plugin-cjs	4.96kB	312 bytes (6.72%) ⬆️
@codecov/solidstart-plugin-cjs	1.69kB	368 bytes (27.75%) ⬆️
@codecov/sveltekit-plugin-cjs	1.69kB	367 bytes (27.8%) ⬆️
@codecov/sveltekit-plugin-esm	891 bytes	198 bytes (-18.18%) ⬇️
@codecov/remix-vite-plugin-esm	1.43kB	341 bytes (31.28%) ⬆️
@codecov/nuxt-plugin-cjs	1.74kB	337 bytes (24.05%) ⬆️
@codecov/nextjs-webpack-plugin-cjs	2.58kB	471 bytes (22.3%) ⬆️
@codecov/webpack-plugin-esm	3.39kB	209 bytes (-5.81%) ⬇️
@codecov/rollup-plugin-cjs	3.17kB	316 bytes (11.07%) ⬆️
@codecov/remix-vite-plugin-cjs	1.68kB	363 bytes (27.65%) ⬆️
@codecov/astro-plugin-cjs	1.68kB	337 bytes (25.04%) ⬆️
@codecov/example-sveltekit-app-client-esm	727.49kB	2 bytes (0.0%) ⬆️
@codecov/example-sveltekit-app-server-esm	984.45kB	1 bytes (0.0%) ⬆️

codecov · 2024-12-06T14:59:37Z

Bundle Report

Changes will decrease total bundle size by 30.43kB (-0.48%) ⬇️. This is within the configured threshold ✅

Detailed changes

Bundle name	Size	Change
@codecov/vite-plugin-cjs	3.14kB	303 bytes (10.67%) ⬆️
@codecov/remix-vite-plugin-cjs	1.68kB	363 bytes (27.65%) ⬆️
@codecov/bundler-plugin-core-cjs	64.28kB	5.25kB (8.89%) ⬆️
@codecov/bundler-plugin-core-esm	14.37kB	38.79kB (-72.96%) ⬇️
@codecov/rollup-plugin-cjs	3.17kB	316 bytes (11.07%) ⬆️
@codecov/nuxt-plugin-cjs	1.74kB	337 bytes (24.05%) ⬆️
@codecov/rollup-plugin-esm	1.3kB	1.07kB (-45.02%) ⬇️
@codecov/nuxt-plugin-esm	1.54kB	315 bytes (25.67%) ⬆️
@codecov/sveltekit-plugin-cjs	1.69kB	367 bytes (27.8%) ⬆️
@codecov/webpack-plugin-cjs	4.96kB	312 bytes (6.72%) ⬆️
@codecov/solidstart-plugin-cjs	1.69kB	368 bytes (27.75%) ⬆️
@codecov/solidstart-plugin-esm	1.44kB	346 bytes (31.71%) ⬆️
@codecov/example-sveltekit-app-client-esm	727.49kB	2 bytes (-0.0%) ⬇️
@codecov/example-sveltekit-app-server-esm	984.45kB	1 bytes (-0.0%) ⬇️
@codecov/nextjs-webpack-plugin-cjs	2.58kB	471 bytes (22.3%) ⬆️
@codecov/bundle-analyzer-esm	3.45kB	347 bytes (11.17%) ⬆️
@codecov/astro-plugin-cjs	1.68kB	337 bytes (25.04%) ⬆️

Lms24

Hey, I heard you were looking for a review from the JS SDK team, so I took a look (also, sorry for the delay!). I think in general, this LGTM. We're taking care of removing stack traces and potential PII and the client/scope handling seems to be mostly in line with the Sentry bundler plugins 👍

Are there specific questions or things you'd like us to look at in detail?

.changeset/green-goats-thank.md

Co-authored-by: Lukas Stracke <[email protected]>

…e thrown

suejung-sentry

Had a couple questions, but looked good!

suejung-sentry · 2025-01-07T18:15:57Z

packages/astro-plugin/src/index.ts

+      pluginVersion: PLUGIN_VERSION,
+      options,
+      bundler: unpluginMetaContext.framework,
+      metaFramework: "astro",


What do these 2 keys represent again? bundler vs. metaFramework? On first glance, I would have expected the values to be flipped here with bundler being "astro" and the metaFramework being from the enum from unplugin.

Bundler is the underlying bundler (Rollup, Vite, Webpack), this value is derived from Unplugin itself, whereas the meta-framework value is what we pass through as things like Astro are built on-top of bundlers (Vite in this case), bundlers being the "lowest" system in the stack.

Got it, thanks!
If we can declare an enum or type for the acceptable strings that may be nice in the future.

suejung-sentry · 2025-01-07T18:20:42Z

packages/bundler-plugin-core/src/sentry/telemetry.ts

+    platform: "node",
+    runtime: { name: "node", version: global.process.version },
+
+    dsn: "https://[email protected]/4506739665207296",


Any thoughts on making a network call to Codecov to fetch this dsn? If we hardcode it here, we won't be able to change it for anyone using old versions of the plugin. Maybe overkill since it will make it slower and add more failure pathways, though.
Do we have any other examples of how it's done around in Sentry as guidance perhaps?

I'd like to avoid any extra calls, as well as the work to maintain an endpoint to distribute this value. Typically for any client based application the DSN is just shipped with the package, this is how it is done for their bundler plugins.

Can you expand upon your concerns around users using older versions of the plugin?

Okay great, good to know that's the usual pattern!

Oh yeah re: older versions of plugin - I mean say version 1 has this DSN hardcoded and that's what they're running. Say a year down the road we need to change the way we configure things and we have a new DSN for our org the old one gets deprecated, and hardcode the new one in version 2. Anyone still on version 1 is broken until they upgrade.
Basically backwards compatibility can potentially be annoying. This is probably overengineering for where we are though so we can go with hardcoding!

suejung-sentry · 2025-01-07T18:24:25Z

packages/webpack-plugin/src/index.ts

+        telemetryPlugin({
+          sentryClient: sentryConfig.sentryClient,
+          sentryScope: sentryConfig.sentryScope,
+          shouldSendTelemetry: options.telemetry,


nit: I've lost track a little on why different keys of telemetry, enableTelemetry and shouldSendTelemetry across the different layers. Was it intentional to name them differently? Also should we create and push this plugin into the list at all if it's not enabled? Maybe missing something on that

okay i've gone through and went with telemetry for everything ... this has been in progress for too long I guess I started coming up with new names 😭

Ah nice, sorry for any trouble.
That looks good!
My other thought in there was buried - what do you think RE: only creating/pushing the plugin into the list if it's enabled (to save running any unneeded code)?
We can do that in a separate pass if we think it's worthwhile

suejung-sentry · 2025-01-07T18:28:12Z

packages/bundler-plugin-core/src/utils/Output.ts

+
+interface SentryConfig {
+  sentryClient?: Client;
+  sentryScope?: Scope;


nit: thoughts on naming this as client and scope since they are already in something named SentryConfig?

Hrrmrmrmrm, for this one, I think i'll leave it as is, i tend to lean towards more verbose naming. We may be looking at adding in amplitude and if we do there may be more clients (idk about scopes) but this keeps it a bit clearer as to what's going on with these.

Sure sounds good - thanks!

suejung-sentry · 2025-01-07T18:33:33Z

packages/bundler-plugin-core/src/utils/Output.ts

-      }
-      debug(`Error uploading stats: "${error}"`, { enabled: this.debug });
-      return;
+      throw error;


Was it intentional to remove the if (emitError) throw error check here? It's possible more things will slip through to throw that used to be "swallowed" with this change I think, in case that has any unintended effects.

Uhh this was a newly added in throw, sooooo nice catch and my bad 😅

Oh okay sounds good! I think either way works if we have some errors we treat as more "catastrophic". Adding the check here makes sense - thanks!

suejung-sentry · 2025-01-07T18:38:31Z

packages/bundler-plugin-core/src/sentry/transports.ts

+      },
+    };
+  };
+}


nice, these look tricky

#thanks-sentry 💪

nicholas-codecov requested review from suejung-sentry and AbhiPrasad December 6, 2024 14:35

Lms24 reviewed Jan 2, 2025

View reviewed changes

.changeset/green-goats-thank.md Outdated Show resolved Hide resolved

nicholas-codecov and others added 13 commits January 2, 2025 08:45

refactor how we define and access the plugin name and version

fd4fcb7

setup Sentry in the bundler plugin core

d83cb03

add to the plugins

26b4a73

disable telemetry in the integration tests

bd34936

update lock file

b9d057d

add in changeset

56421c7

fix type issues

5f6b20e

add in fix for next js webpack plugin not using the correct webpack

4c6b2e7

set owner and repo tags

937c3cd

add op's to the spans

8e60f72

collect some more info about meta frameworks and bundlers

92352c3

add in some more comments

83c8e0c

Update .changeset/green-goats-thank.md

d04efd9

Co-authored-by: Lukas Stracke <[email protected]>

nicholas-codecov force-pushed the gh-eng-954-add-sentry-to-plugins branch from 0103d28 to d04efd9 Compare January 2, 2025 14:20

actually send the sentry config to the Output constructor

a8bfd11

Lms24 mentioned this pull request Jan 3, 2025

fix(core): Fork scope if custom scope is passed to startSpan getsentry/sentry-javascript#14900

Merged

nicholas-codecov added 7 commits January 6, 2025 07:15

force transaction span wrapping all of output.write method

e23ed03

default values to 0 not undefined

04dcead

manually set the parent span

6343834

mock out startSpan from Sentry

5b16c22

remove mocks for getScope and clone

c26a9e1

wrap outer span in a try catch to actually throw any errors if they'r…

11ac6b9

…e thrown

bring back early returns if no provider or upload url

e5ba3cc

suejung-sentry approved these changes Jan 7, 2025

View reviewed changes

nicholas-codecov added 2 commits January 8, 2025 08:09

add in emitError wrapper

e31749a

use common naming for telemetry

e167fcb

nicholas-codecov mentioned this pull request Jan 8, 2025

feat: Add sentry to bundler plugins #72

Closed

nicholas-codecov merged commit 01de9e5 into main Jan 8, 2025
65 of 69 checks passed

nicholas-codecov deleted the gh-eng-954-add-sentry-to-plugins branch January 8, 2025 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Sentry to the bundler plugins #224

feat: Add Sentry to the bundler plugins #224

nicholas-codecov commented Dec 6, 2024 •

edited

Loading

codecov-notifications bot commented Dec 6, 2024 •

edited

Loading

codecov bot commented Dec 6, 2024 •

edited

Loading

codecov-staging bot commented Dec 6, 2024 •

edited

Loading

codecov bot commented Dec 6, 2024 •

edited

Loading

Lms24 left a comment

suejung-sentry left a comment

suejung-sentry Jan 7, 2025

nicholas-codecov Jan 8, 2025

suejung-sentry Jan 8, 2025

suejung-sentry Jan 7, 2025

nicholas-codecov Jan 8, 2025

suejung-sentry Jan 8, 2025

suejung-sentry Jan 7, 2025

nicholas-codecov Jan 8, 2025

suejung-sentry Jan 8, 2025

suejung-sentry Jan 7, 2025

nicholas-codecov Jan 8, 2025

suejung-sentry Jan 8, 2025

suejung-sentry Jan 7, 2025

nicholas-codecov Jan 8, 2025

suejung-sentry Jan 8, 2025

suejung-sentry Jan 7, 2025

nicholas-codecov Jan 8, 2025

feat: Add Sentry to the bundler plugins #224

feat: Add Sentry to the bundler plugins #224

Conversation

nicholas-codecov commented Dec 6, 2024 • edited Loading

Description

Notable Changes

codecov-notifications bot commented Dec 6, 2024 • edited Loading

Codecov Report

codecov bot commented Dec 6, 2024 • edited Loading

Codecov Report

codecov-staging bot commented Dec 6, 2024 • edited Loading

Bundle Report

codecov bot commented Dec 6, 2024 • edited Loading

Bundle Report

Lms24 left a comment

Choose a reason for hiding this comment

suejung-sentry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholas-codecov commented Dec 6, 2024 •

edited

Loading

codecov-notifications bot commented Dec 6, 2024 •

edited

Loading

codecov bot commented Dec 6, 2024 •

edited

Loading

codecov-staging bot commented Dec 6, 2024 •

edited

Loading

codecov bot commented Dec 6, 2024 •

edited

Loading