More cleanup related to error logging#9942
Conversation
2bbd201 to
e731217
Compare
0175e1b to
d61c890
Compare
c675dd0 to
88b2f30
Compare
smklein
left a comment
smklein
left a comment
There was a problem hiding this comment.
The structure overall looks great here. Thanks for doing this cleanup; hopefully this makes our errors more sensible. LGTM modulo a few comments below.
| @@ -244,19 +246,21 @@ pub struct UnloadKeyError { | |||
|
|
|||
| #[derive(Debug, thiserror::Error)] | |||
There was a problem hiding this comment.
Should this one derive SlogInlineError too?
There was a problem hiding this comment.
This is a bit of an open question, and I think it mostly depends on organizational culture, so I'm going to throw it back to you, to some extent.
The code will fail to compile if one tries to use an error type after the semicolon in a logging macro, without having derived SlogInlineError, so it's reasonably convenient, and not very error-prone, I think, to plan to add the derive at the time one is adding code that relies on it. Because of that, I opted not to add derive calls in the changes unless they were needed.
On the other hand, the added friction of having to add the derive can be costly to others, and it's occurring to me now that it could be a useful way to mark an error as being transitioned to rely on the source function instead of echoing source error messages in its Display implementation.
So I could go either way on this one, depending on what you think. Otherwise, I lean toward being lazy about it.
There was a problem hiding this comment.
I'm totally fine punting it - there's sorta a distinction between "error structs annotating with Derive" vs "callers expecting underlying errors to have already added the derive". Makes sense that the latter would be a bit onerous, since it requires everyone to be converted already.
I think adding the derive more universally would be a reasonable long-term goal, but I'm okay preventing this PR from getting larger!
There was a problem hiding this comment.
Thanks again for looking at this!
Yeah, another part of my thought process here was that I'm a bit uncertain about whether this might be revisited later on. There could be some observability-related motivation to switch to something more structured than what SlogInlineError does, like ArrayInlineError, so I also didn't want to go out of my way to do something that might never be used before it gets replaced.
To add to the analysis paralysis, I also had made an open PR on slog-error-chain, which adds blanket trait implementations that make it ever so slightly more convenient to create one of these adapters. At some point shortly after, I saw display_error_chain mentioned in the omicron codebase, and noticed that it also has blanket traits for Errors and Results.
One can achieve almost the same convenience by adding a postfix snippet to their rust-analyzer config, but I feel like the method call is a bit more readable, and it's one less thing for each developer to have to configure in their own editing setup.
I don't have strong opinions about this in general, though - it's much safer and easier to switch between these options than the initial cleanup is, and it's more important to preserve the source information in some form than to get the reporting of it exactly right up front.
88b2f30 to
c91eb7b
Compare
|
My push reset the workflow, but it looks like the last one was green, at least: I wasn't expecting it to be finished so quickly. I guess #10018 is working. |
This change updates all handlers of omicron_sled_agent::Error to use the InlineErrorChain adapter, removes all duplicate source error messages from its Display implementation, and adds the #[source] attribute to all applicable source errors.
It looks like all code paths that handle ConfigError are already attaching it to error types that will log the full chain of sources, so there's nothing to do for this type, save for cleaning up its implementation of `Display`.
Previously, only the next level down would be logged, which could discard source information in case of NodeRequestError::Fsm and NodeRequestError::Recv error types. This commit links bootstore::NodeRequestError as a source instead of formatting it as a string, which would drop the rest of the chain.
This commit updates handle_start_sled_agent_request to log the full error chain in case a SledAgentServerStartError is returned. Previously, one level of cause would be logged for the FailedLearnerInit variant, and CommitToLedger errors were logged without any information about the source.
The main consumer of this error type seems to be omicron_sled_agent::sled_agent::Error, so this commit has the effect of fixing duplication in error logging that was introduced by a previous commit within the same PR.
This change modifies all of the code paths that handle omicron_sled_agent::instance::Error so they use the InlineErrorChain adapter before logging error messages, and then updates the enum definition to avoid duplicate error emission from its `Display` implementation.
Similar to previous commits, this updates all handlers of omicron_sled_agent::services::Error such that they recurse into sources instead of relying on the duplication of source error messages in its `Display` implementation. It then also removes all duplicated error strings from its `Display` implementation, defining sources instead, where applicable.
In many cases, this will be overkill, because io::Error variants generally don't have linked sources. Unfortunately, there is one corner case, which is the Other variant, and in general, there's no easy way to know an io::Error isn't going to be that variant, so this change updates most code paths that handle io::Error.
c91eb7b to
0e40cc3
Compare
|
@smklein Thanks for the review, this is ready for you to look at again. I'm wondering how you'd want to address the question about where to derive If once a day is too frequent to remind about this, please let me know. It's nice to have hit a merge conflict that resulted from someone else independently improving an overlapping code path, though – I kind of like it when that happens 🙂 |
|
This looks good to me - if CI is happy, I'll merge for you. Thanks for making this change! Looking forward to our clearer error chains! (As an FYI for future changes - we generally try to avoid force-pushing, especially after reviews have started, because Github makes it pretty painful to see diffs between commits if any force-pushing has occurred. I will re-review the entire PR if there are any force-pushes, as a matter of completeness -- but I obviously don't need to do that for non-force-pushed incremental changes) |
Oh, sorry about that - it was mostly to rebase on the main branch and fix conflicts, but I did also slip a few changes in there. At times, I've coped with this pain (in GitLab, which behaves in the same sad way) by leaving explanatory comments next to each push, and trying to make separate pushes for rebases and other changes, so the compare link actually does something meaningful. I wasn't sure how important that would be for anyone's review workflow, though, so I didn't bother here. I wish I had, though: I hate to impose any extra review effort 😅 On the reviewing side, I've also sometimes found it useful to work around this by diffing the diffs (using |
|
@smklein Thanks for remembering to merge this! I'm so used to having to remind people, sadly, that it was a bit of a pleasant surprise. |
2 participants
This is a bit of a grab bag of error logging related fixes, carefully ordered (as described in #9804) to err on the side of potentially logging source error messages multiple times, rather than not at all.
This is related to #9804.