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

feat(new source): Initial logplex source implementation #1540

Merged
merged 6 commits into from
Jan 21, 2020
Merged

Conversation

lukesteensen
Copy link
Member

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.

Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
@binarylogic binarylogic changed the title feat(new source): add logplex source feat(new source): Initial logplex source implementation Jan 17, 2020
Copy link
Contributor

@binarylogic binarylogic left a 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.

Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
Copy link
Contributor

@LucioFranco LucioFranco left a 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 :)


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");
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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");
Copy link
Contributor

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?

Copy link
Member Author

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.

}

#[test]
fn router_log() {
Copy link
Contributor

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).

Copy link
Member Author

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>
@lukesteensen lukesteensen merged commit ccee427 into master Jan 21, 2020
@lukesteensen lukesteensen deleted the heroku branch January 21, 2020 02:00
@binarylogic binarylogic added type: feature A value-adding code addition that introduce new functionality. and removed type: new feature labels Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature A value-adding code addition that introduce new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants