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

Switch logging to hclog #685

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sinkingpoint
Copy link

Currently, serf takes a standard lib log.Logger. This makes using serf as a library a bit weird because you have to have a logger that may produce output in a different format to your main binary. There are ways to hack around this, e.g. by using a log.Logger that writes to some intermediate buffer that parses the log output, but this loses a lot of context that we would otherwise have.

In this commit, we swap serf over to taking an hclog.Logger instead. As an interface, this means that library consumers can use whatever logger they want, and we can use hclog's built-in support for log levels and structured logging.

@sinkingpoint sinkingpoint requested a review from a team as a code owner April 16, 2023 21:55
@sinkingpoint sinkingpoint requested review from freddygv and removed request for a team April 16, 2023 21:55
@hashicorp-cla
Copy link

hashicorp-cla commented Apr 16, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@freddygv freddygv left a comment

Choose a reason for hiding this comment

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

Hi @sinkingpoint, thanks for the contribution, this is heading in a good direction.

The main thing I noticed is that the command/agent pkg still uses a *log.Logger in various ways, which leads to mismatched logs when running the Serf binary.

Notice the timestamp differences below:

==> Log data will now stream in as it occurs:

2023/04/17 11:05:12 [INFO] agent: Serf agent starting
2023-04-17T11:05:12.814-0600 [INFO]  serf: EventMemberJoin: test 10.0.0.145
2023/04/17 11:05:13 [INFO] agent: Received event: member-join
2023/04/17 12:07:24 [DEBUG] memberlist: Stream connection from=[::1]:54771

Could you update this to make the logging consistent for events coming from the serf library, the command/agent package, and memberlist?

serf/keymanager.go Outdated Show resolved Hide resolved
@sinkingpoint
Copy link
Author

Thanks for the review @freddygv ! I was being a bit lazy and only changed the path I was using 😂. I've just pushed a change that I think updates the rest of the code. The only thing I'm a bit unsure on is if we want to stop passing the logOutput around and instead pass around the complete hclog.Logger so that subcomponents don't have to make their own.

Currently, serf takes a standard lib log.Logger. This makes using serf
as a library a bit weird because you have to have a logger that may produce
output in a different format to your main binary. There are ways to hack around
this, e.g. by using a log.Logger that writes to some intermediate buffer that parses
the log output, but this loses a lot of context that we would otherwise have.

In this commit, we swap serf over to taking an hclog.Logger instead. As an
interface, this means that library consumers can use whatever logger they
want, and we can use hclog's built-in support for log levels and structured
logging.

Signed-off-by: sinkingpoint <colin@quirl.co.nz>
@freddygv
Copy link
Contributor

The only thing I'm a bit unsure on is if we want to stop passing the logOutput around and instead pass around the complete hclog.Logger so that subcomponents don't have to make their own.

Hmm I'm not sure about that either at the moment. I'll do some thinking about it.

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