Skip to content

Conversation

@sahidvelji
Copy link
Contributor

@sahidvelji sahidvelji commented May 19, 2025

This PR

  • add go doc comments
  • add a stage key to the logs as per the spec
  • ensure LoggingHook method receivers are all named h (the buildArgs method had a receiver named l)
  • use consistent parameter names for the Before, After, Error, and Finally methods
  • simplify the buildArgs method by removing the error return parameter
  • use slog.Attr instead of interface{} types to log attributes
  • the above allows the use of Logger.LogAttrs, which is the most efficient form of log output
  • as a consequence of the above, the logging performed by the logging hook is now context aware (the ctx parameter is passed to Logger.LogAttrs)

Related Issues

Notes

Follow-up Tasks

How to test

@sahidvelji sahidvelji changed the title refactor: simplify and improve logging hookq refactor: simplify and improve logging hook May 19, 2025
@codecov
Copy link

codecov bot commented May 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.80%. Comparing base (0f6d828) to head (0b7656d).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #361      +/-   ##
==========================================
+ Coverage   88.20%   88.80%   +0.60%     
==========================================
  Files          14       14              
  Lines        1407     1403       -4     
==========================================
+ Hits         1241     1246       +5     
+ Misses        142      136       -6     
+ Partials       24       21       -3     
Flag Coverage Δ
e2e 88.80% <100.00%> (+0.60%) ⬆️
unit 88.80% <100.00%> (+0.60%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Sahid Velji <sahidvelji@gmail.com>
Signed-off-by: Sahid Velji <sahidvelji@gmail.com>
Signed-off-by: Sahid Velji <sahidvelji@gmail.com>
@sahidvelji sahidvelji force-pushed the log-hook branch 2 times, most recently from 235b3e1 to 1791cd8 Compare May 19, 2025 02:39
@sahidvelji sahidvelji marked this pull request as ready for review May 19, 2025 02:40
@sahidvelji sahidvelji requested review from a team as code owners May 19, 2025 02:40
Copy link
Member

@aepfli aepfli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contributions. This looks really helpful. Unfortunately, I am not as proficient with Golang, so I will wait for another approval before merging.

Side question: do we have automated tests for this?

@sahidvelji
Copy link
Contributor Author

sahidvelji commented May 19, 2025

Good call on tests. I added the stage key as expected output in the tests. Other than the addition of the stage key, the functionality of the logging hook remains the same as before.

Signed-off-by: Sahid Velji <sahidvelji@gmail.com>
@beeme1mr
Copy link
Member

Hey @sahidvelji, could you please also add a short section to the readme that mentions the hook? Here's an example from Java. It would be nice to include a short code example too (something currently missing from the Java example).

@sahidvelji
Copy link
Contributor Author

sahidvelji commented May 20, 2025

Hey @sahidvelji, could you please also add a short section to the readme that mentions the hook? Here's an example from Java. It would be nice to include a short code example too (something currently missing from the Java example).

@beeme1mr Yes that makes sense, but I'd like to do that in a separate PR if you're OK with it. I have created an issue. Feel free to assign me to it.

@beeme1mr beeme1mr merged commit 5d41ca0 into open-feature:main May 20, 2025
6 checks passed
@sahidvelji sahidvelji deleted the log-hook branch May 20, 2025 18:16
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.

3 participants