refactor(http/prom): Simplify `record_response` middleware #3242

cratelyn · 2024-09-27T23:47:16Z

this branch makes a variety of small, non-breaking changes to the linkerd-http-prom crate, and the outbound proxy code related to route- and backend-level metrics.

this branch does not make particularly significant breaking changes to the StreamLabel or MkStreamLabel traits. this branch is focused on minimizing the type complexity of these traits, and consequently in dependent code.

these changes are provided in distinct, atomic commits. readers are encourages to review this branch commit-by-commit. the primary changes that are worth highlighting, mentioned in comments below are:

StreamLabel is now object-safe, making use of dyn StreamLabel possible. we now use request::Parts and response::Parts parameters in our trait methods. this means that we no longer need to be generic across request/response bodies, but retain the ability to inspect e.g. status codes.
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. these types are removed.
ResponseMetrics and RequestMetrics are generic over L: StreamLabel. these were previously generic over DurationLabels and StatusLabels, even though the same parent L was used in practice.
documentation to various interfaces is added.

cratelyn · 2024-09-27T23:49:11Z

linkerd/http/prom/src/record_response.rs

-    fn init_response<B>(&mut self, rsp: &http::Response<B>);
+    fn init_response(&mut self, rsp: &http::response::Parts);


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.

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.

This is awesome! Getting rid of the parameter simplified a lot by the looks of it.

cratelyn · 2024-09-27T23:56:54Z

nb: this isn't expected to compile right now, i'm refraining from updating app code until the linkerd-http-prom interfaces are more situated.

cratelyn · 2024-09-30T22:55:00Z

linkerd/http/prom/src/record_response/response.rs

-pub struct ResponseMetrics<DurL, StatL> {
-    duration: DurationFamily<DurL>,
-    statuses: Family<StatL, Counter>,
+pub struct ResponseMetrics<L: StreamLabel> {
+    duration: DurationFamily<L::DurationLabels>,
+    statuses: Family<L::StatusLabels, Counter>,


here's another core idea in this branch i'd like to highlight:

in the outbound proxy code today, we define an alias tying DurL and StatL to the same L: StreamLabel implementation. so, the two parameters here aren't giving us any benefit or serving any need, and we can simplify the code in this submodule (and the equivalent request.rs sibling) by making this generic across L rather than its underlying label types.

cratelyn · 2024-09-30T22:55:36Z

linkerd/http/prom/src/record_response/response.rs

-impl<DurL, StatL> Default for ResponseMetrics<DurL, StatL>
-where
-    StatL: EncodeLabelSet + Clone + Eq + std::fmt::Debug + std::hash::Hash + Send + Sync + 'static,
-    DurL: EncodeLabelSet + Clone + Eq + std::fmt::Debug + std::hash::Hash + Send + Sync + 'static,
-{
+impl<L: StreamLabel> Default for ResponseMetrics<L> {


👌 👩‍🍳 💋 voila!

Signed-off-by: katelyn martin <[email protected]>

`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 Signed-off-by: katelyn martin <[email protected]>

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. Signed-off-by: katelyn martin <[email protected]>

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! Signed-off-by: katelyn martin <[email protected]>

`L` and `M` are not contained within `NewRecordResponse`. this PhantomData marker exists to make this generic over the metric and label types it is generating. strictly speaking, there isn't really a `() -> (L, M)` function in play, though `ExtractParams` is _close_. this is a little bit subjective, but we can constrain this just as well with a `PhantomData<(L, M)>`. Signed-off-by: katelyn martin <[email protected]>

this commit updates the `MkStreamLabel` and `StreamLabel` trait implementations in the outbound proxy code to reflect their latest signatures. Signed-off-by: katelyn martin <[email protected]>

there is a lot of redirection caused by reëxporting `MkStreamLabel` from various `metrics.rs` places. this commit adds `MkStreamLabel` to the list of symbols exposed by the top-level namespace of `linkerd_http_prom`, and consistently imports that trait from there, instead of various local reëxports. Signed-off-by: katelyn martin <[email protected]>

this is another slightly subjective change. i had a fair amount of difficulty learning how the different pieces of the metrics infrastructure fit together. a personal roadblock to getting oriented is that many of the label and metrics structures are reëxported from various places, as are some of the traits provided by the underlying `linkerd-http-prom` library. this removes the `pub use` in the backend metrics submodule. this makes imports in tests mildly more involved, but has the upside that types live in, and are imported from, a single place. Signed-off-by: katelyn martin <[email protected]>

Signed-off-by: katelyn martin <[email protected]>

this removes a type parameter on `RequestMetrics` and `ResponseMetrics`, which in practice were being tied to the same `L: StreamLabel` implementation. this greatly simplifies code in the `record_response` middleware, and removes some type aliases from the route- and backend-level metrics submodules. see the diff sections below shaped like this: ```diff -impl<DurL, StatL> RequestMetrics<DurL, StatL> -where - DurL: EncodeLabelSet + Clone + Eq + std::fmt::Debug + std::hash::Hash + Send + Sync + 'static, - StatL: EncodeLabelSet + Clone + Eq + std::fmt::Debug + std::hash::Hash + Send + Sync + 'static, -{ +impl<L: StreamLabel> RequestMetrics<L> { ``` Signed-off-by: katelyn martin <[email protected]>

this adds some documentation, mostly pointing out that this submodule exists to provide trait implementations to encode label sets, for use in accessing metrics from a `Family`. Signed-off-by: katelyn martin <[email protected]>

this is a bit of a self-contained system, with `ResponseFuture` acting as the public face of it. let's codify that by making it its own submodule. Signed-off-by: katelyn martin <[email protected]>

cratelyn · 2024-10-01T20:35:37Z

linkerd/http/prom/src/record_response.rs

-    type DurationLabels: EncodeLabelSet
-        + Clone
-        + Eq
-        + std::fmt::Debug
-        + std::hash::Hash
-        + Send
-        + Sync
-        + 'static;
-    type StatusLabels: EncodeLabelSet
-        + Clone
-        + Eq
-        + std::fmt::Debug
-        + std::hash::Hash
-        + Send
-        + Sync
-        + 'static;
-
-    type StreamLabel: StreamLabel<
-        DurationLabels = Self::DurationLabels,
-        StatusLabels = Self::StatusLabels,
-    >;
+    type StreamLabel: StreamLabel;


much simpler! 🙂

Signed-off-by: katelyn martin <[email protected]>

sfleen

One minor nit, everything else looks great!

linkerd/http/prom/src/record_response.rs

sfleen · 2024-10-01T20:49:27Z

linkerd/http/prom/src/record_response.rs

-    fn init_response<B>(&mut self, rsp: &http::Response<B>);
+    fn init_response(&mut self, rsp: &http::response::Parts);


This is awesome! Getting rid of the parameter simplified a lot by the looks of it.

@sfleen

This reverts commit 9ea61d6. > I believe `fn() -> T` allows covariance over T without triggering `dropck`. I _think_ we'd want to keep that, since this produces `(L, M)` via the extract instead of storing it. @sfleen makes the astute point that the marker type has some covariance properties we should preserve. accordingly, this reverts the previous commit changing that `PhantomData`. #3242 (comment) Co-Authored-By: Scott Fleemer <[email protected]> Signed-off-by: katelyn martin <[email protected]>

cratelyn self-assigned this Sep 27, 2024

cratelyn commented Sep 27, 2024

View reviewed changes

cratelyn mentioned this pull request Sep 30, 2024

refactor(prom): Remove MkStreamLabel's associated types #3234

Closed

cratelyn force-pushed the kate/simplify-metrics-traits branch 3 times, most recently from 0b475f7 to a642395 Compare September 30, 2024 22:52

cratelyn commented Sep 30, 2024

View reviewed changes

cratelyn force-pushed the kate/simplify-metrics-traits branch 3 times, most recently from 26975e4 to 80de30c Compare October 1, 2024 20:12

cratelyn added 14 commits October 1, 2024 16:15

docs(app): Document StreamLabel grpc/http implementors

cb52f40

Signed-off-by: katelyn martin <[email protected]>

docs(prom): Document record_response interfaces

b984daa

Signed-off-by: katelyn martin <[email protected]>

refactor(app/outbound): Update MkStreamLabel and StreamLabel

951cc93

this commit updates the `MkStreamLabel` and `StreamLabel` trait implementations in the outbound proxy code to reflect their latest signatures. Signed-off-by: katelyn martin <[email protected]>

refactor(prom): Remove count_reqs reëxports

4375752

Signed-off-by: katelyn martin <[email protected]>

docs(app): Document route metrics' label set types

6dc17fc

this adds some documentation, mostly pointing out that this submodule exists to provide trait implementations to encode label sets, for use in accessing metrics from a `Family`. Signed-off-by: katelyn martin <[email protected]>

refactor(prom): Outline ResponseFuture into a submodule

f80f790

this is a bit of a self-contained system, with `ResponseFuture` acting as the public face of it. let's codify that by making it its own submodule. Signed-off-by: katelyn martin <[email protected]>

cratelyn force-pushed the kate/simplify-metrics-traits branch from 80de30c to f80f790 Compare October 1, 2024 20:15

cratelyn changed the title ~~wip(prom): Simplify metrics traits~~ refactor(http/prom): Simplify record_response middleware Oct 1, 2024

cratelyn marked this pull request as ready for review October 1, 2024 20:33

cratelyn requested a review from a team as a code owner October 1, 2024 20:33

cratelyn requested review from olix0r and sfleen October 1, 2024 20:34

cratelyn commented Oct 1, 2024

View reviewed changes

fix(prom): Fix rustdoc private item link

3d0a46e

Signed-off-by: katelyn martin <[email protected]>

sfleen approved these changes Oct 1, 2024

View reviewed changes

cratelyn marked this pull request as draft October 3, 2024 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(http/prom): Simplify `record_response` middleware #3242

refactor(http/prom): Simplify `record_response` middleware #3242

cratelyn commented Sep 27, 2024 •

edited

Loading

cratelyn Sep 27, 2024

cratelyn Sep 27, 2024

sfleen Oct 1, 2024

cratelyn commented Sep 27, 2024

cratelyn Sep 30, 2024

cratelyn Sep 30, 2024

cratelyn Oct 1, 2024

sfleen left a comment

sfleen Oct 1, 2024

		fn init_response<B>(&mut self, rsp: &http::Response<B>);
		fn init_response(&mut self, rsp: &http::response::Parts);

refactor(http/prom): Simplify record_response middleware #3242

Are you sure you want to change the base?

refactor(http/prom): Simplify record_response middleware #3242

Conversation

cratelyn commented Sep 27, 2024 • edited Loading

cratelyn Sep 27, 2024

Choose a reason for hiding this comment

cratelyn Sep 27, 2024

Choose a reason for hiding this comment

sfleen Oct 1, 2024

Choose a reason for hiding this comment

cratelyn commented Sep 27, 2024

cratelyn Sep 30, 2024

Choose a reason for hiding this comment

cratelyn Sep 30, 2024

Choose a reason for hiding this comment

cratelyn Oct 1, 2024

Choose a reason for hiding this comment

sfleen left a comment

Choose a reason for hiding this comment

sfleen Oct 1, 2024

Choose a reason for hiding this comment

refactor(http/prom): Simplify `record_response` middleware #3242

refactor(http/prom): Simplify `record_response` middleware #3242

cratelyn commented Sep 27, 2024 •

edited

Loading