Skip to content

feat(astro): Update @sentry/astro to use OpenTelemetry #10960

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

Merged
merged 2 commits into from
Mar 7, 2024

Conversation

mydea
Copy link
Member

@mydea mydea commented Mar 7, 2024

Let's see how this goes, as a first meta SDK using OTEL under the hood! 🤞

For now I decided to remove the Prisma integration in Astro as that leads to weird build issues etc....

@mydea mydea requested review from lforst, Lms24, AbhiPrasad and s1gr1d March 7, 2024 10:53
@mydea mydea self-assigned this Mar 7, 2024
Copy link
Member

@s1gr1d s1gr1d left a comment

Choose a reason for hiding this comment

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

Looks good from my side :)

Copy link
Contributor

github-actions bot commented Mar 7, 2024

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped) 77.56 KB (0%)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 68.8 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing, Replay with Canvas) - Webpack (gzipped) 72.7 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 62.35 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 33.03 KB (-0.02% 🔽)
@sentry/browser (incl. browserTracingIntegration) - Webpack (gzipped) 33.03 KB (-0.02% 🔽)
@sentry/browser (incl. Feedback) - Webpack (gzipped) 31.36 KB (0%)
@sentry/browser (incl. sendFeedback) - Webpack (gzipped) 31.37 KB (0%)
@sentry/browser - Webpack (gzipped) 22.57 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) - CDN Bundle (gzipped) 75.63 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay) - CDN Bundle (gzipped) 67.24 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing) - CDN Bundle (gzipped) 33.09 KB (+0.03% 🔺)
@sentry/browser - CDN Bundle (gzipped) 24.01 KB (0%)
@sentry/browser (incl. Tracing, Replay) - CDN Bundle (minified & uncompressed) 211.07 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing) - CDN Bundle (minified & uncompressed) 99.87 KB (+0.06% 🔺)
@sentry/browser - CDN Bundle (minified & uncompressed) 71.76 KB (0%)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 69.05 KB (+0.01% 🔺)
@sentry/react - Webpack (gzipped) 22.6 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 85.52 KB (+0.05% 🔺)
@sentry/nextjs Client - Webpack (gzipped) 49.93 KB (+0.07% 🔺)
@sentry-internal/feedback - Webpack (gzipped) 17.41 KB (0%)

@mydea
Copy link
Member Author

mydea commented Mar 7, 2024

Update: while tests pass here, I could not get this to really run in a local test app.
First, it requires #10928 to even run at all. Even then, when running astro build I get an error:

16:18:36 [ERROR] [vite] x Build failed in 512ms
[commonjs--resolver] Failed to resolve entry for package "https". The package may have incorrect main/module/exports specified in its package.json.

So that's one thing.
The other thing is that it does not seem to auto instrument, which I guess is related to it being ESM, which means we'd need to provide an experimental loader, which I'm not sure how do to for an Astro app (e.g. astro dev --experimental-loader=.... does not work)

@timfish
Copy link
Collaborator

timfish commented Mar 7, 2024

which means we'd need to provide an experimental loader,

I'm looking to add that once #10928 is merged since it adds to the package exports that get added there

@mydea
Copy link
Member Author

mydea commented Mar 7, 2024

Update: importing https from node:https instead of https seems to fix this specific build error. still no auto instrumentation though.

@@ -1,5 +1,5 @@
import type { ClientRequest, IncomingHttpHeaders, RequestOptions as HTTPRequestOptions } from 'http';
import type { RequestOptions as HTTPSRequestOptions } from 'https';
import type { ClientRequest, IncomingHttpHeaders, RequestOptions as HTTPRequestOptions } from 'node:http';
Copy link
Collaborator

Choose a reason for hiding this comment

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

These ones shouldn't matter since they're type imports and wont end up in the output

AbhiPrasad added a commit that referenced this pull request Mar 7, 2024
we would like to start using node protocol imports in the SDK to unblock
#10960 and
#10928

This requires us to have a minimum supported Node version of `14.18.0`
as per https://2ality.com/2021/12/node-protocol-imports.html
@AbhiPrasad
Copy link
Member

I know auto-instrumentation is broken with this but

a) I need this to start experimenting with other PRs + we can test out more ESM stuff
b) we can solve these issues before the next alpha release.

hence I'm going to go ahead and merge this.

@AbhiPrasad AbhiPrasad merged commit 7c655fb into develop Mar 7, 2024
@AbhiPrasad AbhiPrasad deleted the fn/astro-node branch March 7, 2024 22:22
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.

4 participants