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

Leveled log #321

Merged
merged 4 commits into from
Apr 30, 2019
Merged

Leveled log #321

merged 4 commits into from
Apr 30, 2019

Conversation

Lz-Gustavo
Copy link
Contributor

Trying to fix some observability issues #272 , #320

This PR excludes support to a generic user-defined log implementing sdt Logger interface, to support hashicorp's hclog Logger interface. Allowing both the easy configuration of a desired level restriction to Config struct, like bellow:

config := raft.DefaultConfig( ) config.LogLevel = "WARN" raft, err := raft.NewRaft( config, ... )

And the configuration of an implemented hclog.Logger interface.

@hashicorp-cla
Copy link

hashicorp-cla commented Apr 29, 2019

CLA assistant check
All committers have signed the CLA.

config.go Outdated Show resolved Hide resolved
@RobbieMcKinstry
Copy link
Contributor

Hey @Lz-Gustavo, thank you so much for this contribution!
It looks like some tests still need to be updated to use the correct logger:

./integ_test.go:114:14: cannot use env.logger (type *log.Logger) as type hclog.Logger in assignment:
    *log.Logger does not implement hclog.Logger (missing Debug method)
./raft_test.go:78:14: cannot use newTestLogger(t) (type *log.Logger) as type hclog.Logger in assignment:
    *log.Logger does not implement hclog.Logger (missing Debug method)
./raft_test.go:631:19: cannot use newTestLoggerWithPrefix(t, string(configuration.Servers[i].ID)) (type *log.Logger) as type hclog.Logger in assignment:
    *log.Logger does not implement hclog.Logger (missing Debug method)

We have been discussing about replacing the usage of Sprintf with using hc-log with key/value pairs instead, but we think that should be a separate PR in case this produces any breaking changes downstream.

@Lz-Gustavo
Copy link
Contributor Author

I'm sorry, I've forgot about test_files on the first push. Now they're all running smoothly.

About Sprintf usage, I first though about using hclog.Fmt() with it's own key/value pairs, but that would result in some small modifications on the logging output string. I tried to preserve the same standard string format.

@RobbieMcKinstry
Copy link
Contributor

Perfect, thank you very much!

@RobbieMcKinstry RobbieMcKinstry merged commit 2021189 into hashicorp:master Apr 30, 2019
@RobbieMcKinstry
Copy link
Contributor

Thank you very much for your contribution @Lz-Gustavo ! This is a much needed boost to observability!

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