Error type suggestions for #2162 #2181
Conversation
|
Some of this is split out (against main) in #2182. |
Bumps windows_aarch64_msvc from 0.42.0 to 0.42.1. --- updated-dependencies: - dependency-name: windows_aarch64_msvc dependency-type: indirect update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [async-trait](https://github.com/dtolnay/async-trait) from 0.1.61 to 0.1.63. - [Release notes](https://github.com/dtolnay/async-trait/releases) - [Commits](dtolnay/async-trait@0.1.61...0.1.63) --- updated-dependencies: - dependency-name: async-trait dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [tokio](https://github.com/tokio-rs/tokio) from 1.24.1 to 1.24.2. - [Release notes](https://github.com/tokio-rs/tokio/releases) - [Commits](https://github.com/tokio-rs/tokio/commits) --- updated-dependencies: - dependency-name: tokio dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
The `boring` crate exposes a `fips` feature flag, but this feature was not exposed by the `linkerd-meshtls-boring`. This change makes it theoretically possible to build the proxy in this mode. This feature is NOT tested in CI due to the fussiness of the required build environment and longer-term maintenance concerns. Signed-off-by: Zahari Dichev <[email protected]>
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
seems fine to me! i left a couple minor comments, but no blockers.
|
|
||
| #[derive(Debug, thiserror::Error)] | ||
| #[error("route ({:?}): {}", .route_labels, .source)] | ||
| struct RouteError { |
There was a problem hiding this comment.
I only added this error type because the ticket for error contexts listed "logical/route/concrete/endpoint" as the desired contexts to include in errors...do we not want to add a route error context after all, or is that still something we care about?
There was a problem hiding this comment.
Gotcha. There probably wasn't much thought behind that description.
Remove RouteError types for now. They're not really critical for debugging. We may restore them later as those stacks evolve.
I think the general service/endpoint coordinates are sufficient for what we really need for diagnostics/error reporting. I want to hold off on doing route-specific things until we see how those types start to settle. I suspect we may want to omit them in the short term--they'll add a ton of verbosity to already-busy error messages. This could change as routes need to actually fail requests with errors, which I don't believe is really possible (other than load shedding on service availability).
| #[derive(Debug, thiserror::Error)] | ||
| #[error("concrete service {addr}: {source}")] | ||
| pub struct ConcreteError { | ||
| addr: ConcreteAddr, | ||
| #[source] | ||
| source: Error, | ||
| } |
There was a problem hiding this comment.
what's the motivation for having separate versions of ConcreteError, LogicalError, and EndpointError for the HTTP and TCP stacks when they are now identical?
There was a problem hiding this comment.
All error types are defined in the module in which they are used, even when they exactly match error types elsewhere. As we move forward with the upcoming changes, we want each stack to be as independent as possible.
As I mentioned when we did a read-through of https://github.com/linkerd/linkerd2-proxy/tree/ver/policy-client-sketch, the current strategy (to support modularity, refactoring, etc) is to focus on decoupling over reuse. We'll be doing the same type of decoupling with target types.
If they really are common, we should also split out the common parts of the stack impl so that a single module can hold it all.
Basically: we should drive towards making each stack module as self-contained as possible.
| impl std::fmt::Debug for Version { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| match self { | ||
| Self::Http1 => write!(f, "1.x"), | ||
| Self::H2 => write!(f, "h2"), | ||
| Self::Http1 => write!(f, "HTTP/1"), | ||
| Self::H2 => write!(f, "HTTP/2"), |
There was a problem hiding this comment.
technically this feels more correct as a Display impl to me, since I feel like the general idiom for Debug is to mimic the way the type appears in Rust source code...but if this was the Display impl, we would just basically never use the Debug impl, and always using this formatting seems fine to me, so it's not a big deal. probably better to just use this formatting consistently.
There was a problem hiding this comment.
FWIW this is what the http crate's Version does -- i'm assuming because you'll never see HTTP/2 anywhere in a request stream...
The HTTP error responder already knows whether a request is considered HTTP/1, HTTP/2, or gRPC. This change updates these log messages to include the relevant protocol information for each failed request. Furthermore, this change updates the `http::Version` enum to implement `Debug` with full HTTP version strings like "HTTP/1" and "HTTP/2". We then remove the version for debug spans, as it's needlessly verbose for most uses. When debugging, we can identify the version based on specific events in a span. It need not exist on every event on the span.
Respondhandler (since it already includes all of this information). This also let's us be explicit about gRPC. This removes needing to wire it through elsewhere.http::Versionnow implementsDebugwith full "HTTP/1" or "HTTP/2" strings. We change uses ofDisplaytoDebug.RouteErrortypes for now. They're not really critical for debugging. We may restore them later as those stacks evolve.