Skip to content

Conversation

@cratelyn
Copy link
Member

@cratelyn cratelyn commented Sep 22, 2025

#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.

The typical pattern for this would be to have a dump bespoke param in this module like:

pub enum MetricsVariant { Http, Grpc }

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 explore what appears to be a promising alternative to the existing model of 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>>.

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 <kate@buoyant.io>
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 <kate@buoyant.io>
@cratelyn cratelyn self-assigned this Sep 22, 2025
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 <kate@buoyant.io>
these now accept specifically the variant, rather than the permitted
target.

Signed-off-by: katelyn martin <kate@buoyant.io>
this type now can extract metrics from arbitrary targets.

Signed-off-by: katelyn martin <kate@buoyant.io>
this type now can extract metrics from arbitrary targets.

Signed-off-by: katelyn martin <kate@buoyant.io>
Signed-off-by: katelyn martin <kate@buoyant.io>
@cratelyn cratelyn force-pushed the kate/app-inbound.router-metrics-middleware-is-target-agnostic branch from 5cb38d8 to 8f8a5fe Compare September 22, 2025 20:45
@cratelyn cratelyn marked this pull request as ready for review September 22, 2025 20:49
@cratelyn cratelyn requested a review from a team as a code owner September 22, 2025 20:49
@cratelyn cratelyn merged commit f2e5da3 into main Sep 24, 2025
15 checks passed
@cratelyn cratelyn deleted the kate/app-inbound.router-metrics-middleware-is-target-agnostic branch September 24, 2025 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants