refactor: FmtLabels impls use exhaustive bindings (#3988)

cratelyn · web-flow · commit 34b46ab6cd5f · 2025-07-03T11:56:14.000-04:00
this is based on #3987. in #3987 (_see https://github.com/linkerd/linkerd2/issues/13821_) we discovered that some of the types that implement [`FmtLabels`](https://github.com/linkerd/linkerd2-proxy/blob/085be9978d437800a1a8ff0c2457b9bb9712a166/linkerd/metrics/src/fmt.rs#L5) could collide when used in registry keys; i.e., they might emit identical label sets, but distinct `Hash` values. #3987 solves two bugs. this pull request proposes a follow-on change, introducing _exhaustive_ bindings to implementations of `FmtLabels`, to prevent this category of bug from reoccurring again in the future. this change means that the introduction of an additional field to any of these label structures, e.g. `OutboundEndpointLabels` or `HTTPLocalRateLimitLabels`, will cause a compilation error unless said new field is handled in the corresponding `FmtLabels` implementation. ### 🔖 a note in writing this pull request, i noticed one label that i believe is unintentionally being elided. i've refrained from changing behavior in this pull request. i do note it though, as an example of this syntax identifying the category of bug i hope to hedge against here. --- * fix: do not key transport metrics registry on `ClientTls` Signed-off-by: katelyn martin <kate@buoyant.io> * fix: do not key transport metrics registry on `ServerTls` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(transport-metrics): exhaustive `Eos: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `ServerLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `TlsAccept: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `TargetAddr: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(metrics): exhaustive `Label: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(http/metrics): exhaustive `Status: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `ControlLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `ProfileRouteLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `InboundEndpointLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `ServerLabel: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `ServerAuthzLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `RouteLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `RouteAuthzLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `OutboundEndpointLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `Authority: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `StackLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/inbound): exhaustive `HTTPLocalRateLimitLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/inbound): exhaustive `Key<L>: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * nit(metrics): remove redundant banner comment these impl blocks are all `FmtLabels`, following another series of the same, above. we don't need another one of these comments. Signed-off-by: katelyn martin <kate@buoyant.io> * nit(metrics): exhaustive `AndThen: FmtMetrics` Signed-off-by: katelyn martin <kate@buoyant.io> * nit(app/core): note unused label see #3262 (618838e), which introduced this label. to preserve behavior, this label remains unused. X-Ref: #3262 Signed-off-by: katelyn martin <kate@buoyant.io> --------- Signed-off-by: katelyn martin <kate@buoyant.io>
diff --git a/linkerd/app/core/src/metrics.rs b/linkerd/app/core/src/metrics.rs
@@ -250,8 +250,10 @@ impl svc::Param<ControlLabels> for control::ControlAddr {
 
 impl FmtLabels for ControlLabels {
     fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "addr=\"{}\",", self.addr)?;
-        TlsConnect::from(&self.server_id).fmt_labels(f)?;
+        let Self { addr, server_id } = self;
+
+        write!(f, "addr=\"{}\",", addr)?;
+        TlsConnect::from(server_id).fmt_labels(f)?;
 
         Ok(())
     }
@@ -281,10 +283,16 @@ impl ProfileRouteLabels {
 
 impl FmtLabels for ProfileRouteLabels {
     fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        self.direction.fmt_labels(f)?;
-        write!(f, ",dst=\"{}\"", self.addr)?;
+        let Self {
+            direction,
+            addr,
+            labels,
+        } = self;
+
+        direction.fmt_labels(f)?;
+        write!(f, ",dst=\"{}\"", addr)?;
 
-        if let Some(labels) = self.labels.as_ref() {
+        if let Some(labels) = labels.as_ref() {
             write!(f, ",{}", labels)?;
         }
 
@@ -317,30 +325,34 @@ impl FmtLabels for EndpointLabels {
 
 impl FmtLabels for InboundEndpointLabels {
     fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        if let Some(a) = self.authority.as_ref() {
+        let Self {
+            tls,
+            authority,
+            target_addr,
+            policy,
+        } = self;
+
+        if let Some(a) = authority.as_ref() {
             Authority(a).fmt_labels(f)?;
             write!(f, ",")?;
         }
 
-        (
-            (TargetAddr(self.target_addr), TlsAccept::from(&self.tls)),
-            &self.policy,
-        )
-            .fmt_labels(f)?;
+        ((TargetAddr(*target_addr), TlsAccept::from(tls)), policy).fmt_labels(f)?;
 
         Ok(())
     }
 }
 
 impl FmtLabels for ServerLabel {
     fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let Self(meta, port) = self;
         write!(
             f,
             "srv_group=\"{}\",srv_kind=\"{}\",srv_name=\"{}\",srv_port=\"{}\"",
-            self.0.group(),
-            self.0.kind(),
-            self.0.name(),
-            self.1
+            meta.group(),
+            meta.kind(),
+            meta.name(),
+            port
         )
     }
 }
@@ -364,39 +376,45 @@ impl prom::EncodeLabelSetMut for ServerLabel {
 
 impl FmtLabels for ServerAuthzLabels {
     fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        self.server.fmt_labels(f)?;
+        let Self { server, authz } = self;
+
+        server.fmt_labels(f)?;
         write!(
             f,
             ",authz_group=\"{}\",authz_kind=\"{}\",authz_name=\"{}\"",
-            self.authz.group(),
-            self.authz.kind(),
-            self.authz.name()
+            authz.group(),
+            authz.kind(),
+            authz.name()
         )
     }
 }
 
 impl FmtLabels for RouteLabels {
     fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        self.server.fmt_labels(f)?;
+        let Self { server, route } = self;
+
+        server.fmt_labels(f)?;
         write!(
             f,
             ",route_group=\"{}\",route_kind=\"{}\",route_name=\"{}\"",
-            self.route.group(),
-            self.route.kind(),
-            self.route.name(),
+            route.group(),
+            route.kind(),
+            route.name(),
         )
     }
 }
 
 impl FmtLabels for RouteAuthzLabels {
     fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        self.route.fmt_labels(f)?;
+        let Self { route, authz } = self;
+
+        route.fmt_labels(f)?;
         write!(
             f,
             ",authz_group=\"{}\",authz_kind=\"{}\",authz_name=\"{}\"",
-            self.authz.group(),
-            self.authz.kind(),
-            self.authz.name(),
+            authz.group(),
+            authz.kind(),
+            authz.name(),
         )
     }
 }
@@ -409,16 +427,25 @@ impl svc::Param<OutboundZoneLocality> for OutboundEndpointLabels {
 
 impl FmtLabels for OutboundEndpointLabels {
     fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        if let Some(a) = self.authority.as_ref() {
+        let Self {
+            server_id,
+            authority,
+            labels,
+            // TODO(kate): this label is not currently emitted.
+            zone_locality: _,
+            target_addr,
+        } = self;
+
+        if let Some(a) = authority.as_ref() {
             Authority(a).fmt_labels(f)?;
             write!(f, ",")?;
         }
 
-        let ta = TargetAddr(self.target_addr);
-        let tls = TlsConnect::from(&self.server_id);
+        let ta = TargetAddr(*target_addr);
+        let tls = TlsConnect::from(server_id);
         (ta, tls).fmt_labels(f)?;
 
-        if let Some(labels) = self.labels.as_ref() {
+        if let Some(labels) = labels.as_ref() {
             write!(f, ",{}", labels)?;
         }
 
@@ -443,7 +470,8 @@ impl FmtLabels for Direction {
 
 impl FmtLabels for Authority<'_> {
     fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "authority=\"{}\"", self.0)
+        let Self(authority) = self;
+        write!(f, "authority=\"{}\"", authority)
     }
 }
 
@@ -498,7 +526,13 @@ impl StackLabels {
 
 impl FmtLabels for StackLabels {
     fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        self.direction.fmt_labels(f)?;
-        write!(f, ",protocol=\"{}\",name=\"{}\"", self.protocol, self.name)
+        let Self {
+            direction,
+            protocol,
+            name,
+        } = self;
+
+        direction.fmt_labels(f)?;
+        write!(f, ",protocol=\"{}\",name=\"{}\"", protocol, name)
     }
 }
diff --git a/linkerd/app/core/src/transport/labels.rs b/linkerd/app/core/src/transport/labels.rs
@@ -99,14 +99,17 @@ impl ServerLabels {
 
 impl FmtLabels for ServerLabels {
     fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        self.direction.fmt_labels(f)?;
+        let Self {
+            direction,
+            tls,
+            target_addr,
+            policy,
+        } = self;
+
+        direction.fmt_labels(f)?;
         f.write_str(",peer=\"src\",")?;
 
-        (
-            (TargetAddr(self.target_addr), TlsAccept(&self.tls)),
-            self.policy.as_ref(),
-        )
-            .fmt_labels(f)?;
+        ((TargetAddr(*target_addr), TlsAccept(tls)), policy.as_ref()).fmt_labels(f)?;
 
         Ok(())
     }
@@ -122,7 +125,8 @@ impl<'t> From<&'t tls::ConditionalServerTlsLabels> for TlsAccept<'t> {
 
 impl FmtLabels for TlsAccept<'_> {
     fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self.0 {
+        let Self(tls) = self;
+        match tls {
             Conditional::None(tls::NoServerTls::Disabled) => {
                 write!(f, "tls=\"disabled\"")
             }
@@ -170,12 +174,13 @@ impl FmtLabels for TlsConnect<'_> {
 
 impl FmtLabels for TargetAddr {
     fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let Self(target_addr) = self;
         write!(
             f,
             "target_addr=\"{}\",target_ip=\"{}\",target_port=\"{}\"",
-            self.0,
-            self.0.ip(),
-            self.0.port()
+            target_addr,
+            target_addr.ip(),
+            target_addr.port()
         )
     }
 }
diff --git a/linkerd/app/inbound/src/metrics/authz.rs b/linkerd/app/inbound/src/metrics/authz.rs
@@ -251,18 +251,24 @@ impl FmtMetrics for TcpAuthzMetrics {
 
 impl FmtLabels for HTTPLocalRateLimitLabels {
     fn fmt_labels(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        self.server.fmt_labels(f)?;
-        if let Some(rl) = &self.rate_limit {
+        let Self {
+            server,
+            rate_limit,
+            scope,
+        } = self;
+
+        server.fmt_labels(f)?;
+        if let Some(rl) = rate_limit {
             write!(
                 f,
                 ",ratelimit_group=\"{}\",ratelimit_kind=\"{}\",ratelimit_name=\"{}\",ratelimit_scope=\"{}\"",
                 rl.group(),
                 rl.kind(),
                 rl.name(),
-                self.scope,
+                scope,
             )
         } else {
-            write!(f, ",ratelimit_scope=\"{}\"", self.scope)
+            write!(f, ",ratelimit_scope=\"{}\"", scope)
         }
     }
 }
@@ -281,7 +287,13 @@ impl<L> Key<L> {
 
 impl<L: FmtLabels> FmtLabels for Key<L> {
     fn fmt_labels(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        (self.target, (&self.labels, TlsAccept(&self.tls))).fmt_labels(f)
+        let Self {
+            target,
+            tls,
+            labels,
+        } = self;
+
+        (target, (labels, TlsAccept(tls))).fmt_labels(f)
     }
 }
 
diff --git a/linkerd/http/metrics/src/requests/report.rs b/linkerd/http/metrics/src/requests/report.rs
@@ -146,6 +146,7 @@ where
 
 impl FmtLabels for Status {
     fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "status_code=\"{}\"", self.0.as_u16())
+        let Self(status) = self;
+        write!(f, "status_code=\"{}\"", status.as_u16())
     }
 }
diff --git a/linkerd/metrics/src/fmt.rs b/linkerd/metrics/src/fmt.rs
@@ -188,8 +188,6 @@ impl<A: FmtLabels, B: FmtLabels> FmtLabels for (Option<A>, B) {
     }
 }
 
-// === impl FmtMetrics ===
-
 impl<'a, A: FmtMetrics + 'a> FmtMetrics for &'a A {
     #[inline]
     fn fmt_metrics(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
@@ -210,8 +208,10 @@ impl<M: FmtMetrics> FmtMetrics for Option<M> {
 impl<A: FmtMetrics, B: FmtMetrics> FmtMetrics for AndThen<A, B> {
     #[inline]
     fn fmt_metrics(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        self.0.fmt_metrics(f)?;
-        self.1.fmt_metrics(f)?;
+        let Self(a, b) = self;
+
+        a.fmt_metrics(f)?;
+        b.fmt_metrics(f)?;
 
         Ok(())
     }
diff --git a/linkerd/metrics/src/histogram.rs b/linkerd/metrics/src/histogram.rs
@@ -224,7 +224,8 @@ impl<V: Into<u64>, F: Factor> FmtMetric for Histogram<V, F> {
 
 impl<K: fmt::Display, V: fmt::Display> FmtLabels for Label<K, V> {
     fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "{}=\"{}\"", self.0, self.1)
+        let Self(k, v) = self;
+        write!(f, "{}=\"{}\"", k, v)
     }
 }
 
diff --git a/linkerd/transport-metrics/src/lib.rs b/linkerd/transport-metrics/src/lib.rs
@@ -82,7 +82,8 @@ impl<K: Eq + Hash + FmtLabels> Registry<K> {
 
 impl FmtLabels for Eos {
     fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self.0 {
+        let Self(errno) = self;
+        match errno {
             None => f.pad("errno=\"\""),
             Some(errno) => write!(f, "errno=\"{}\"", errno),
         }

Original file line number	Diff line number	Diff line change
`@@ -146,6 +146,7 @@ where`
`146`	`146`
`147`	`147`	`impl FmtLabels for Status {`
`148`	`148`	`fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {`
`149`		`- write!(f, "status_code=\"{}\"", self.0.as_u16())`
	`149`	`+ let Self(status) = self;`
	`150`	`+ write!(f, "status_code=\"{}\"", status.as_u16())`
`150`	`151`	`}`
`151`	`152`	`}`
Original file line number	Diff line number	Diff line change
`@@ -188,8 +188,6 @@ impl<A: FmtLabels, B: FmtLabels> FmtLabels for (Option<A>, B) {`
`188`	`188`	`}`
`189`	`189`	`}`
`190`	`190`
`191`		`-// === impl FmtMetrics ===`
`192`		`-`
`193`	`191`	`impl<'a, A: FmtMetrics + 'a> FmtMetrics for &'a A {`
`194`	`192`	`#[inline]`
`195`	`193`	`fn fmt_metrics(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {`
`@@ -210,8 +208,10 @@ impl<M: FmtMetrics> FmtMetrics for Option<M> {`
`210`	`208`	`impl<A: FmtMetrics, B: FmtMetrics> FmtMetrics for AndThen<A, B> {`
`211`	`209`	`#[inline]`
`212`	`210`	`fn fmt_metrics(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {`
`213`		`- self.0.fmt_metrics(f)?;`
`214`		`- self.1.fmt_metrics(f)?;`
	`211`	`+ let Self(a, b) = self;`
	`212`	`+`
	`213`	`+ a.fmt_metrics(f)?;`
	`214`	`+ b.fmt_metrics(f)?;`
`215`	`215`
`216`	`216`	`Ok(())`
`217`	`217`	`}`
Original file line number	Diff line number	Diff line change
`@@ -224,7 +224,8 @@ impl<V: Into<u64>, F: Factor> FmtMetric for Histogram<V, F> {`
`224`	`224`
`225`	`225`	`impl<K: fmt::Display, V: fmt::Display> FmtLabels for Label<K, V> {`
`226`	`226`	`fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {`
`227`		`- write!(f, "{}=\"{}\"", self.0, self.1)`
	`227`	`+ let Self(k, v) = self;`
	`228`	`+ write!(f, "{}=\"{}\"", k, v)`
`228`	`229`	`}`
`229`	`230`	`}`
`230`	`231`
Original file line number	Diff line number	Diff line change
`@@ -82,7 +82,8 @@ impl<K: Eq + Hash + FmtLabels> Registry<K> {`
`82`	`82`
`83`	`83`	`impl FmtLabels for Eos {`
`84`	`84`	`fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {`
`85`		`- match self.0 {`
	`85`	`+ let Self(errno) = self;`
	`86`	`+ match errno {`
`86`	`87`	`None => f.pad("errno=\"\""),`
`87`	`88`	`Some(errno) => write!(f, "errno=\"{}\"", errno),`
`88`	`89`	`}`