feat: add logging hook, rm logging from evaluation #1084
Conversation
Signed-off-by: Todd Baert <[email protected]>
toddbaert
force-pushed
the
feat/logging-compliance
branch
from
064c509 to
fdb14a8
Compare
Signed-off-by: Todd Baert <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1084 +/- ##
============================================
+ Coverage 95.03% 95.31% +0.27%
- Complexity 392 401 +9
============================================
Files 38 39 +1
Lines 886 917 +31
Branches 54 56 +2
============================================
+ Hits 842 874 +32
+ Misses 24 23 -1
Partials 20 20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
| @@ -17,19 +17,28 @@ | |||
| @SuppressWarnings({ "unchecked", "rawtypes" }) | |||
| class HookSupport { | |||
There was a problem hiding this comment.
No functional changes in this class, I just re-ordered some methods to make the class read more sensibly, and added a bit more information to the log statement.
| try { | ||
| hookCode.accept(hook); | ||
| } catch (Exception exception) { | ||
| log.error("Unhandled exception when running {} hook {} (only 'after' hooks should throw)", hookMethod, |
There was a problem hiding this comment.
This ONLY runs for hooks which we expect not to throw (before/finally/error).
Signed-off-by: Todd Baert <[email protected]>
Please check out my addition. |
As discussed in recent meetings, we've had complaints about this from multiple sources. This PR adds a stipulation that no logging is done by the SDK during flag evaluation. It also defines a very simple `logging hook` concept as an included utility. These should be very simple to write and will provide all the needed logging but give authors more control than built in log statements. It will also be a very nice intro to the hooks concept for users. Here's the Java implementation: open-feature/java-sdk#1084 --------- Signed-off-by: Todd Baert <[email protected]>
There was a problem hiding this comment.
Looks good. Please check this remark though [1]
[1] - https://github.com/open-feature/java-sdk/pull/1084/files#r1744351346
|
Implements: open-feature/spec#269
This PR removes logging from the main evaluation code path, with a couple exceptions:
(IMO these represent bugs in code, since they should be handled, and not transient errors).
It also adds a
LoggingHookas per the spec link above, which adds SLF4J structured logs for evaluation objects.