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

Allow configuring an alternative logger #116

Merged
merged 2 commits into from
May 3, 2024

Conversation

jcodybaker
Copy link
Contributor

@jcodybaker jcodybaker commented Feb 19, 2024

This PR provides a backwards compatible way to manage logout by providing a *log.Logger in Client and Server config. If one is not specified, client and server will fall back to the original behavior and use the default log.Default() logger.

Integrators can plumb the output to a custom log sink by creating a new *log.Logger with a customer io.Writer. They can also disable logging by by creating a *log.Logger with the io.Discard writer.

@jcodybaker jcodybaker requested a review from a team as a code owner February 19, 2024 03:52
@DanStough DanStough force-pushed the allow-specifying-a-logger branch 2 times, most recently from 7b621be to c296d9b Compare March 22, 2024 18:42
DanStough
DanStough previously approved these changes Mar 22, 2024
Copy link
Contributor

@DanStough DanStough left a comment

Choose a reason for hiding this comment

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

LGTM. I rebased on main and resolved the conflicts. Thanks again 🙇

client.go Outdated
@@ -49,6 +49,7 @@ type QueryParam struct {
WantUnicastResponse bool // Unicast response desired, as per 5.4 in RFC
DisableIPv4 bool // Whether to disable usage of IPv4 for MDNS operations. Does not affect discovered addresses.
DisableIPv6 bool // Whether to disable usage of IPv6 for MDNS operations. Does not affect discovered addresses.
Log *log.Logger // Optionally provide a *log.Logger to better manage log output.
Copy link
Member

Choose a reason for hiding this comment

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

Why Log here and Logger on the other exported struct field. I think we typically name these sorts of optional config fields Logger over in serf & memberlist support libraries.

@ClasEnkvist
Copy link

ClasEnkvist commented Apr 24, 2024

Are there any plans for merging this PR anytime soon? It would be much appreciated. Thanks.

@DanStough
Copy link
Contributor

@jcodybaker do you want to make the suggested change from Log to Logger or would you like me to handle it and merge?

@jcodybaker
Copy link
Contributor Author

Sorry for the delay on merging here. This was for a personal project and the day job got busy. Thanks for the rebase and review. @DanStough feel free to merge on reapproval.

@ClasEnkvist
Copy link

@DanStough I have tested the changes locally and the log gets rerouted as expected 👍 Would be awsome if this PR could be merged in the near future.

@DanStough DanStough merged commit 901c2c5 into hashicorp:main May 3, 2024
2 checks passed
@DanStough
Copy link
Contributor

Thank you all!

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.

4 participants