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

Catch fatal errors and output as structured #103

Merged
merged 11 commits into from Feb 11, 2020

Conversation

cablett
Copy link
Collaborator

@cablett cablett commented Jan 24, 2020

Hi @tpitale ! We're really appreciating the structured logging 🙌 and there's a further feature tweak I'd like to suggest.

It would be really helpful if we could somehow make all the logs structured, as log ingestion into ElasticSearch and other tools is a lot more reliable if everything is structured.

If you're concerned this could cause folks some issues, we could make it a config variable all_structured that defaults to off so anyone else using mail_room would not be affected unless they explicitly switch it on.

I've made the following PR. What do you think?

@tpitale
Copy link
Owner

tpitale commented Jan 24, 2020

So … the delivery logger is probably not what we want to output as a structured log. It's a delivery type.

If you do want a structured delivery log, add a new one. But … I'm not sure what structure would be added.

@cablett cablett force-pushed the cablett-everything-structured-log branch from fc6ccfd to 5b6ae1d Compare January 27, 2020 10:53
@cablett
Copy link
Collaborator Author

cablett commented Jan 27, 2020

You are right, thank you! I got a bit ahead of myself there 😅 This really has nothing to do with the Delivery type.

I'm thinking how we'd automatically wrap multi-line messages, such as errors/backtraces, in JSON to be auto-imported into our logging engine (which otherwise treats each line of the backtrace as a separate event). I think such a thing would have to happen at the CLI level. I've added a change that is a bit of a strawman but kind of shows the direction I'm thinking.

Having a JSON object rather than a plaintext exception might make the output look a little funny to humans (but still readable, right?) but to any system running the CLI, it would make things much easier.

What do you think?

@cablett cablett changed the title Everything is a structured log! Catch fatal errors and output as structured Jan 27, 2020
bin/mail_room Outdated Show resolved Hide resolved
@tpitale
Copy link
Owner

tpitale commented Jan 27, 2020

You could maybe accomplish some of this with a new CLI argument such as --log-exit-as=json or something. Then https://github.com/tpitale/mail_room/blob/master/lib/mail_room/cli.rb#L52 could be wrapped in a rescue and structured output. If that config is not set, you would re-raise the exception.

@cablett
Copy link
Collaborator Author

cablett commented Feb 10, 2020

@tpitale what do you think of the current state of this PR? If you think it's the right track I'll write some more comprehensive tests.

lib/mail_room/cli.rb Outdated Show resolved Hide resolved
- Add tests
- Add crash handler to CLI
- Crash handler either throws the error or outputs the selected format
@tpitale
Copy link
Owner

tpitale commented Feb 11, 2020

@cablett 🙌 Ready to 🚢?

@cablett
Copy link
Collaborator Author

cablett commented Feb 11, 2020

Ready when you are! Cheers friend! I'll keep an eye on when you release so we can upgrade.

Thank you @tpitale ! ✨

@tpitale tpitale merged commit 135a794 into tpitale:master Feb 11, 2020
@cablett cablett deleted the cablett-everything-structured-log branch February 12, 2020 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants