flagd
flagd copied to clipboard
refactor: Use standard OTel SDK env vars
- https://opentelemetry.io/docs/concepts/sdk-configuration/
- https://opentelemetry.io/docs/concepts/sdk-configuration/general-sdk-configuration/
- https://opentelemetry.io/docs/concepts/sdk-configuration/otlp-exporter-configuration/
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.
Deploy Preview for polite-licorice-3db33c canceled.
Name | Link |
---|---|
Latest commit | ab8020e24ebd95803d0a0af7f0633e5999aa9463 |
Latest deploy log | https://app.netlify.com/sites/polite-licorice-3db33c/deploys/659e031b955d330008ff2320 |
Codecov Report
Attention: 34 lines
in your changes are missing coverage. Please review.
Comparison is base (
55dc8cf
) 73.53% compared to head (ab8020e
) 73.04%.
Files | Patch % | Lines |
---|---|---|
core/pkg/telemetry/builder.go | 58.97% | 31 Missing and 1 partial :warning: |
core/pkg/runtime/from_config.go | 0.00% | 2 Missing :warning: |
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
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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:
- Should telemetry wire-up ever return errors?
- OTel SDKs tend to err on the side of
never break the app no matter what
- Existing behavior returns errors in some cases.
- But on some new code paths I've opted to log debug messages instead of returning errors, based on the rationale that if we're deferring to the OTel SDK for wire-up, then we should defer to its style of graceful failure too.
- Mostly asking this question for my edification in the event that this is something the project already has a strong opinion on.
- OTel SDKs tend to err on the side of
- The tests are passing...but I also may have broken the intent of one or more of the tests.
- Some of the tests were related to the above
should we error
question, so just looking for feedback here on what the project/org expects in terms of coverage/quality in tests (on both fronts, I expect what is currently here will probably need some work).
- Some of the tests were related to the above
- Do we/anyone have a well-defined/formal integration environment? Are the unit tests that environment? How would we know if I've borked something subtle here?
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.
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 https://github.com/open-feature/flagd/issues/1141#issuecomment-1886716151.
FYI waylaid by some other priorities, but this is still on my radar.