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

Migrate log library to go-kit/kit/log #687

Closed
wants to merge 1 commit into from
Closed

Migrate log library to go-kit/kit/log #687

wants to merge 1 commit into from

Conversation

breed808
Copy link
Contributor

prometheus/common/log library is deprecated. Migration to go-kit/kit/log library will allow importing of the shared TLS library for prometheus exporters in the future (see #680 for context).

This can't be merged until support for Windows Event Log is added to the go-kit logging library, so this pull request is merely a draft for review/discussion.

Old prometheus/common/log library is deprecated. Migration to
go-kit/kit/log library will allow importing of the shared TLS library
for prometheus exporters in the future.

Signed-off-by: Ben Reedy <[email protected]>
@breed808
Copy link
Contributor Author

breed808 commented Dec 30, 2020

I've got this at the point where the CI is happy, and testing on my Windows 10 VM is looking good.
At this stage the init() function in collector/cpu.go, is using the nil logger object before it is initialized in main().
There's also the issue of event log functionality not currently supported by the go-kit/kit/log library.

I'm not sure if passing the logger object around as a pseudo-global variable is best, so feedback on that would be welcome.

@carlpett
Copy link
Collaborator

In light of #693, do we actually need to do this to get the TLS bits?
But on the topic of the deprecated common/log package - I think I'd rather prefer that we just copy the logger code over to this repo. That way we do not have to write a go-kit/log integration for Windows, or rewrite any logging code in the exporter. Thoughts? I don't think we gain a lot from a more sophisticated logger like go-kit/log, or am I missing some killer feature?

@breed808
Copy link
Contributor Author

do we actually need to do this to get the TLS bits?

Not at all, this was more a curiosity on my part to see how easily a migration to go-kit/log would be. If the logger issue in #693 can be resolved I don't see a pressing need to pursue this library.

I think I'd rather prefer that we just copy the logger code over to this repo

I'm not overly familiar with the code in common/log, do you think maintenance of this would be troublesome? If not, I've no issues with it.

I don't think we gain a lot from a more sophisticated logger like go-kit/log, or am I missing some killer feature?

go-kit/log appears to focus on structured logging more so than common/log, and standardization of logging library with node_exporter would be nice, but neither of those are strongly desired.
Plus we'd need to get event logging working with the library somehow.

@carlpett
Copy link
Collaborator

I'm not overly familiar with the code in common/log, do you think maintenance of this would be troublesome? If not, I've no issues with it.

We've left it as-is for several years, and I'd expect us to be able to continue to ignore it :) Worst case, the code is fairly straightforward, and I'm probably the author of most of the parts we use.

Agree that go-kit/log has some nice properties for a general logging system. However, due to how the Windows Event Log works, I would expect we'd basically end up just serializing everything into a string anyway, throwing away the structure? And as you say, we'd have to rewrite the integration as well.

@breed808
Copy link
Contributor Author

Yes, I think the structuring would only be relevant when not logging to Event Log. I'd be interested to know how many users are running the exporter under the MSI or EXE.

Regardless, I think the idea to maintain the common/log code ourselves sounds like a good idea, so I'll close off this request.

@breed808 breed808 closed this Jan 17, 2021
@breed808 breed808 deleted the log branch February 2, 2021 10:05
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.

2 participants