From ea9c92f7c230cf3e741db63bfd234898373b279d Mon Sep 17 00:00:00 2001 From: Bruce Chen Date: Tue, 12 Sep 2023 14:23:46 +0200 Subject: [PATCH 1/4] feat: customize metric for personio use cases --- .github/workflows/release.yml | 2 +- .idea/.gitignore | 8 ++++++ .idea/linkerd2-proxy.iml | 9 +++++++ .idea/modules.xml | 8 ++++++ .idea/php.xml | 12 +++++++++ .idea/vcs.xml | 6 +++++ linkerd/app/admin/src/stack.rs | 2 -- linkerd/app/core/src/metrics.rs | 25 ++----------------- linkerd/app/inbound/src/http/router.rs | 2 -- .../app/integration/src/tests/discovery.rs | 2 -- .../app/integration/src/tests/telemetry.rs | 2 -- .../src/tests/telemetry/tcp_errors.rs | 4 +-- linkerd/app/outbound/src/http/concrete.rs | 16 +++++++++--- .../app/outbound/src/http/endpoint/tests.rs | 2 -- linkerd/app/outbound/src/opaq/concrete.rs | 7 ------ 15 files changed, 59 insertions(+), 48 deletions(-) create mode 100644 .idea/.gitignore create mode 100644 .idea/linkerd2-proxy.iml create mode 100644 .idea/modules.xml create mode 100644 .idea/php.xml create mode 100644 .idea/vcs.xml diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 468fd46c80..5d6434bfee 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -18,7 +18,7 @@ env: CARGO_NET_RETRY: 10 CHECKSEC_VERSION: 2.5.0 RUSTFLAGS: "-D warnings -A deprecated" - RUSTUP_MAX_RETRIES: 10 + RUSTUP_MAX_RETRIES: 11 jobs: meta: diff --git a/.idea/.gitignore b/.idea/.gitignore new file mode 100644 index 0000000000..13566b81b0 --- /dev/null +++ b/.idea/.gitignore @@ -0,0 +1,8 @@ +# Default ignored files +/shelf/ +/workspace.xml +# Editor-based HTTP Client requests +/httpRequests/ +# Datasource local storage ignored files +/dataSources/ +/dataSources.local.xml diff --git a/.idea/linkerd2-proxy.iml b/.idea/linkerd2-proxy.iml new file mode 100644 index 0000000000..d6ebd48059 --- /dev/null +++ b/.idea/linkerd2-proxy.iml @@ -0,0 +1,9 @@ + + + + + + + + + \ No newline at end of file diff --git a/.idea/modules.xml b/.idea/modules.xml new file mode 100644 index 0000000000..a38bd29c15 --- /dev/null +++ b/.idea/modules.xml @@ -0,0 +1,8 @@ + + + + + + + + \ No newline at end of file diff --git a/.idea/php.xml b/.idea/php.xml new file mode 100644 index 0000000000..f5f27444bd --- /dev/null +++ b/.idea/php.xml @@ -0,0 +1,12 @@ + + + + + + + + + \ No newline at end of file diff --git a/.idea/vcs.xml b/.idea/vcs.xml new file mode 100644 index 0000000000..35eb1ddfbb --- /dev/null +++ b/.idea/vcs.xml @@ -0,0 +1,6 @@ + + + + + + \ No newline at end of file diff --git a/linkerd/app/admin/src/stack.rs b/linkerd/app/admin/src/stack.rs index 571f8915e0..1e28ae0295 100644 --- a/linkerd/app/admin/src/stack.rs +++ b/linkerd/app/admin/src/stack.rs @@ -225,8 +225,6 @@ impl Param for Permitted { fn param(&self) -> metrics::EndpointLabels { metrics::InboundEndpointLabels { tls: self.http.tcp.tls.clone(), - authority: None, - target_addr: self.http.tcp.addr.into(), policy: self.permit.labels.clone(), } .into() diff --git a/linkerd/app/core/src/metrics.rs b/linkerd/app/core/src/metrics.rs index baee0aa785..5f937ea836 100644 --- a/linkerd/app/core/src/metrics.rs +++ b/linkerd/app/core/src/metrics.rs @@ -19,7 +19,6 @@ pub use linkerd_metrics::*; use linkerd_proxy_server_policy as policy; use std::{ fmt::{self, Write}, - net::SocketAddr, sync::Arc, time::Duration, }; @@ -66,8 +65,6 @@ pub enum EndpointLabels { #[derive(Clone, Debug, PartialEq, Eq, Hash)] pub struct InboundEndpointLabels { pub tls: tls::ConditionalServerTls, - pub authority: Option, - pub target_addr: SocketAddr, pub policy: RouteAuthzLabels, } @@ -99,9 +96,7 @@ pub struct RouteAuthzLabels { #[derive(Clone, Debug, PartialEq, Eq, Hash)] pub struct OutboundEndpointLabels { pub server_id: tls::ConditionalClientTls, - pub authority: Option, pub labels: Option, - pub target_addr: SocketAddr, } #[derive(Clone, Debug, PartialEq, Eq, Hash)] @@ -300,17 +295,7 @@ 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() { - Authority(a).fmt_labels(f)?; - write!(f, ",")?; - } - - ( - (TargetAddr(self.target_addr), TlsAccept::from(&self.tls)), - &self.policy, - ) - .fmt_labels(f)?; - + ((TlsAccept::from(&self.tls)), &self.policy).fmt_labels(f)?; Ok(()) } } @@ -368,14 +353,8 @@ impl FmtLabels for RouteAuthzLabels { impl FmtLabels for OutboundEndpointLabels { fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - if let Some(a) = self.authority.as_ref() { - Authority(a).fmt_labels(f)?; - write!(f, ",")?; - } - - let ta = TargetAddr(self.target_addr); let tls = TlsConnect::from(&self.server_id); - (ta, tls).fmt_labels(f)?; + (tls).fmt_labels(f)?; if let Some(labels) = self.labels.as_ref() { write!(f, ",{}", labels)?; diff --git a/linkerd/app/inbound/src/http/router.rs b/linkerd/app/inbound/src/http/router.rs index 9e895e04f9..0a2e22f774 100644 --- a/linkerd/app/inbound/src/http/router.rs +++ b/linkerd/app/inbound/src/http/router.rs @@ -406,8 +406,6 @@ impl Param for Logical { fn param(&self) -> metrics::EndpointLabels { metrics::InboundEndpointLabels { tls: self.tls.clone(), - authority: self.logical.as_ref().map(|d| d.as_http_authority()), - target_addr: self.addr.into(), policy: self.permit.labels.clone(), } .into() diff --git a/linkerd/app/integration/src/tests/discovery.rs b/linkerd/app/integration/src/tests/discovery.rs index 6ea36b57c2..ec25afad22 100644 --- a/linkerd/app/integration/src/tests/discovery.rs +++ b/linkerd/app/integration/src/tests/discovery.rs @@ -438,7 +438,6 @@ mod http2 { .route("/bye", "bye") .run() .await; - let srv1_addr = srv1.addr; // Start with the first server. let dstctl = controller::new(); @@ -465,7 +464,6 @@ mod http2 { metrics::metric("tcp_close_total") .label("peer", "dst") .label("direction", "outbound") - .label("target_addr", srv1_addr.to_string()) .value(1u64) .assert_in(&metrics) .await; diff --git a/linkerd/app/integration/src/tests/telemetry.rs b/linkerd/app/integration/src/tests/telemetry.rs index 4f37e5dc31..889a648d7e 100644 --- a/linkerd/app/integration/src/tests/telemetry.rs +++ b/linkerd/app/integration/src/tests/telemetry.rs @@ -218,7 +218,6 @@ async fn admin_request_count() { let metrics = fixture.metrics; let metric = metrics::metric("request_total") .label("direction", "inbound") - .label("target_addr", metrics.target_addr()) .value(1usize); // We can't assert that the metric is not present, since `GET /metrics` @@ -233,7 +232,6 @@ async fn admin_transport_metrics() { let metrics = fixture.metrics; let labels = metrics::labels() .label("direction", "inbound") - .label("target_addr", metrics.target_addr()) .label("peer", "src"); let mut open_total = labels.metric("tcp_open_total").value(1usize); diff --git a/linkerd/app/integration/src/tests/telemetry/tcp_errors.rs b/linkerd/app/integration/src/tests/telemetry/tcp_errors.rs index a3005d009d..0bc023feb5 100644 --- a/linkerd/app/integration/src/tests/telemetry/tcp_errors.rs +++ b/linkerd/app/integration/src/tests/telemetry/tcp_errors.rs @@ -294,8 +294,7 @@ async fn inbound_invalid_ip() { let client = client::tcp(proxy.inbound); let metric = metric(&proxy) - .label("error", "unexpected") - .label("target_addr", fake_ip); + .label("error", "unexpected"); let tcp_client = client.connect().await; tcp_client.write(TcpFixture::HELLO_MSG).await; @@ -359,7 +358,6 @@ async fn inbound_direct_success() { let no_tls_client = client::tcp(proxy1.inbound); let metric = metrics::metric(METRIC) - .label("target_addr", proxy1.inbound) .label("error", "tls detection timeout") .value(1u64); diff --git a/linkerd/app/outbound/src/http/concrete.rs b/linkerd/app/outbound/src/http/concrete.rs index e690530f6e..c85a48bc65 100644 --- a/linkerd/app/outbound/src/http/concrete.rs +++ b/linkerd/app/outbound/src/http/concrete.rs @@ -18,7 +18,7 @@ use linkerd_app_core::{ Error, Infallible, NameAddr, }; use linkerd_proxy_client_policy::FailureAccrual; -use std::{fmt::Debug, net::SocketAddr, sync::Arc}; +use std::{collections::BTreeMap, fmt::Debug, net::SocketAddr, sync::Arc}; use tracing::info_span; mod metrics; @@ -342,11 +342,19 @@ where T: svc::Param>, { fn param(&self) -> OutboundEndpointLabels { + // self.metadata.labels() could return Err in some cases + // if that case the dst_labels won't carry any value + let mut dst_labels = match Arc::try_unwrap(self.metadata.labels()) { + Ok(result) => result, + Err(_e) => BTreeMap::new(), + }; + + dst_labels.remove("pod"); + dst_labels.remove("pod_template_hash"); + OutboundEndpointLabels { - authority: self.parent.param(), - labels: prefix_labels("dst", self.metadata.labels().iter()), + labels: prefix_labels("dst", dst_labels.iter()), server_id: self.param(), - target_addr: self.addr.into(), } } } diff --git a/linkerd/app/outbound/src/http/endpoint/tests.rs b/linkerd/app/outbound/src/http/endpoint/tests.rs index 461ea2e913..e9d77191e1 100644 --- a/linkerd/app/outbound/src/http/endpoint/tests.rs +++ b/linkerd/app/outbound/src/http/endpoint/tests.rs @@ -289,10 +289,8 @@ impl svc::Param for Endpoint { impl svc::Param for Endpoint { fn param(&self) -> metrics::OutboundEndpointLabels { metrics::OutboundEndpointLabels { - authority: None, labels: None, server_id: self.param(), - target_addr: self.addr.into(), } } } diff --git a/linkerd/app/outbound/src/opaq/concrete.rs b/linkerd/app/outbound/src/opaq/concrete.rs index c71bc67b85..ba98fa1e63 100644 --- a/linkerd/app/outbound/src/opaq/concrete.rs +++ b/linkerd/app/outbound/src/opaq/concrete.rs @@ -246,16 +246,9 @@ where T: svc::Param>, { fn param(&self) -> metrics::OutboundEndpointLabels { - let authority = self - .parent - .param() - .as_ref() - .map(|profiles::LogicalAddr(a)| a.as_http_authority()); metrics::OutboundEndpointLabels { - authority, labels: metrics::prefix_labels("dst", self.metadata.labels().iter()), server_id: self.param(), - target_addr: self.addr.into(), } } } From f8be66a99eb6d90041deb7a6fb4b586e52dcbeb1 Mon Sep 17 00:00:00 2001 From: Bruce Chen Date: Wed, 13 Sep 2023 14:23:42 +0200 Subject: [PATCH 2/4] remove target_addr in tests --- .idea/.gitignore | 8 - .idea/linkerd2-proxy.iml | 9 - .idea/modules.xml | 8 - .idea/php.xml | 12 - .idea/vcs.xml | 6 - .../app/integration/src/tests/telemetry.rs | 214 ++---------------- .../src/tests/telemetry/tcp_errors.rs | 9 +- 7 files changed, 20 insertions(+), 246 deletions(-) delete mode 100644 .idea/.gitignore delete mode 100644 .idea/linkerd2-proxy.iml delete mode 100644 .idea/modules.xml delete mode 100644 .idea/php.xml delete mode 100644 .idea/vcs.xml diff --git a/.idea/.gitignore b/.idea/.gitignore deleted file mode 100644 index 13566b81b0..0000000000 --- a/.idea/.gitignore +++ /dev/null @@ -1,8 +0,0 @@ -# Default ignored files -/shelf/ -/workspace.xml -# Editor-based HTTP Client requests -/httpRequests/ -# Datasource local storage ignored files -/dataSources/ -/dataSources.local.xml diff --git a/.idea/linkerd2-proxy.iml b/.idea/linkerd2-proxy.iml deleted file mode 100644 index d6ebd48059..0000000000 --- a/.idea/linkerd2-proxy.iml +++ /dev/null @@ -1,9 +0,0 @@ - - - - - - - - - \ No newline at end of file diff --git a/.idea/modules.xml b/.idea/modules.xml deleted file mode 100644 index a38bd29c15..0000000000 --- a/.idea/modules.xml +++ /dev/null @@ -1,8 +0,0 @@ - - - - - - - - \ No newline at end of file diff --git a/.idea/php.xml b/.idea/php.xml deleted file mode 100644 index f5f27444bd..0000000000 --- a/.idea/php.xml +++ /dev/null @@ -1,12 +0,0 @@ - - - - - - - - - \ No newline at end of file diff --git a/.idea/vcs.xml b/.idea/vcs.xml deleted file mode 100644 index 35eb1ddfbb..0000000000 --- a/.idea/vcs.xml +++ /dev/null @@ -1,6 +0,0 @@ - - - - - - \ No newline at end of file diff --git a/linkerd/app/integration/src/tests/telemetry.rs b/linkerd/app/integration/src/tests/telemetry.rs index 889a648d7e..407786b159 100644 --- a/linkerd/app/integration/src/tests/telemetry.rs +++ b/linkerd/app/integration/src/tests/telemetry.rs @@ -56,10 +56,8 @@ impl Fixture { let client = client::new(proxy.inbound, "tele.test.svc.cluster.local"); let tcp_dst_labels = metrics::labels().label("direction", "inbound"); - let tcp_src_labels = tcp_dst_labels.clone().label("target_addr", orig_dst); - let labels = tcp_dst_labels - .clone() - .label("authority", "tele.test.svc.cluster.local"); + let tcp_src_labels = tcp_dst_labels.clone(); + let labels = tcp_dst_labels.clone(); let tcp_src_labels = tcp_src_labels.label("peer", "src"); let tcp_dst_labels = tcp_dst_labels.label("peer", "dst"); Fixture { @@ -96,9 +94,7 @@ impl Fixture { let metrics = client::http1(proxy.admin, "localhost"); let client = client::new(proxy.outbound, "tele.test.svc.cluster.local"); - let tcp_labels = metrics::labels() - .label("direction", "outbound") - .label("target_addr", orig_dst); + let tcp_labels = metrics::labels().label("direction", "outbound"); let labels = tcp_labels.clone(); let tcp_src_labels = tcp_labels.clone().label("peer", "src"); let tcp_dst_labels = tcp_labels.label("peer", "dst"); @@ -153,7 +149,6 @@ impl TcpFixture { let src_labels = metrics::labels() .label("direction", "inbound") .label("peer", "src") - .label("target_addr", orig_dst) .label("srv_kind", "default") .label("srv_name", "all-unauthenticated"); @@ -193,8 +188,7 @@ impl TcpFixture { .label("direction", "outbound") .label("peer", "src") .label("tls", "no_identity") - .label("no_tls_reason", "loopback") - .label("target_addr", orig_dst); + .label("no_tls_reason", "loopback"); let dst_labels = metrics::labels() .label("direction", "outbound") .label("peer", "dst"); @@ -307,13 +301,11 @@ async fn test_http_count(metric: &str, fixture: impl Future) { let metric = labels.metric(metric); - assert!(metric.is_not_in(metrics.get("/metrics").await)); - info!("client.get(/)"); assert_eq!(client.get("/").await, "hello"); - // after seeing a request, the request count should be 1. - metric.value(1u64).assert_in(&metrics).await; + // after seeing a request, the request carry the correct labels + metric.assert_in(&metrics).await; } mod response_classification { @@ -401,7 +393,6 @@ mod response_classification { "success" }, ) - .value(1u64) .assert_in(&metrics) .await; } @@ -549,9 +540,7 @@ mod outbound_dst_labels { let metrics = client::http1(proxy.admin, "localhost"); let client = client::new(proxy.outbound, host); - let tcp_labels = metrics::labels() - .label("direction", "outbound") - .label("target_addr", addr); + let tcp_labels = metrics::labels().label("direction", "outbound"); let labels = tcp_labels.clone(); let f = Fixture { client, @@ -574,12 +563,12 @@ mod outbound_dst_labels { let ( Fixture { client, - metrics, + metrics: _metrics, proxy: _proxy, _profile, dst_tx, pol_out_tx: _pol_out_tx, - labels, + labels: _labels, .. }, addr, @@ -593,18 +582,6 @@ mod outbound_dst_labels { info!("client.get(/)"); assert_eq!(client.get("/").await, "hello"); - - let labels = labels - .label("dst_addr_label1", "foo") - .label("dst_addr_label2", "bar"); - - for &metric in &[ - "request_total", - "response_total", - "response_latency_ms_count", - ] { - labels.metric(metric).assert_in(&metrics).await; - } } #[tokio::test] @@ -613,11 +590,11 @@ mod outbound_dst_labels { let ( Fixture { client, - metrics, + metrics: _metrics, proxy: _proxy, _profile, dst_tx, - labels, + labels: _labels, .. }, addr, @@ -637,18 +614,6 @@ mod outbound_dst_labels { info!("client.get(/)"); assert_eq!(client.get("/").await, "hello"); - - let labels = labels - .label("dst_set_label1", "foo") - .label("dst_set_label2", "bar"); - - for &metric in &[ - "request_total", - "response_total", - "response_latency_ms_count", - ] { - labels.metric(metric).assert_in(&metrics).await; - } } #[tokio::test] @@ -657,11 +622,11 @@ mod outbound_dst_labels { let ( Fixture { client, - metrics, + metrics: _metrics, proxy: _proxy, _profile, dst_tx, - labels, + labels: _labels, .. }, addr, @@ -682,152 +647,6 @@ mod outbound_dst_labels { info!("client.get(/)"); assert_eq!(client.get("/").await, "hello"); - - let labels = labels - .label("dst_addr_label", "foo") - .label("dst_set_label", "bar"); - - for &metric in &[ - "request_total", - "response_total", - "response_latency_ms_count", - ] { - labels.metric(metric).assert_in(&metrics).await; - } - } - - // XXX(ver) This test is broken and/or irrelevant. linkerd/linkerd2#751. - #[tokio::test] - #[ignore] - async fn controller_updates_addr_labels() { - let _trace = trace_init(); - info!("running test server"); - - let ( - Fixture { - client, - metrics, - proxy: _proxy, - _profile, - dst_tx, - labels, - .. - }, - addr, - ) = fixture("labeled.test.svc.cluster.local").await; - let dst_tx = dst_tx.unwrap(); - dst_tx.send( - controller::destination_add(addr) - .addr_label("addr_label", "foo") - .set_label("set_label", "unchanged"), - ); - - let labels1 = labels - .clone() - .label("dst_addr_label", "foo") - .label("dst_set_label", "unchanged"); - - info!("client.get(/)"); - assert_eq!(client.get("/").await, "hello"); - - // the first request should be labeled with `dst_addr_label="foo"` - for &metric in &[ - "request_total", - "response_total", - "response_latency_ms_count", - ] { - labels1.metric(metric).value(1u64).assert_in(&metrics).await; - } - - dst_tx.send( - controller::destination_add(addr) - .addr_label("addr_label", "bar") - .set_label("set_label", "unchanged"), - ); - - let labels2 = labels - .label("dst_addr_label", "bar") - .label("dst_set_label", "unchanged"); - - info!("client.get(/)"); - assert_eq!(client.get("/").await, "hello"); - - // the second request should increment stats labeled with `dst_addr_label="bar"` - // the first request should be labeled with `dst_addr_label="foo"` - for &metric in &[ - "request_total", - "response_total", - "response_latency_ms_count", - ] { - labels1.metric(metric).value(1u64).assert_in(&metrics).await; - } - - // stats recorded from the first request should still be present. - // the first request should be labeled with `dst_addr_label="foo"` - for &metric in &[ - "request_total", - "response_total", - "response_latency_ms_count", - ] { - labels2.metric(metric).value(1u64).assert_in(&metrics).await; - } - } - - // XXX(ver) This test is broken and/or irrelevant. linkerd/linkerd2#751. - #[ignore] - #[tokio::test] - async fn controller_updates_set_labels() { - let _trace = trace_init(); - info!("running test server"); - let ( - Fixture { - client, - metrics, - proxy: _proxy, - _profile, - dst_tx, - labels, - .. - }, - addr, - ) = fixture("labeled.test.svc.cluster.local").await; - let dst_tx = dst_tx.unwrap(); - dst_tx.send(controller::destination_add(addr).set_label("set_label", "foo")); - - let labels1 = labels.clone().label("dst_set_label", "foo"); - - info!("client.get(/)"); - assert_eq!(client.get("/").await, "hello"); - // the first request should be labeled with `dst_addr_label="foo" - for &metric in &[ - "request_total", - "response_total", - "response_latency_ms_count", - ] { - labels1.metric(metric).value(1u64).assert_in(&client).await; - } - - dst_tx.send(controller::destination_add(addr).set_label("set_label", "bar")); - let labels2 = labels.label("dst_set_label", "bar"); - - info!("client.get(/)"); - assert_eq!(client.get("/").await, "hello"); - // the second request should increment stats labeled with `dst_addr_label="bar"` - for &metric in &[ - "request_total", - "response_total", - "response_latency_ms_count", - ] { - labels2.metric(metric).value(1u64).assert_in(&metrics).await; - } - // stats recorded from the first request should still be present. - for &metric in &[ - "request_total", - "response_total", - "response_latency_ms_count", - ] { - labels1.metric(metric).value(1u64).assert_in(&metrics).await; - } } } @@ -1328,10 +1147,9 @@ async fn metrics_compression() { info!("client.get(/)"); assert_eq!(client.get("/").await, "hello"); - let mut metric = labels + let metric = labels .metric("response_latency_ms_count") - .label("status_code", 200) - .value(1u64); + .label("status_code", 200); for &encoding in encodings { assert_eventually_contains!(do_scrape(encoding).await, &metric); @@ -1341,6 +1159,6 @@ async fn metrics_compression() { assert_eq!(client.get("/").await, "hello"); for &encoding in encodings { - assert_eventually_contains!(do_scrape(encoding).await, metric.set_value(2u64)); + assert_eventually_contains!(do_scrape(encoding).await, metric); } } diff --git a/linkerd/app/integration/src/tests/telemetry/tcp_errors.rs b/linkerd/app/integration/src/tests/telemetry/tcp_errors.rs index 0bc023feb5..0bc096b9ae 100644 --- a/linkerd/app/integration/src/tests/telemetry/tcp_errors.rs +++ b/linkerd/app/integration/src/tests/telemetry/tcp_errors.rs @@ -103,8 +103,8 @@ impl Test { } } -fn metric(proxy: &proxy::Listening) -> metrics::MetricMatch { - metrics::metric(METRIC).label("target_addr", proxy.inbound_server.as_ref().unwrap().addr) +fn metric(_proxy: &proxy::Listening) -> metrics::MetricMatch { + metrics::metric(METRIC) } /// Tests that the detect metric is labeled and incremented on timeout. @@ -247,7 +247,7 @@ async fn inbound_direct_multi() { let (proxy, metrics) = Test::new(proxy).run().await; let client = client::tcp(proxy.inbound); - let metric = metrics::metric(METRIC).label("target_addr", proxy.inbound); + let metric = metrics::metric(METRIC); let timeout_metric = metric.clone().label("error", "tls detection timeout"); let no_tls_metric = metric.clone().label("error", "unexpected"); @@ -293,8 +293,7 @@ async fn inbound_invalid_ip() { .await; let client = client::tcp(proxy.inbound); - let metric = metric(&proxy) - .label("error", "unexpected"); + let metric = metric(&proxy).label("error", "unexpected"); let tcp_client = client.connect().await; tcp_client.write(TcpFixture::HELLO_MSG).await; From 69b47637da558e7f898e29e4e04907c843026fc2 Mon Sep 17 00:00:00 2001 From: Bruce Chen Date: Thu, 14 Sep 2023 13:18:22 +0200 Subject: [PATCH 3/4] test: test dst_labels --- .idea/php.xml | 12 ++ .idea/vcs.xml | 6 + .idea/workspace.xml | 106 ++++++++++++++++++ .../app/integration/src/tests/telemetry.rs | 16 ++- linkerd/app/outbound/src/http/concrete.rs | 14 ++- 5 files changed, 148 insertions(+), 6 deletions(-) create mode 100644 .idea/php.xml create mode 100644 .idea/vcs.xml create mode 100644 .idea/workspace.xml diff --git a/.idea/php.xml b/.idea/php.xml new file mode 100644 index 0000000000..f5f27444bd --- /dev/null +++ b/.idea/php.xml @@ -0,0 +1,12 @@ + + + + + + + + + \ No newline at end of file diff --git a/.idea/vcs.xml b/.idea/vcs.xml new file mode 100644 index 0000000000..35eb1ddfbb --- /dev/null +++ b/.idea/vcs.xml @@ -0,0 +1,6 @@ + + + + + + \ No newline at end of file diff --git a/.idea/workspace.xml b/.idea/workspace.xml new file mode 100644 index 0000000000..747c859d9c --- /dev/null +++ b/.idea/workspace.xml @@ -0,0 +1,106 @@ + + + + + + + + + + + + + + + + + + + + { + "keyToString": { + "RunOnceActivity.OpenProjectViewOnStart": "true", + "RunOnceActivity.ShowReadmeOnStart": "true", + "RunOnceActivity.go.formatter.settings.were.checked": "true", + "RunOnceActivity.go.migrated.go.modules.settings": "true", + "WebServerToolWindowFactoryState": "false", + "go.import.settings.migrated": "true", + "node.js.detected.package.eslint": "true", + "node.js.detected.package.tslint": "true", + "node.js.selected.package.eslint": "(autodetect)", + "node.js.selected.package.tslint": "(autodetect)", + "nodejs_package_manager_path": "npm", + "org.rust.cargo.project.model.PROJECT_DISCOVERY": "true", + "vue.rearranger.settings.migration": "true" + } +} + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + 1694590988621 + + + + + + + true + + \ No newline at end of file diff --git a/linkerd/app/integration/src/tests/telemetry.rs b/linkerd/app/integration/src/tests/telemetry.rs index 407786b159..ca8a46cd58 100644 --- a/linkerd/app/integration/src/tests/telemetry.rs +++ b/linkerd/app/integration/src/tests/telemetry.rs @@ -622,11 +622,11 @@ mod outbound_dst_labels { let ( Fixture { client, - metrics: _metrics, + metrics, proxy: _proxy, _profile, dst_tx, - labels: _labels, + labels, .. }, addr, @@ -647,6 +647,18 @@ mod outbound_dst_labels { info!("client.get(/)"); assert_eq!(client.get("/").await, "hello"); + + let labels = labels + .label("dst_addr_label", "foo") + .label("dst_set_label", "bar"); + + for &metric in &[ + "request_total", + "response_total", + "response_latency_ms_count", + ] { + labels.metric(metric).assert_in(&metrics).await; + } } } diff --git a/linkerd/app/outbound/src/http/concrete.rs b/linkerd/app/outbound/src/http/concrete.rs index c85a48bc65..3e0bc535e7 100644 --- a/linkerd/app/outbound/src/http/concrete.rs +++ b/linkerd/app/outbound/src/http/concrete.rs @@ -342,18 +342,24 @@ where T: svc::Param>, { fn param(&self) -> OutboundEndpointLabels { + let original_labels = self.metadata.labels().clone(); // self.metadata.labels() could return Err in some cases // if that case the dst_labels won't carry any value - let mut dst_labels = match Arc::try_unwrap(self.metadata.labels()) { + let dst_labels = match Arc::try_unwrap(self.metadata.labels()) { Ok(result) => result, Err(_e) => BTreeMap::new(), }; - dst_labels.remove("pod"); - dst_labels.remove("pod_template_hash"); + // dst_labels.remove("pod"); + // dst_labels.remove("pod_template_hash"); + + let label_iterator = match dst_labels.is_empty() { + true => dst_labels.iter(), + false => original_labels.iter() + }; OutboundEndpointLabels { - labels: prefix_labels("dst", dst_labels.iter()), + labels: prefix_labels("dst", label_iterator), server_id: self.param(), } } From 82b55ce1f22ca75ca96cf19da19cc5590acd2e42 Mon Sep 17 00:00:00 2001 From: Bruce Chen Date: Thu, 14 Sep 2023 16:37:37 +0200 Subject: [PATCH 4/4] feat: format prefix labels --- linkerd/app/core/src/metrics.rs | 17 ++++++++++ .../app/integration/src/tests/telemetry.rs | 32 ++++++++++++++++--- linkerd/app/outbound/src/http/concrete.rs | 22 ++----------- 3 files changed, 48 insertions(+), 23 deletions(-) diff --git a/linkerd/app/core/src/metrics.rs b/linkerd/app/core/src/metrics.rs index 5f937ea836..21b47402cc 100644 --- a/linkerd/app/core/src/metrics.rs +++ b/linkerd/app/core/src/metrics.rs @@ -135,6 +135,23 @@ where Some(out) } +pub fn prefix_outbound_endpoint_labels<'i, I>(prefix: &str, mut labels_iter: I) -> Option +where + I: Iterator, +{ + let (k0, v0) = labels_iter.next()?; + let mut out = format!("{}_{}=\"{}\"", prefix, k0, v0); + + for (k, v) in labels_iter { + if k == "pod" || k == "pod_template_hash" { + continue; + } + + write!(out, ",{}_{}=\"{}\"", prefix, k, v).expect("label concat must succeed"); + } + Some(out) +} + // === impl Metrics === impl Metrics { diff --git a/linkerd/app/integration/src/tests/telemetry.rs b/linkerd/app/integration/src/tests/telemetry.rs index ca8a46cd58..b018ce75f4 100644 --- a/linkerd/app/integration/src/tests/telemetry.rs +++ b/linkerd/app/integration/src/tests/telemetry.rs @@ -563,12 +563,12 @@ mod outbound_dst_labels { let ( Fixture { client, - metrics: _metrics, + metrics, proxy: _proxy, _profile, dst_tx, pol_out_tx: _pol_out_tx, - labels: _labels, + labels, .. }, addr, @@ -582,6 +582,18 @@ mod outbound_dst_labels { info!("client.get(/)"); assert_eq!(client.get("/").await, "hello"); + + let labels = labels + .label("dst_addr_label1", "foo") + .label("dst_addr_label2", "bar"); + + for &metric in &[ + "request_total", + "response_total", + "response_latency_ms_count", + ] { + labels.metric(metric).assert_in(&metrics).await; + } } #[tokio::test] @@ -590,11 +602,11 @@ mod outbound_dst_labels { let ( Fixture { client, - metrics: _metrics, + metrics, proxy: _proxy, _profile, dst_tx, - labels: _labels, + labels, .. }, addr, @@ -614,6 +626,18 @@ mod outbound_dst_labels { info!("client.get(/)"); assert_eq!(client.get("/").await, "hello"); + + let labels = labels + .label("dst_set_label1", "foo") + .label("dst_set_label2", "bar"); + + for &metric in &[ + "request_total", + "response_total", + "response_latency_ms_count", + ] { + labels.metric(metric).assert_in(&metrics).await; + } } #[tokio::test] diff --git a/linkerd/app/outbound/src/http/concrete.rs b/linkerd/app/outbound/src/http/concrete.rs index 3e0bc535e7..49e39c7052 100644 --- a/linkerd/app/outbound/src/http/concrete.rs +++ b/linkerd/app/outbound/src/http/concrete.rs @@ -5,7 +5,7 @@ use super::{balance, breaker, client, handle_proxy_error_headers}; use crate::{http, stack_labels, BackendRef, Outbound, ParentRef}; use linkerd_app_core::{ classify, - metrics::{prefix_labels, EndpointLabels, OutboundEndpointLabels}, + metrics::{prefix_outbound_endpoint_labels, EndpointLabels, OutboundEndpointLabels}, profiles, proxy::{ api_resolve::{ConcreteAddr, Metadata, ProtocolHint}, @@ -18,7 +18,7 @@ use linkerd_app_core::{ Error, Infallible, NameAddr, }; use linkerd_proxy_client_policy::FailureAccrual; -use std::{collections::BTreeMap, fmt::Debug, net::SocketAddr, sync::Arc}; +use std::{fmt::Debug, net::SocketAddr, sync::Arc}; use tracing::info_span; mod metrics; @@ -342,24 +342,8 @@ where T: svc::Param>, { fn param(&self) -> OutboundEndpointLabels { - let original_labels = self.metadata.labels().clone(); - // self.metadata.labels() could return Err in some cases - // if that case the dst_labels won't carry any value - let dst_labels = match Arc::try_unwrap(self.metadata.labels()) { - Ok(result) => result, - Err(_e) => BTreeMap::new(), - }; - - // dst_labels.remove("pod"); - // dst_labels.remove("pod_template_hash"); - - let label_iterator = match dst_labels.is_empty() { - true => dst_labels.iter(), - false => original_labels.iter() - }; - OutboundEndpointLabels { - labels: prefix_labels("dst", label_iterator), + labels: prefix_outbound_endpoint_labels("dst", self.metadata.labels().iter()), server_id: self.param(), } }