implement MultiReporter #24
Conversation
Cargo.toml
Outdated
| aws-config = { version = "1", optional = true } | ||
| aws-sdk-s3 = { version = "1", optional = true } | ||
| chrono = "0.4" | ||
| futures = "0.3" |
There was a problem hiding this comment.
nit: can we specify the features we want rather than relying on default features?
src/reporter/multi.rs
Outdated
| .map(|reporter| reporter.report(jfr.clone(), metadata)), | ||
| ) | ||
| .await; | ||
| // return the first error |
There was a problem hiding this comment.
It seems bad to swallow errors past the first? Since they could be totally different destination types / failure reasons?
I don't think Vec<Box<dyn std::error::Error + Send>> is good because it is unpleasant to interact with compared to something directly implementing std::error::Error.
Ideally we would fold the errors somehow, but still preserves per-reporter failure attribution.
Perhaps its worth adding an optional context() method on the Reporter trait that we default to empty, implement for S3Reporter, etc?
But even without that we could at least flatten down the errors in a way that maintains ordering if not direct context.
src/reporter/multi.rs
Outdated
| use std::fmt; | ||
|
|
||
| #[derive(Debug, thiserror::Error)] | ||
| pub struct MultiError { |
There was a problem hiding this comment.
This needs a doc comment, even just something simple like "this is an aggregated error that includes per-reporter report errors identified by their debug impl")
2 participants
📬 Issue #, if available:
✍️ Description of changes:
🔏 By submitting this pull request