flagd icon indicating copy to clipboard operation
flagd copied to clipboard

refactor: Use standard OTel SDK env vars

Open austindrenski opened this issue 1 year ago • 6 comments

  • 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.

austindrenski avatar Jan 08 '24 23:01 austindrenski

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

netlify[bot] avatar Jan 08 '24 23:01 netlify[bot]

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.

codecov[bot] avatar Jan 10 '24 01:01 codecov[bot]

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.
  • 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).
  • 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)

austindrenski avatar Jan 10 '24 02:01 austindrenski

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.

beeme1mr avatar Jan 10 '24 17:01 beeme1mr

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.

austindrenski avatar Jan 11 '24 13:01 austindrenski

FYI waylaid by some other priorities, but this is still on my radar.

austindrenski avatar Jan 29 '24 14:01 austindrenski