Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: add logging hook, rm logging from evaluation #289

Merged
merged 13 commits into from
Oct 23, 2024

Conversation

liran2000
Copy link
Member

@liran2000 liran2000 commented Oct 3, 2024

Implements: [FEATURE] Implement Logging Hook

This PR removes logging from the main evaluation code path, with a few exceptions, like unhandled errors in event handlers. These cases are printed with default slog.
It also adds a LoggingHook as per the spec link above, using slog.

logr usage is removed, but exported APIs remains for backward compatibility.

Java implementation

Closes: #284

@toddbaert @Kavindu-Dodan

Signed-off-by: liran2000 <liran2000@gmail.com>
Signed-off-by: liran2000 <liran2000@gmail.com>
Signed-off-by: liran2000 <liran2000@gmail.com>
Signed-off-by: liran2000 <liran2000@gmail.com>
Signed-off-by: liran2000 <liran2000@gmail.com>
@liran2000 liran2000 marked this pull request as ready for review October 3, 2024 10:23
@liran2000 liran2000 requested a review from a team as a code owner October 3, 2024 10:23
@toddbaert
Copy link
Member

@liran2000 sorry I missed this! I will review today. Looks like there's some CI failures though.

@liran2000
Copy link
Member Author

@liran2000 sorry I missed this! I will review today. Looks like there's some CI failures though.

Thanks, this PR requires GO version to be upgraded to 1.21 before proceeding, hence the failures.

@toddbaert
Copy link
Member

I've opened: #294

README.md Show resolved Hide resolved
@toddbaert
Copy link
Member

@liran2000 thanks so much; sorry for the wait.

#289 (comment) is my only question. Approved, but wonder what your thoughts on that are.

@toddbaert toddbaert self-requested a review October 22, 2024 13:03
@toddbaert
Copy link
Member

@liran2000 I've merged the Go 1.2.1 change, there's a few lint/CI failures.

Signed-off-by: liran2000 <liran2000@gmail.com>
Signed-off-by: liran2000 <liran2000@gmail.com>
Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 82.75862% with 10 lines in your changes missing coverage. Please review.

Project coverage is 82.19%. Comparing base (ddfffdd) to head (81acdf6).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
openfeature/hooks/logging_hook.go 81.63% 6 Missing and 3 partials ⚠️
openfeature/event_executor.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #289      +/-   ##
==========================================
+ Coverage   81.89%   82.19%   +0.30%     
==========================================
  Files          10       11       +1     
  Lines        1215     1219       +4     
==========================================
+ Hits          995     1002       +7     
+ Misses        199      193       -6     
- Partials       21       24       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: liran2000 <liran2000@gmail.com>
Signed-off-by: liran2000 <liran2000@gmail.com>
@toddbaert toddbaert requested a review from aepfli October 22, 2024 16:25
Signed-off-by: liran2000 <liran2000@gmail.com>
README.md Show resolved Hide resolved
Signed-off-by: liran2000 <liran2000@gmail.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert
Copy link
Member

Thanks again @liran2000

@toddbaert toddbaert merged commit 7850eec into open-feature:main Oct 23, 2024
8 checks passed
@liran2000 liran2000 deleted the issue/284 branch October 24, 2024 08:05
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Implement Logging Hook
3 participants