Firehose topic for IncidentLogger #96

Merged
merged 0 commits into from Nov 20, 2012

Conversation

Projects
None yet
3 participants
Contributor

grantr commented Sep 24, 2012

IncidentLogger currently doesn't support the perfectly valid logging need to have every message logged instead of just errors.

This branch adds a firehose topic that gets every LogEvent above the configured log level. An example FirehoseConsumer is also included that just wraps a ::Logger instance.

This feature has the potential to cause serious slowdowns if the fanout notifier is not performant. I haven't done any benchmarks yet but someone probably should before the IncidentLogger is enabled by default.

Speaking of things that should happen before IncidentLogger is enabled: still no tests.

Also something that should happen before this goes out is a shutdown hierarchy. Occasionally the notifier gets shut down before the consumers. This is a bigger problem now because the logger tries to publish the shutdown message, but gets a DeadActorError which is logged to STDERR by the fallback logger.

Owner

tarcieri commented Sep 26, 2012

Sorry I haven't had time to look at this yet, been pretty busy with Strange Loop

Contributor

grantr commented Sep 26, 2012

No problem, good luck at the conference.

@grantr grantr merged commit f07e038 into celluloid:master Nov 20, 2012

1 check passed

default The Travis build passed
Details
Owner

tarcieri commented Nov 20, 2012

This doesn't have non-negligible perf impacts, right?

Contributor

grantr commented Nov 20, 2012

I don't really know, I haven't tested perf yet. It's mostly dependent on performance of the fanout notifier.

Contributor

grantr commented Nov 20, 2012

oh snap this was not supposed to be merged into master.. when did i get commit rights!?!

Owner

tarcieri commented Nov 20, 2012

You've had them for awhile ;) Feel free to revert this if you don't think it's ready

Contributor

grantr commented Nov 20, 2012

Seems like this was caused by me rebasing the branch to master and accidentally removing the commit. That seems to have closed the PR even though nothing was merged.

I'll put this branch back in place and work somewhere else.

Owner

halorgium commented Nov 20, 2012

Yes, you can re-push the commits to your branch.
GitHub closes the PR if there are no commits left.

Owner

tarcieri commented Nov 20, 2012

@halorgium I was confused because it claims it was merged even though it was not

Contributor

grantr commented Nov 20, 2012

Ok I pushed the old branch again, but I can't figure out how to re-open this. I'll create a new PR.

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