RegEx in tracingOrigins config breaks ember build

[dagroe]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which package are you using?

@sentry/ember

SDK Version

7.14.1

Framework Version

ember 4.7.0

Link to Sentry event

No response

Steps to Reproduce

1. Create new app using ember-cli 4.7.0 ember new ember-quickstart --lang en
2. install sentry SDK ember install @sentry/ember
3. add config to config/environment.js, make sure to include RegEx in tracingOrigins, e.g.

  ENV['@sentry/ember'] = {
    sentry: {
      tracesSampleRate: 1.0,
      debug: false,
      browserTracingOptions: {
        tracingOrigins: [
          'localhost',
          /^\//,
        ],
      },
    },
  };

4. run ember ember serve

Expected Result

ember builds and runs app

Actual Result

build fails with error

non serializable item found in config: /^\//
[Embroider:MacrosConfig] the given config from '/YOUR_PATH/node_modules/@sentry/ember' for packageName 'undefined' is not JSON serializable.

[AbhiPrasad]

Hey @dagroe, the config will break since it only accepts serializable options. To have more advanced options, you'll probably need to initialize Sentry yourself as per https://docs.sentry.io/platforms/javascript/guides/ember/#configure cc @k-fish.

[dagroe]

Hi @AbhiPrasad, sorry I forgot to add that the above RegEx is suggested in the sentry docs:
https://docs.sentry.io/platforms/javascript/guides/ember/performance/instrumentation/automatic-instrumentation/

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

Sentry.init({
  dsn: "https://examplePublicKey@o0.ingest.sentry.io/0",

  // To set a uniform sample rate
  tracesSampleRate: 0.2,

  // Configuration for Ember's default tracing integration.
  browserTracingOptions: {
    tracingOrigins: ["localhost", "my-site-url.com", /^\//],
  },
});

[AbhiPrasad]

Ah good catch - we need to make sure this doesn't break.

[k-fish]

It should be fine using the regex as is recommended in the docs, using Sentry.init and calling it within the application. Using Ember config to pass Sentry options can't serialize functions / objects etc. which is why the regex is breaking. Try switching to pass the Sentry options via Sentry.init?

@AbhiPrasad, we should probably have a deprecated warning for passing the sentry options via config, considering it's come up before.

[mydea]

While I think it is a good idea to deprecate ENV['@sentry/ember'].sentry, and this can pretty easily be substituted with Sentry.init(), one thing we may want to to allow is to define dsn in environment.js. As it is probably the most common scenario to set this via process.env or similar, it makes sense to allow to define it there, vs. manually piping it through and set it in app.js. Maybe add a new dsn option to ENV['@sentry/ember']?

[k-fish]

@mydea yeah that's not a bad idea. For context there is a .sentry in the first place since I was trying to separate the "sentry" config from the "sentry/ember" addon config for clarity/consistency between our sdks, but I agree most people will probably be piping dsn in. To keep the "options" somewhat split, perhaps we don't have to fully deprecate ENV['@sentry/ember'].sentry but throw a more explicit warning to not use more than a select few fields / serializable objects in there?

Thoughts?

[mydea]

I'd say both are possible.

We can also leverage (=copy) the validation that also happens in embroider (which, incidentally, I wrote there for exactly this issue 😂 see embroider-build/embroider#1083). but, as you can see there, this is not entirely straightforward, and would also kind of duplicate all of this functionality/checks. But might be a bit more "user friendly" this way, as theoretically you may also want to set other config based on env (e.g. debug, or similar).

Anyhow, for me both approaches seem OK! The more user friendly would be to validate it and throw a nicer error message, I guess.