wip(prom): Simplify metrics traits #3242
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`MkStreamLabel` is, in short, a generic `(&Request) -> Option<StreamLabel>` function. we use it to inspect a request, and potentially provide the caller with an object that can provide relevant labels. the `StreamLabel` interface includes associated types for the labels used for metrics related to request/response duration, and counting status codes. we do not however, actually need to separately define these associated types in the `MkStreamLabel` contract. instead, we can return a generic `StreamLabel` of some sort, and leave the responsibility of the (admittedly baroque) associated type access to our type aliases like `RecordResponseDuration` and `RecordRequestDuration`. this change has a pleasant knock-on effect of leaving a number of the labels submodule's type aliases unused. this commit accordingly removes aliases like `HttpRouteRsp`, `GrpcRouteRsp`, `HttpRouteBackendRsp`, and `GrpcRouteBackendRsp`. this is a small initial step towards simplifying code that must interact with the `MkStreamLabel` interface. Signed-off-by: katelyn martin <[email protected]>
our north star is making our metrics middleware easier to work with. we want `StreamLabel` to be boxable. this will let us avoid needing to thread baroque generics throughout our layer / service code, and instead rely on dynamic dispatch instead of static `L: StreamLabel` bounds. in order to move in that direction, we must take a small step back first. these traits must be object-safe in order to do that. for a trait to be object safe, per the Rust Reference, we may not use any function-level generics. accordingly, this moves the `<B>` bounds up to the top-level traits. this makes some of our type aliases icky, and introduces new generic parameters to various types/interfaces. we will be able to walk those back in subsequent commits, now that these traits can be placed in e.g. `Box<dyn StreamLabel>` values. NB: as this is an "Zwischenzug"¹ commit, we don't update the app crates. this commit will fail to compile if the entire workspace is built. Signed-off-by: katelyn martin <[email protected]> 1 - https://en.wikipedia.org/wiki/Zwischenzug
this commit updates the signature of `mk_stream_labeler()` and `init_response()` to instead examine the `http::request::Parts` and `http::response::Parts`, respectively. this is another step to simplify our label traits. no implementation of `MkStreamLabel` examines the body of the request. moreover, none even uses the `req` parameter at all! the intent of this signature seems reasonable however, we may want to look at certain requests and opt to ignore them. one could imagine an `l5d-*` header to refrain from installing telemetry on the response body, for example. however, as the last commit showed, we are subject to plenty of pain related to body generics. let's remove the need for them.
now that we have switched over to inspect the `Parts` head material of the requests and responses, we can remove these type parameters, whilst remaining object-safe. nice!
cratelyn
commented
Sep 27, 2024
Comment on lines
-79
to
+61
| fn init_response<B>(&mut self, rsp: &http::Response<B>); | ||
| fn init_response(&mut self, rsp: &http::response::Parts); |
There was a problem hiding this comment.
this is a core insight i want to highlight.
we don't need or want to see the response body in our implementations of this hook. we do want to e.g. record the start time, but for now, using Parts gives us most of the same information without having to lug around that darn B type parameter.
There was a problem hiding this comment.
ditto for MkStreamLabel. inspecting Parts means that we can have object-safe versions of these traits, crucial if we want to lean on type erasure and box these up over in our application code.
|
nb: this isn't expected to compile right now, i'm refraining from updating app code until the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
work in progress... 🚧
this is still in progress, but i'll start checking in incremental snapshots of progress here. 📸