open-feature / python-sdk

Python SDK for OpenFeature
https://openfeature.dev
Apache License 2.0
47 stars 16 forks source link

fix: remove exception logging during evaluation #347

Closed beeme1mr closed 1 month ago

beeme1mr commented 1 month ago

This PR

Impacts

https://github.com/open-telemetry/opentelemetry-demo/issues/1628

Notes

The OTel demo was seeing overly verbose log messages when they disabled a flag. That's because a FLAG_NOT_FOUND error was raised by the provider, and the SDK was logging all exceptions. I've updated the SDK to no longer log OpenFeatureErrors since providers commonly raise these, which can lead to log spam. It's worth noting that this information can still be accessed via a hook or evaluation details.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.41%. Comparing base (5c7bd14) to head (c4c3d3f).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #347 +/- ## ========================================== - Coverage 97.41% 97.41% -0.01% ========================================== Files 26 26 Lines 1240 1239 -1 ========================================== - Hits 1208 1207 -1 Misses 32 32 ``` | [Flag](https://app.codecov.io/gh/open-feature/python-sdk/pull/347/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-feature) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/open-feature/python-sdk/pull/347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-feature) | `97.41% <ø> (-0.01%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-feature#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dyladan commented 1 month ago

Is this affecting only the Python SDK or is it something that should be specified more generally?

beeme1mr commented 1 month ago

Is this affecting only the Python SDK or is it something that should be specified more generally?

Logging isn't covered in the spec. We could consider general guidance but I would be reluctant to go any deeper than that.

toddbaert commented 1 month ago

Is this affecting only the Python SDK or is it something that should be specified more generally?

After approving, I'm actually considering if we should specify something here. I'm wondering if this is the wrong solution to the problem.

I think we should be logging errors if a flag is not found, and that this change will make debugging harder. I think this is a situation where we'd want log-spam.

toddbaert commented 1 month ago

The Java SDK throws here: https://github.com/open-feature/java-sdk/blob/fc40209edcffc063a474aec7bfe9a880e0966750/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java#L135C1-L136C1