Skip to content
New issue

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

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

Already on GitHub? Sign in 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Implement Logging Hook
3 participants