rfc(decision): Environment Variable conventions#121
Conversation
cleptric
force-pushed
the
rfc/draft-environment-variable-conventions
branch
from
4e2da89 to
be30ec7
Compare
cleptric
changed the title
There was a problem hiding this comment.
General:
I think this makes sense and at least partially enables easier configuration of the SDK.
I have an important question though: Is the goal of supporting env variables to read them at SDK init time and store the values as long as the application is alive? Or should SDKs also observe updates to env variables after they're initialized?
- If only at init time, all good, we can make this work fairly easily.
- In case of the latter, we need to be aware though that this has deeper implications on how SDKs currently deal with options. For instance, we currently don't guarantee support for "updating" of
getClient().getOptions(). It's possible (at least in JS) but very likely, we have some instrumentation (e.g. an integration) that hard-copied e.g. sample rates when they initialized, stored them away and wouldn't receive the update value. If we were to support dynamic updates of options, we need to ensure that this doesn't happen anywhere. IOW, we'll need to re-query the env variables a lot.
Also, users (and we) need to be aware that setting options via env variables will only work for primitive values. Adding integrations or callbacks (beforeSend) like this will not work.
JS Specific and also dependent on my initial question:
- Since Browser and Server SDKs inherit from a core SDK package, we need to find a way to a) only add env variable reads in the server side or b) make these reads a no-op in the browser
- Both but especially b) will have a bundle size impact. Negligible in server-land, something we need to keep an eye on in browser-land.
|
Just for init, everything else, as you rightly pointed out, has too many side effects. |
|
Sorry, tapped something while reviewing that marked it as ready and I don't see a way to unmark it on GH mobile now. |
| - `SENTRY_DSN` | ||
| - `SENTRY_RELEASE` | ||
| - `SENTRY_ENVIRONMENT` | ||
| - `SENTRY_SAMPLE_RATE` | ||
| - `SENTRY_TRACES_SAMPLE_RATE` | ||
| - `SENTRY_PROFILES_SAMPLE_RATE` | ||
| - `SENTRY_DEBUG` |
There was a problem hiding this comment.
Just stumbled across this RFC. We could also list SENTRY_TRACE and SENTRY_BAGGAGE, which could be picked up to continue traces.
There was a problem hiding this comment.
Yep, there is/was another RFC for that, but I can add it and actually move this RFC forward 😬
|
I rebased and resolved merge conflicts. About the drawbacks - there are only drawbacks when rolling out, that not all SDKs have this just yet. @cleptric any suggestions how to move this forward? |
JPeer264
force-pushed
the
rfc/draft-environment-variable-conventions
branch
from
a295f11 to
1b70176
Compare
|
We should port the content to the develop docs. |
…16326) <!-- Use this checklist to make sure your PR is ready for merge. You may delete any sections you don't need. --> ## DESCRIBE YOUR PR This adds a common understanding of which environment variables should be available across all SDKs (origin of the idea: getsentry/rfcs#121). I added couple of SDKs with a tiny example, so we don't show just one There are still options missing, which could be added in the future (or now if we are already sure it is a good idea to add now): - Logs - Metrics - Tags - `SENTRY_BREADCRUMBS_LOGS_ENABLED` - `SENTRY_TRACE_MISSING_ROUTES_ENABLED` Following was taken as example: https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/ (added @Lms24 because of the client-side specifics) ## IS YOUR CHANGE URGENT? Help us prioritize incoming PRs by letting us know when the change needs to go live. - [ ] Urgent deadline (GA date, etc.): <!-- ENTER DATE HERE --> - [ ] Other deadline: <!-- ENTER DATE HERE --> - [x] None: Not urgent, can wait up to 1 week+ ## SLA - Teamwork makes the dream work, so please add a reviewer to your PRs. - Please give the docs team up to 1 week to review your PR unless you've added an urgent due date to it. Thanks in advance for your help! ## PRE-MERGE CHECKLIST *Make sure you've checked the following before merging your changes:* - [ ] Checked Vercel preview for correctness, including links - [ ] PR was reviewed and approved by any necessary SMEs (subject matter experts) - [ ] PR was reviewed and approved by a member of the [Sentry docs team](https://github.com/orgs/getsentry/teams/docs) ## EXTRA RESOURCES - [Sentry Docs contributor guide](https://docs.sentry.io/contributing/) --------- Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
|
getsentry/sentry-docs#16326 has been merged. I guess this RFC can be closed (I'll close it, feel free to reopen) |
6 participants
TODO. Rendered RFC