getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
8.02k stars 1.59k forks source link

@sentry/astro hard-codes DSN during build stage for output:server with node #12412

Closed arty-name closed 2 days ago

arty-name commented 5 months ago

Is there an existing issue for this?

How do you use Sentry?

Self-hosted/on-premise

Which SDK are you using?

@sentry/astro

SDK Version

8.8.0

Framework Version

4.10.0

Link to Sentry event

No response

SDK Setup

  output: 'server',
  adapter: node({ mode: 'standalone' }),
  integrations: [sentry({ enabled: true })],

Steps to Reproduce

  1. Run astro build without providing DSN
  2. Run PUBLIC_SENTRY_DSN=FAIL astro preview

Or visit the minimal reproduction at https://stackblitz.com/edit/github-5bh1gz?file=package.json

Expected Result

Sentry should log Invalid Sentry Dsn: FAIL

Actual Result

Sentry ignores the provided DSN and does not activate.

Sentry logs this error if you provide PUBLIC_SENTRY_DSN=FAIL during the build stage. Providing a correct DSN at runtime does not fix that.

Lms24 commented 5 months ago

Hey @arty-name thanks for writing in!

If I understand correctly, the issue seems to be that when we generate the server-side Sentry.init code, we use import.meta.env.PUBLIC_SENTRY_DSN, which, during build already, is replaced by the environment variable's value. So you're not able to override the value during runtime with PUBLIC_SENTRY_DSN. Is this correct?

If so, I think we could get away with replacing import.meta.env with process.env but not sure yet

arty-name commented 5 months ago

You understood the issue correctly, Lukas!

I do believe that the change to process.env should help. By the way, in case you haven’t stumbled on this yet, Astro node integration handles the environment variables in a peculiar manner.

Lms24 commented 5 months ago

That's peculiar indeed and I wasn't aware of this at all 😬 I guess we need to try if switching to process.env would make any difference at all then. But ultimately, I believe this is only something users can control.

getsantry[bot] commented 1 week ago

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

arty-name commented 4 days ago

I don’t know why "waiting for community" label was added here. I have just verified that even with the latest versions of dependencies the error still happens:


    "@astrojs/node": "^8.3.4",
    "@sentry/astro": "^8.40.0",
    "astro": "^4.16.15"
chargome commented 2 days ago

Hey @arty-name, passing the DSN directly to our astro integration will be deprecated soon (https://github.com/getsentry/sentry-javascript/pull/14489), instead we encourage users to create runtime specific config files for client and server respectively.

arty-name commented 2 days ago

Thank you for taking time to reply @chargome but I do not pass the DSN to the integration

chargome commented 2 days ago

@arty-name yep I saw that, but the recommended setup for the server instrumentation is creating a sentry.sever.config.js file where you add:

import * as Sentry from "@sentry/astro";

Sentry.init({
  dsn: // get your dsn here

  // Define how likely traces are sampled. Adjust this value in production,
  // or use tracesSampler for greater control.
  tracesSampleRate: 1.0,
});

So here you can get the dsn whichever way you want.

arty-name commented 2 days ago

Has the previously suggested approach of providing the environment variable PUBLIC_SENTRY_DSN been deprecated as well?

chargome commented 2 days ago

Yes inherently, since providing a sentry.sever.config.js will prevent the integration from automatically creating this file. We took this step because providing the config values to the integration introduces intransparent behaviour which we want to prevent. I hope this setup does not cause you too much extra hassle!

arty-name commented 2 days ago

Thank you for your patience and detailed explanations @chargome! I guess I will close this ticket then.