-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 source): Initial logplex
source implementation
#1540
Conversation
Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
logplex
source implementation
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! Docs look good.
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 looks great! I left a few questions, nothing really blocking :)
src/sources/logplex.rs
Outdated
|
||
if events.len() != msg_count { | ||
error!(message = "Parsed event count does not match message count header", event_count = events.len(), %msg_count); | ||
debug_assert!(false, "Parsed event count does not match message count header"); |
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.
Can you explain why we need this?
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.
If we're in this branch, that means we've failed to parse the body correctly. In development, I want that to blow up, fail tests, etc. In production, I want to spit out an error but keep going so that we remain available for users.
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.
Would
if cfg_is!(test) {
panic!(...)
}
be more clear here than debug_assert? Mostly, so that people know this should panic in tests.
.map(|_| warp::reply()) | ||
}); | ||
|
||
let ping = warp::get2().and(warp::path("ping")).map(|| "pong"); |
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.
is this required or was this just for testing?
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.
Not required, but it's convenient when setting up infrastructure to make sure the server is reachable. Think of it as a minimum viable healthcheck, since we have no other routes without side effects.
src/sources/logplex.rs
Outdated
} | ||
|
||
#[test] | ||
fn router_log() { |
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.
Would it make sense to test line_to_event
a bit more? I wonder if we could fuzz or quickcheck at the most extreme too (not now but in the future).
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.
Yeah, it would. I've deliberately kept it pretty simple for now and provided fallbacks for when assumptions aren't met, but it would definitely be a good thing to fuzz.
In the meantime, I'll push a couple of other simple unit tests for it.
Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
Nothing too fancy, just accepts text bodies over HTTP where each line is roughly syslog-formatted. We do some basic parsing but fall back to forwarding the raw line if anything seems off.