Feat: Preserve original error message in warn! logs#36
Feat: Preserve original error message in warn! logs#36skairunner wants to merge 3 commits intoUnleash:mainfrom
Conversation
rbtcollins
left a comment
rbtcollins
left a comment
There was a problem hiding this comment.
This should come in after the tokio support patch I think. I also think you'll find it much easier to add more detailed diagnostics at that point.
But that said, I don't think logging the error is a good approach here. Instead I suggest adding opentelemetry support and updating the message to log the failed request ID. As you say the error could be occuring anywhere in the stack outwards from this layer, and in distributed systems hooking into observability is much more useful than sending folk grovelling through logs.
src/client.rs
Outdated
| let req = self.http.post(&metrics_endpoint); | ||
| match http_types::Body::from_json(&metrics) { | ||
| Err(e) => { | ||
| warn!( |
There was a problem hiding this comment.
I don't like this pattern - it discards key ergonomics around error handling and will grow the code substantially - it has nearly doubled in size.
There was a problem hiding this comment.
I don't understand what the previous code did that this iteration of the code does not do, that discards key ergonomics around error handling. If anything, the previous code discarding errors seems like an antipattern. Can you clarify what you mean?
|
I've tried a different approach, lifting all the update logic into its own fn and With regards to opentelemetry, I'm hesitant to implement what sounds like will be a fairly large change. My intention was to do a small, in-place (which i suppose is no longer applicable), uncontroversial fix that would immediately improve my use-case. While I think it would be ideal to integrate observability, |
|
I have pushed a change that makes non-JSON responses from the features endpoint be more obvious, a la #37 |
|
Thank you. The tokio patch is merged now, and I'll try to find time to review this in detail later this week or next. |
2 participants
This PR rearranges the error handling in
poll_for_updates. It preserves the original error to aid in debugging.About the changes
Closes #35 .
Important files
Only one file was touched.
Discussion points
The original code collected whether an error happened in a var that is later checked and logged. I instead embedded the
warn!calls right where errors happen. I believe this aids in clarity and also means there can be slightly different error messages depending on the situation, but I am open to other ways of handling it as well.