-
Notifications
You must be signed in to change notification settings - Fork 38
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: logging requirements and logging hook #269
Conversation
0ae0cfb
to
4319b54
Compare
Signed-off-by: Todd Baert <[email protected]>
4319b54
to
dd59837
Compare
Signed-off-by: Todd Baert <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Left one question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I have a minor question. With this spec change, will the LogHook
become part of the SDK and should it be moved to the SDK repo?
Yep! I've implemented one in Java now here: open-feature/java-sdk#1084 A |
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