impl Error for TraceCtxError and ParseSpanIdError #13
Conversation
There was a problem hiding this comment.
I'm not so sure about the change to telemetry_layer.rs - is this needed? But I'm otherwise down to make these implement Display and Error!
@ericsampson do you think we can release these changes alongside #14 ? Or should these come out in separate releases? They seem like low-risk changes to me. Thoughts?
| @@ -237,7 +237,7 @@ where | |||
| parent_id: Some(self.trace_ctx_registry.promote_span_id(parent_id)), | |||
| initialized_at, | |||
| meta: event.metadata(), | |||
| service_name: &self.service_name, | |||
There was a problem hiding this comment.
This de-reference looks backwards incompatible. Is this change necessary to improve error handling?
There was a problem hiding this comment.
@jfhbrook-at-work This change is not necessary for the error handling. It's a warning that was only fixed since CI complained about it. I had not thought about backwards-compatibility, so thanks for pointing this out.
As a warning: this same change is also in #12 for the same reason.
There was a problem hiding this comment.
Thanks for the heads up! I'll keep an eye on it.
| use TraceCtxError::*; | ||
| write!(f, "{}", | ||
| match self { | ||
| TelemetryLayerNotRegistered => "`TelemetryLayer` is not a registered subscriber of the current Span", |
There was a problem hiding this comment.
These messages look OK to me, though I'd defer to @chrisdickinson for organizational opinions here.
|
Shipped in #14. Thanks! |
By implementing
std::error::Errorit's possible to use them withanyhowfor example. Also, from my understanding, it's expected behavior for errors anyway.I tried to use a shorter, yet still understandable, version of the docstrings for the error messages. If you'd prefer something different, feel free to change or let me know what to put.