-
Notifications
You must be signed in to change notification settings - Fork 71
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
refactor: Use standard OTel SDK env vars #1142
base: main
Are you sure you want to change the base?
refactor: Use standard OTel SDK env vars #1142
Conversation
✅ Deploy Preview for polite-licorice-3db33c canceled.
|
1cf9cdf
to
ab2e71f
Compare
a8a0560
to
6d99c91
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1142 +/- ##
==========================================
- Coverage 73.53% 73.04% -0.49%
==========================================
Files 32 32
Lines 3113 3135 +22
==========================================
+ Hits 2289 2290 +1
- Misses 717 740 +23
+ Partials 107 105 -2 ☔ View full report in Codecov by Sentry. |
9624a97
to
f01ad47
Compare
- https://opentelemetry.io/docs/concepts/sdk-configuration Signed-off-by: Austin Drenski <[email protected]>
f01ad47
to
ab8020e
Compare
Full transparency, I'm still relatively new to golang, so please take everything I've written here with a grain of salt. That said, I think this is in a decent place for a first round of reviews. Areas of concern/in need of feedback:
Anyways, please have at it and let me know what you think 🙇 (and again, I'm still new to golang, so if you see garbage code, please call it out for being garbage code, rather get the feedback sooner rather than later lol) |
I think this is a good idea, but I would prefer to make this a non-breaking change. We can mark the arguments as deprecated and completely remove them at a later point. I think we can make the existing arguments and environment variables into the official OTel environment variables. |
Okay, that makes sense. I started this PR just as a sample to gauge interest, so went in axe swinging, but can definitely scale back the frontend changes. I'll take another pass to bring those frontend flags/envs back and try to get everything aligned with @thisthat's comments in #1141 (comment). |
FYI waylaid by some other priorities, but this is still on my radar. |
Closes: #1141
Note
Fully aware that this neither sufficient nor suitable as is, but opening as a companion to #1141 to provide a starting point for discussion/consideration.