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

log: support syslog formatter #333

Closed
fabxc opened this issue Aug 3, 2016 · 10 comments
Closed

log: support syslog formatter #333

fabxc opened this issue Aug 3, 2016 · 10 comments

Comments

@fabxc
Copy link
Contributor

fabxc commented Aug 3, 2016

The option of a syslog formatter would add a critical feature for Prometheus to consider switching to the go-kit log package.

We are currently using our own structured logging wrapping logrus and added our syslog formatter here: https://github.com/prometheus/common/blob/master/log/syslog_formatter.go

@ChrisHines
Copy link
Member

I think it would be fantastic if the Go kit log package could be adopted by Prometheus. Can you help with an implementation? I don't have any experience with syslog or how Prometheus uses it. I can certainly help think through how to integrate it with Go kit log and review or polish the code once we figure out how it should work.

I've taken a look at the code you linked. It seems to me that the biggest challenge is how to surface the log levels to the logging client and preserve them through to the eventual call to one of the syslog.Writer methods.

@fabxc
Copy link
Contributor Author

fabxc commented Aug 4, 2016

To be honest, neither have I. This was a third party contribution to Prometheus's log package.
Log levels seem like a challenge indeed.

If I can find time I'd love to dig into it a bit but I cannot give any guarantees at the moment.

@ChrisHines
Copy link
Member

Looking more closely, I see that Prometheus already defines a common/log.Logger interface that surfaces the log levels to the rest of the code base. That answers the question of how to surface the log levels.

Since that is taken care of, I can imagine an implementation that would work within Prometheus. I am not sure how much of that code would be general purpose enough to publish within Go kit itself. It is probably easier to build the specific Prometheus solution first and see what we learn from that.

I could help with a PR for https://github.com/prometheus/common that replaced the current implementation of common/log.Logger with a Go kit log based implementation (including a new syslog adapter) if that is desired.

@fabxc
Copy link
Contributor Author

fabxc commented Aug 15, 2016

My idea (haven't asked other maintainers extensively yet) was more about replacing our usage of github.com/prometheus/common/log with the go-kit log package through our entire code base.

In the end we don't really want to maintain our own log package if it's in the end has the same goals as an existing one.

The only thing the gokit logger does not have, which was a requirement back then, is colored output.

@ChrisHines
Copy link
Member

We have a colored Logger in the kit/log/term package. See if it meets your
needs.

On Mon, Aug 15, 2016, 10:52 AM Fabian Reinartz notifications@github.com
wrote:

My idea (haven't asked other maintainers extensively yet) was more about
replacing our usage of github.com/prometheus/common/log with the go-kit
log package through our entire code base.

In the end we don't really want to maintain our own log package if it's in
the end has the same goals as an existing one.

The only thing the gokit logger does not have, which was a requirement
back then, is colored output.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#333 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABAvZ5HI0ocZ8ZIiEyuK9-6LsqPWIFq2ks5qgH0-gaJpZM4JbhKk
.

@fabxc
Copy link
Contributor Author

fabxc commented Aug 15, 2016

Ah, my oversight, thanks!

@ChrisHines
Copy link
Member

@fabxc:

My idea (haven't asked other maintainers extensively yet) was more about replacing our usage of github.com/prometheus/common/log with the go-kit log package through our entire code base.

That is certainly a more ambitious idea. As I said in my first comment. I am glad to help however I can if your idea gains traction.

@ereOn
Copy link

ereOn commented Mar 7, 2017

Has any progress been done on this ? If not, I'd be glad to help.

@ereOn
Copy link

ereOn commented Mar 7, 2017

I actually went ahead as an exercise, and came up with #486.

Let me know what you think. I haven't done anything in terms of documentation/tests (except for manual tests at least of course).

Another option I thought about was: instead of creating a new logger class, coming up with a specific syslog writer, detect at a lower level that the writer is "log-level capable" and rely on that instead. It's too late for me to clearly assert the validity of that idea. If I get the time, I can try to come up with another example PR for this change tomorrow.

@peterbourgon
Copy link
Member

Supported with #574.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants