Skip to content

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

Merged
AbhiPrasad merged 2 commits intodevelopfrom
fn/astro-node
Mar 7, 2024
Merged

feat(astro): Update @sentry/astro to use OpenTelemetry#10960
AbhiPrasad merged 2 commits intodevelopfrom
fn/astro-node

Conversation

@mydea
Copy link
Copy Markdown
Member

@mydea mydea commented Mar 7, 2024 •
edited
Loading

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 AbhiPrasad, Lms24, lforst and s1gr1d March 7, 2024 10:53
@mydea mydea self-assigned this Mar 7, 2024
s1gr1d
s1gr1d approved these changes Mar 7, 2024
View reviewed changes
Copy link
Copy Markdown
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 :)

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 7, 2024 •
edited
Loading

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 mydea mentioned this pull request Mar 7, 2024
Merged
@mydea
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

timfish
timfish reviewed Mar 7, 2024
View reviewed changes
packages/node-experimental/src/transports/http-module.ts
@@ -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
Copy Markdown
Collaborator

@timfish timfish Mar 7, 2024

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 AbhiPrasad mentioned this pull request Mar 7, 2024
Merged
AbhiPrasad added a commit that referenced this pull request Mar 7, 2024
@AbhiPrasad
feat: Set required node version to >=14.18.0 for all packages (#10968)
fa19ce0 
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
Copy Markdown
Contributor

AbhiPrasad commented Mar 7, 2024

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
@AbhiPrasad AbhiPrasad mentioned this pull request Mar 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timfish timfish timfish left review comments

@s1gr1d s1gr1d s1gr1d approved these changes

@lforst lforst Awaiting requested review from lforst

@Lms24 Lms24 Awaiting requested review from Lms24

+1 more reviewer

@AbhiPrasad AbhiPrasad AbhiPrasad approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mydea mydea

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mydea @timfish @AbhiPrasad @s1gr1d