-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(new sink): Initial papertrail
sink
#1835
Conversation
Signed-off-by: Lucio Franco <luciofranco14@gmail.com>
Signed-off-by: Lucio Franco <luciofranco14@gmail.com>
src/sinks/papertrail.rs
Outdated
} | ||
} | ||
|
||
fn encode_event(mut event: crate::Event, pid: u32, encoding: &Encoding) -> Option<Bytes> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems very reasonable for this to return a Result
no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since our sinks don't return errors we are forced to handle errors within the sink. I have yet to see an encoding panic in any of our sinks and we use this option trick.
Do you have a test account I can use, or should I make one? :) |
@Hoverbear you probably want to make one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the broken Encoding::Text
option it seems functional and documented. :)
Text works fine actually, seems it was just that I missed it. |
.meta/sinks/papertrail.toml
Outdated
examples = ["logs.papertrailapp.com:12345"] | ||
description = "The endpoint to stream logs to." | ||
|
||
[sinks.papertrail.options.encoding] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should default to json
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not following the reason to make this a default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #1915 we removed defaults where there was more than one option according to advice from @binarylogic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be a case by case basis. Clearly Honeycomb should receive structured data. It doesn’t even have full text searching capabilities...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't honeycomb? To me here it totally makes sense to let the user choose since papertrail doesn't takes advantage of the message field in syslog.
According to @Hoverbear we are no longer settings defaults? #1832 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, I'm losing my mind. Sure, I think for papertrail it makes sense to require it. For Honeycomb not so much.
@LucioFranco This PR desires your attention. :) |
Signed-off-by: Lucio Franco <luciofranco14@gmail.com>
Signed-off-by: Lucio Franco <luciofranco14@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
This implements a basic papertrail sink that sends logs via syslog rfc3164.
Closes #1509