From e190dae90098bd3df7b6388f7cc2a863d9cfdfc1 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Thu, 21 Nov 2024 09:56:49 -0500 Subject: [PATCH] refactor(app): Outline route label extractor (#3337) * refactor(app): Hoist label extractor out of retry middleware this commit promotes the `RetryLabelExtract` type out of the route retry middleware, and into the metrics submodule. in preparation for generalizing this component, we rename it to `RouteLabelExtract`. Signed-off-by: katelyn martin * refactor(app): Outline route label extraction this addresses the `TODO` comment, moving the `RouteLabelExtract` type out of the policy route layer function. this defines a method on `MatchedRoute` which may be called to retrieve a `RouteLabelExtract`. that type holds references to the contruction-time information about the route, and can be used to later inspect an outbound request at call-time, returning a `labels::Route` set of labels. this is a helpful piece of reusable glue that is broadly useful in instrumenting our route-level middleware. Signed-off-by: katelyn martin --------- Signed-off-by: katelyn martin --- .../outbound/src/http/logical/policy/route.rs | 13 +------ .../logical/policy/route/metrics/labels.rs | 37 ++++++++++++++++++- .../src/http/logical/policy/route/retry.rs | 24 +++--------- 3 files changed, 44 insertions(+), 30 deletions(-) diff --git a/linkerd/app/outbound/src/http/logical/policy/route.rs b/linkerd/app/outbound/src/http/logical/policy/route.rs index cc1033e4d3..fd6cbdebff 100644 --- a/linkerd/app/outbound/src/http/logical/policy/route.rs +++ b/linkerd/app/outbound/src/http/logical/policy/route.rs @@ -124,18 +124,9 @@ where .push_on_service(svc::LoadShed::layer()) .push(filters::NewApplyFilters::::layer()) .push({ - // TODO(kate): extracting route labels like this should ideally live somewhere - // else, like e.g. the `SetExtensions` middleware. - let mk_extract = |rt: &Self| { - let Route { - parent_ref, - route_ref, - .. - } = &rt.params; - retry::RetryLabelExtract(parent_ref.clone(), route_ref.clone()) - }; + let mk = Self::label_extractor; let metrics = metrics.retry.clone(); - retry::NewHttpRetry::layer_via_mk(mk_extract, metrics) + retry::NewHttpRetry::layer_via_mk(mk, metrics) }) .check_new::() .check_new_service::>() diff --git a/linkerd/app/outbound/src/http/logical/policy/route/metrics/labels.rs b/linkerd/app/outbound/src/http/logical/policy/route/metrics/labels.rs index fa355c2751..0378bf4a44 100644 --- a/linkerd/app/outbound/src/http/logical/policy/route/metrics/labels.rs +++ b/linkerd/app/outbound/src/http/logical/policy/route/metrics/labels.rs @@ -1,6 +1,7 @@ //! Prometheus label types. use linkerd_app_core::{ - dns, errors, metrics::prom::EncodeLabelSetMut, proxy::http, Error as BoxError, + dns, errors, metrics::prom::EncodeLabelSetMut, proxy::http, svc::ExtractParam, + Error as BoxError, }; use prometheus_client::encoding::*; @@ -39,6 +40,9 @@ pub struct GrpcRsp { pub error: Option, } +#[derive(Clone, Debug)] +pub struct RouteLabelExtract(pub ParentRef, pub RouteRef); + #[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)] pub enum Error { FailFast, @@ -211,6 +215,37 @@ impl EncodeLabelSet for GrpcRsp { } } +// === impl MatchedRoute === + +impl super::super::MatchedRoute { + /// Returns a [`RouteLabelExtract`]. + /// + /// The extractor returned by this function provides a [`ExtractParam`] implementation + /// that can be used to acquire the route-level labels corresponding to a given outbound + /// request. + pub(crate) fn label_extractor(&self) -> RouteLabelExtract { + use super::super::Route; + let Route { + parent_ref, + route_ref, + .. + } = &self.params; + + RouteLabelExtract(parent_ref.clone(), route_ref.clone()) + } +} + +// === impl RouteLabelExtract === + +impl ExtractParam> for RouteLabelExtract { + fn extract_param(&self, t: &http::Request) -> Route { + let Self(parent, route) = self; + let uri = t.uri(); + + Route::new(parent.clone(), route.clone(), uri) + } +} + // === impl Error === impl Error { diff --git a/linkerd/app/outbound/src/http/logical/policy/route/retry.rs b/linkerd/app/outbound/src/http/logical/policy/route/retry.rs index a6d856918e..fe54a08509 100644 --- a/linkerd/app/outbound/src/http/logical/policy/route/retry.rs +++ b/linkerd/app/outbound/src/http/logical/policy/route/retry.rs @@ -1,12 +1,14 @@ -use super::{extensions, metrics::labels::Route as RouteLabels}; -use crate::{ParentRef, RouteRef}; +use super::{ + extensions, + metrics::labels::{Route as RouteLabels, RouteLabelExtract}, +}; use futures::future::{Either, Ready}; use linkerd_app_core::{ cause_ref, classify, exp_backoff::ExponentialBackoff, is_caused_by, proxy::http::{self, stream_timeouts::ResponseTimeoutError}, - svc::{self, http::h2, ExtractParam}, + svc::{self, http::h2}, Error, Result, }; use linkerd_http_retry::{self as retry, peek_trailers::PeekTrailersBody}; @@ -18,7 +20,7 @@ use tokio::time; pub struct IsRetry(()); pub type NewHttpRetry = - retry::NewHttpRetry; + retry::NewHttpRetry; #[derive(Clone, Debug)] pub struct RetryPolicy { @@ -31,9 +33,6 @@ pub struct RetryPolicy { pub backoff: Option, } -#[derive(Clone, Debug)] -pub struct RetryLabelExtract(pub ParentRef, pub RouteRef); - pub type RouteRetryMetrics = retry::MetricFamilies; // === impl RetryPolicy === @@ -163,14 +162,3 @@ impl RetryPolicy { false } } - -// === impl RetryLabelExtract === - -impl ExtractParam> for RetryLabelExtract { - fn extract_param(&self, t: &http::Request) -> RouteLabels { - let Self(parent, route) = self; - let uri = t.uri(); - - RouteLabels::new(parent.clone(), route.clone(), uri) - } -}