-
Notifications
You must be signed in to change notification settings - Fork 283
feat(app/inbound): request body frame size metrics #4180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
) #4119 introduced a `Permitted<T>` structure, to provide a more formal notion of a `(HttpRoutePermit, T)` tuple. during review of #4180, we had a discussion about how the inbound proxy's http router had metrics layers which were tightly coupled to the notion of a `Permitted<T>`: > This isn't a hard blocker--but this is unfortunate that we're coupling > this impl to the Permitted target type -- target types are usually an > _internal implementation detail_ and not a public api that is depended > on elsewhere. > > ... > > and then have our Permitted impl `Param<MetricsVariant> for Permitted<T>`. > > then this module remains decoupled from the concrete target types. > > That is, the metrics module is decoupled from _where it's called_--as > long as the caller provides a target that can give us what we need to > build our extractors, we're good to go. this branch performs a series of changes to decouple our metric layers' extractors from `Permitted<T>`. importantly, this has the effect of leaving the inbound metrics layers' extractors agnostic to their target: they now provide implementations like `svc::ExtractParam<BodyDataMetrics, T>` rather than `svc::ExtractParam<BodyDataMetrics, Permitted<T>>`. see #4180. --- * refactor(app/inbound): `Permitted<T>` is a struct this restructures `Permitted<T>` so that it is no longer an enum. we instead define a "dumb" enum that represents the grpc/http dichotomy, and return to a model in which `Permitted<T>` is a `struct`. Signed-off-by: katelyn martin <[email protected]> * refactor(app/inbound): `Permitted<T>` is not `T: Param<P>` in order to carve out space to allow us to define other `Param<P>` implementations for `Permitted<T>`, we remove this blanket deference to the inner `T` target. Signed-off-by: katelyn martin <[email protected]> * refactor(app/inbound): `Permitted<T>` exposes `Param<P>` impls we cannot expose `T` or any of its own parameters, but we can expose the permit, its variant, and its labels. Signed-off-by: katelyn martin <[email protected]> * refactor(app/inbound): `family` lookups accept `PermitVariant` these now accept specifically the variant, rather than the permitted target. Signed-off-by: katelyn martin <[email protected]> * refactor(app/inbound): `ExtractRecordBodyDataMetrics` is `T`-agnostic this type now can extract metrics from arbitrary targets. Signed-off-by: katelyn martin <[email protected]> * refactor(app/inbound): `ExtractRequestCount` is `T`-agnostic this type now can extract metrics from arbitrary targets. Signed-off-by: katelyn martin <[email protected]> * refactor(app/inbound): `LogicalPerRequest::from` is `T`-agnostic Signed-off-by: katelyn martin <[email protected]> --------- Signed-off-by: katelyn martin <[email protected]>
13220e8 to
8a313b9
Compare
402ab4a to
e8bb635
Compare
this structure representing the frame size metrics is shared by the request and response instrumentation. the `body_data::request` and `body_data::response` submodules reëxport this, because the outbound proxy which uses these middleware separately in route-level and backend-level metrics layers. the inbound proxy, however, uses them each in the same place (see #4180), which is ergonomically odd. this commit introduces a top-level `pub use`, exposing this metrics structure from `body_data`, for consumers that instrument request and response bodies in one place. Signed-off-by: katelyn martin <[email protected]>
this structure representing the frame size metrics is shared by the request and response instrumentation. the `body_data::request` and `body_data::response` submodules reëxport this, because the outbound proxy which uses these middleware separately in route-level and backend-level metrics layers. the inbound proxy, however, uses them each in the same place (see #4180), which is ergonomically odd. this commit introduces a top-level `pub use`, exposing this metrics structure from `body_data`, for consumers that instrument request and response bodies in one place. Signed-off-by: katelyn martin <[email protected]>
see #4180. we will shortly introduce an additional layer of body frame size instrumentation. in order to avoid ambiguity in our extractor's names, we rename this type to `ExtractResponseBodyDataMetrics`, to indicate that it extracts _response_ body metrics. Signed-off-by: katelyn martin <[email protected]>
this structure representing the frame size metrics is shared by the request and response instrumentation. the `body_data::request` and `body_data::response` submodules reëxport this, because the outbound proxy which uses these middleware separately in route-level and backend-level metrics layers. the inbound proxy, however, uses them each in the same place (see #4180), which is ergonomically odd. this commit introduces a top-level `pub use`, exposing this metrics structure from `body_data`, for consumers that instrument request and response bodies in one place. Signed-off-by: katelyn martin <[email protected]>
see #4180. we will shortly introduce an additional layer of body frame size instrumentation. in order to avoid ambiguity in our extractor's names, we rename this type to `ExtractResponseBodyDataMetrics`, to indicate that it extracts _response_ body metrics. Signed-off-by: katelyn martin <[email protected]>
0dea9d1 to
7f7ac1e
Compare
this structure representing the frame size metrics is shared by the request and response instrumentation. the `body_data::request` and `body_data::response` submodules reëxport this, because the outbound proxy which uses these middleware separately in route-level and backend-level metrics layers. the inbound proxy, however, uses them each in the same place (see #4180), which is ergonomically odd. this commit introduces a top-level `pub use`, exposing this metrics structure from `body_data`, for consumers that instrument request and response bodies in one place. Signed-off-by: katelyn martin <[email protected]>
see #4180. to make way for the use of request body metrics middleware, we use further qualified paths to `body_data::response::NewRecordBodyData`. to be consistent across the board, we apply this change to the request counting middleware as well. NB: this is based upon #4186. Signed-off-by: katelyn martin <[email protected]>
our metrics layer is generic across an `N`-typed service factory. this commit introduces types aliases that apply `X`-typed extractor parameters to the `linkerd-http-prom` middleware, which are unaware of a concrete metrics extractor. we use this to reduce the complexity of the `layer()` function's type signature. see #4180. NB: based upon #4188, and #4186. Signed-off-by: katelyn martin <[email protected]>
this structure representing the frame size metrics is shared by the request and response instrumentation. the `body_data::request` and `body_data::response` submodules reëxport this, because the outbound proxy which uses these middleware separately in route-level and backend-level metrics layers. the inbound proxy, however, uses them each in the same place (see #4180), which is ergonomically odd. this commit introduces a top-level `pub use`, exposing this metrics structure from `body_data`, for consumers that instrument request and response bodies in one place. Signed-off-by: katelyn martin <[email protected]>
see #4180. we will shortly introduce an additional layer of body frame size instrumentation. in order to avoid ambiguity in our extractor's names, we rename this type to `ExtractResponseBodyDataMetrics`, to indicate that it extracts _response_ body metrics. Signed-off-by: katelyn martin <[email protected]>
7f7ac1e to
c9b8859
Compare
see #4180. to make way for the use of request body metrics middleware, we use further qualified paths to `body_data::response::NewRecordBodyData`. to be consistent across the board, we apply this change to the request counting middleware as well. NB: this is based upon #4186. Signed-off-by: katelyn martin <[email protected]>
our metrics layer is generic across an `N`-typed service factory. this commit introduces types aliases that apply `X`-typed extractor parameters to the `linkerd-http-prom` middleware, which are unaware of a concrete metrics extractor. we use this to reduce the complexity of the `layer()` function's type signature. see #4180. NB: based upon #4188, and #4186. Signed-off-by: katelyn martin <[email protected]>
c9b8859 to
cb68324
Compare
this structure representing the frame size metrics is shared by the request and response instrumentation. the `body_data::request` and `body_data::response` submodules reëxport this, because the outbound proxy which uses these middleware separately in route-level and backend-level metrics layers. the inbound proxy, however, uses them each in the same place (see #4180), which is ergonomically odd. this commit introduces a top-level `pub use`, exposing this metrics structure from `body_data`, for consumers that instrument request and response bodies in one place. Signed-off-by: katelyn martin <[email protected]>
see #4180. we will shortly introduce an additional layer of body frame size instrumentation. in order to avoid ambiguity in our extractor's names, we rename this type to `ExtractResponseBodyDataMetrics`, to indicate that it extracts _response_ body metrics. Signed-off-by: katelyn martin <[email protected]>
see #4180. to make way for the use of request body metrics middleware, we use further qualified paths to `body_data::response::NewRecordBodyData`. to be consistent across the board, we apply this change to the request counting middleware as well. NB: this is based upon #4186. Signed-off-by: katelyn martin <[email protected]>
see #4180. to make way for the use of request body metrics middleware, we use further qualified paths to `body_data::response::NewRecordBodyData`. to be consistent across the board, we apply this change to the request counting middleware as well. NB: this is based upon #4186. Signed-off-by: katelyn martin <[email protected]>
our metrics layer is generic across an `N`-typed service factory. this commit introduces types aliases that apply `X`-typed extractor parameters to the `linkerd-http-prom` middleware, which are unaware of a concrete metrics extractor. we use this to reduce the complexity of the `layer()` function's type signature. see #4180. NB: based upon #4188, and #4186. Signed-off-by: katelyn martin <[email protected]>
see #4180. to make way for the use of request body metrics middleware, we use further qualified paths to `body_data::response::NewRecordBodyData`. to be consistent across the board, we apply this change to the request counting middleware as well. NB: this is based upon #4186. Signed-off-by: katelyn martin <[email protected]>
our metrics layer is generic across an `N`-typed service factory. this commit introduces types aliases that apply `X`-typed extractor parameters to the `linkerd-http-prom` middleware, which are unaware of a concrete metrics extractor. we use this to reduce the complexity of the `layer()` function's type signature. see #4180. NB: based upon #4188, and #4186. Signed-off-by: katelyn martin <[email protected]>
cb68324 to
7c57384
Compare
our metrics layer is generic across an `N`-typed service factory. this commit introduces types aliases that apply `X`-typed extractor parameters to the `linkerd-http-prom` middleware, which are unaware of a concrete metrics extractor. we use this to reduce the complexity of the `layer()` function's type signature. see #4180. NB: based upon #4188, and #4186. Signed-off-by: katelyn martin <[email protected]>
this commit introduces an additional layer of telemetry to the inbound proxy's http router. either http and grpc metrics are used, depending upon the policy that authorized a given request. this is based upon #4174, which refactored the request body telemetry middleware to be metrics agnostic. see: * #4188 * #4187 * #4186 * #4174 * #4165 * #4166 * #4127 Signed-off-by: katelyn martin <[email protected]>
7c57384 to
049a8de
Compare
| impl EncodeLabelSetMut for RequestBodyDataLabels { | ||
| fn encode_label_set(&self, enc: &mut LabelSetEncoder<'_>) -> std::fmt::Result { | ||
| use encoding::EncodeLabel as _; | ||
|
|
||
| let Self { | ||
| route: | ||
| RouteLabels { | ||
| server: ServerLabel(parent, port), | ||
| route, | ||
| }, | ||
| } = self; | ||
|
|
||
| ("parent_group", parent.group()).encode(enc.encode_label())?; | ||
| ("parent_kind", parent.kind()).encode(enc.encode_label())?; | ||
| ("parent_name", parent.name()).encode(enc.encode_label())?; | ||
| ("parent_port", *port).encode(enc.encode_label())?; | ||
|
|
||
| ("route_group", route.group()).encode(enc.encode_label())?; | ||
| ("route_kind", route.kind()).encode(enc.encode_label())?; | ||
| ("route_name", route.name()).encode(enc.encode_label())?; | ||
|
|
||
| Ok(()) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why we need a new labels impl. Can we call RouteLabels::encode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
within the scope of this pull request, because i'm following the same pattern that was established in #4127 and #4165.
zooming out further, because linkerd_app_core's RouteLabels structure does not implement EncodeLabelSet. you may be thinking of the similarly named types in linkerd/app/outbound/src/opaq/logical/route.rs and linkerd/app/outbound/src/tls/logical/route.rs?
i do, fwiw, think that there's an unfortunately high amount of boilerplate in each of these layers. do you think we should follow the example to the outbound proxy, and introduce a RouteLabels structure here that does offer an encode() implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should reduce the boilerplate but it's not a blocker.
in #4180, we noted during review that encoding prometheus labels for route and parent metadata entails a slightly unfortunate amount of repetitive boilerplate. in the outbound proxy, we define a local `RouteLabels` type that implements the `EncodeLabelSet` and `EncodeLabelSetMut`, which we use to integrate label sets with the `prometheus_client` SDK. this commit introduces an equivalent pattern to the inbound proxy. a `RouteLabels` newtype wrapper over the shared `linkerd_app_core` structure is introduced, which in turn simplifies the `EncodeLabelSet` and `EncodeLabelSetMut` implementations of the `RequestCountLabels`, `ResponseBodyDataLabels`, and `RequestBodyDataLabels` types. Signed-off-by: katelyn martin <[email protected]>
in #4180, we noted during review that encoding prometheus labels for route and parent metadata entails a slightly unfortunate amount of repetitive boilerplate. in the outbound proxy, we define a local `RouteLabels` type that implements the `EncodeLabelSet` and `EncodeLabelSetMut`, which we use to integrate label sets with the `prometheus_client` SDK. this commit introduces an equivalent pattern to the inbound proxy. a `RouteLabels` newtype wrapper over the shared `linkerd_app_core` structure is introduced, which in turn simplifies the `EncodeLabelSet` and `EncodeLabelSetMut` implementations of the `RequestCountLabels`, `ResponseBodyDataLabels`, and `RequestBodyDataLabels` types. Signed-off-by: katelyn martin <[email protected]>
this commit introduces an additional layer of telemetry to the inbound
proxy's http router.
either http and grpc metrics are used, depending upon the policy that
authorized a given request.
this is based upon #4174, which refactored the
request body telemetry middleware to be metrics agnostic.
this change is based upon a collection of small prerequisite changes:
ExtractResponseBodyDataMetrics#4187body_dataexportsBodyDataMetrics#4186request::NewRecordBodyDatais metrics agnostic #4174related previous work:
InboundMetrics#4166Signed-off-by: katelyn martin [email protected]