fix: don't hard-error on unreadable messages #103
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For reasons I don't understand, Galileo can fail to create a MessageInfo object from a Message. If that conversion fails, we can't proceed. The unwrap was causing a fatal error; instead, let's just log it and keep going. Closes #101.
cratelyn
reviewed
Jun 5, 2024
| let fmt_layer = tracing_subscriber::fmt::layer().with_target(true); | ||
| let fmt_layer = tracing_subscriber::fmt::layer() | ||
| .with_target(true) | ||
| .with_ansi(std::io::stderr().is_terminal()) |
There was a problem hiding this comment.
logging to stderr feels mildly surprising to me, but i don't have a strong opinion about this 👍
There was a problem hiding this comment.
Yeah, subjective, I guess. Putting on my sysadmin hat, I feel strongly that "stdout is for output, stderr is for logging and diagnostics." But in the case of galileo serve, where the only output is logging, stdout is probably fine. Old habits die hard.
src/handler.rs
Outdated
| Ok(m) => m, | ||
| Err(e) => { | ||
| // We can't return an error, but we also can't proceed, so log the error | ||
| // and bare-return to bail out. |
There was a problem hiding this comment.
Suggested change
| // and bare-return to bail out. | |
| // and early-return to bail out. |
lightly held comment nit, tioli ("take-it-or-leave-it")
| @@ -261,7 +261,15 @@ impl EventHandler for Handler { | |||
| /// window, then Galileo will respond instructing the user to wait longer. | |||
| async fn message(&self, ctx: Context, message: Message) { | |||
| // Check whether we should proceed. | |||
| let message_info = MessageInfo::new(&message, &ctx).unwrap(); | |||
| let message_info = match MessageInfo::new(&message, &ctx) { | |||
There was a problem hiding this comment.
this looks great 👍
eventually it could be cool to expose a metric counting messages observed / parsed, but that's another item for the rainy day bucket 🙂
conorsch
force-pushed
the
101-dont-error-on-message-ids
branch
from
66466a7
to
3d75d77
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For reasons I don't understand, Galileo can fail to create a MessageInfo object from a Message. If that conversion fails, we can't proceed. The unwrap was causing a fatal error; instead, let's just log it and keep going. Closes #101.
While I was in there, also patched up the logging, to make the logs easier to read when searching. Therefore closes #87.