access logs: consistently quote fields (istio#1095)

* access logs: consistently quote fields Before we were semi-randomly quoting or not quoting fields. Now we always quote strings. * to_value
MorrisLaw · Jun 3, 2024 · 718f6f0 · 718f6f0
1 parent 8d366c4
commit 718f6f0
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 50 deletions.
diff --git a/src/admin.rs b/src/admin.rs
@@ -72,7 +72,7 @@ pub struct Service {
     s: Server<State>,
 }
 
-#[derive(serde::Serialize, Debug, Clone)]
+#[derive(serde::Serialize, Clone)]
 #[serde(rename_all = "camelCase")]
 pub struct ConfigDump {
     #[serde(flatten)]

diff --git a/src/metrics.rs b/src/metrics.rs
@@ -19,6 +19,7 @@ use prometheus_client::encoding::{EncodeLabelValue, LabelValueEncoder};
 use prometheus_client::registry::Registry;
 use tracing::error;
 use tracing::field::{display, DisplayValue};
+use tracing_core::field::Value;
 
 use crate::identity::Identity;
 
@@ -108,6 +109,9 @@ impl DefaultedUnknown<RichStrng> {
     pub fn display(&self) -> Option<DisplayValue<&str>> {
         self.as_ref().map(|rs| display(rs.as_str()))
     }
+    pub fn to_value(&self) -> Option<impl Value + '_> {
+        self.as_ref().map(|rs| rs.as_str())
+    }
 }
 
 impl<T> DefaultedUnknown<T> {

diff --git a/src/proxy/metrics.rs b/src/proxy/metrics.rs
@@ -24,15 +24,15 @@ use prometheus_client::metrics::family::Family;
 use prometheus_client::registry::Registry;
 
 use tracing::event;
+use tracing_core::field::Value;
 
 use crate::identity::Identity;
-use crate::metrics::{DefaultedUnknown, DeferRecorder, Deferred, IncrementRecorder};
+use crate::metrics::DefaultedUnknown;
 
 use crate::state::service::ServiceDescription;
 use crate::state::workload::Workload;
 use crate::strng::{RichStrng, Strng};
 
-#[derive(Debug)]
 pub struct Metrics {
     pub connection_opens: Family<CommonTrafficLabels, Counter>,
     pub connection_close: Family<CommonTrafficLabels, Counter>,
@@ -43,38 +43,6 @@ pub struct Metrics {
     pub on_demand_dns: Family<OnDemandDnsLabels, Counter>,
 }
 
-impl Metrics {
-    #[must_use = "metric will be dropped (and thus recorded) immediately if not assigned"]
-    /// increment_defer is used to increment a metric now and another metric later once the MetricGuard is dropped
-    ///
-    /// # Examples
-    ///
-    /// ```ignore
-    /// let connection_open = ConnectionOpen {};
-    /// // Record connection opened now
-    /// let connection_close = self.metrics.increment_defer::<_, ConnectionClosed>(&connection_open);
-    /// // Eventually, report connection closed
-    /// drop(connection_close);
-    /// ```
-    pub fn increment_defer<'a, M1, M2>(
-        &'a self,
-        event: &'a M1,
-    ) -> Deferred<'a, impl FnOnce(&'a Self), Self>
-    where
-        M1: Clone + 'a,
-        M2: From<&'a M1> + 'a,
-        Metrics: IncrementRecorder<M1> + IncrementRecorder<M2>,
-    {
-        self.increment(event);
-        let m2: M2 = event.into();
-        self.defer_record(move |metrics| {
-            metrics.increment(&m2);
-        })
-    }
-}
-
-impl DeferRecorder for Metrics {}
-
 #[derive(Clone, Copy, Default, Debug, Hash, PartialEq, Eq, EncodeLabelValue)]
 pub enum Reporter {
     #[default]
@@ -431,16 +399,16 @@ impl ConnectionResult {
             tracing::Level::DEBUG,
 
             src.addr = %src.0,
-            src.workload = src.1.as_deref().map(display),
-            src.namespace = tl.source_workload_namespace.display(),
-            src.identity = tl.source_principal.as_ref().filter(|_| mtls).map(|id| id.to_string()),
+            src.workload = src.1.as_deref().map(to_value),
+            src.namespace = tl.source_workload_namespace.to_value(),
+            src.identity = tl.source_principal.as_ref().filter(|_| mtls).map(to_value_owned),
 
             dst.addr = %dst.0,
             dst.hbone_addr = hbone_target.map(display),
-            dst.service = tl.destination_service.display(),
-            dst.workload = dst.1.as_deref().map(display),
-            dst.namespace = tl.destination_workload_namespace.display(),
-            dst.identity = tl.destination_principal.as_ref().filter(|_| mtls).map(|id| id.to_string()),
+            dst.service = tl.destination_service.to_value(),
+            dst.workload = dst.1.as_deref().map(to_value),
+            dst.namespace = tl.destination_workload_namespace.to_value(),
+            dst.identity = tl.destination_principal.as_ref().filter(|_| mtls).map(to_value_owned),
 
             direction = if tl.reporter == Reporter::source {
                 "outbound"
@@ -514,16 +482,16 @@ impl ConnectionResult {
             res,
 
             src.addr = %self.src.0,
-            src.workload = self.src.1.as_deref().map(display),
-            src.namespace = tl.source_workload_namespace.display(),
-            src.identity = tl.source_principal.as_ref().filter(|_| mtls).map(|id| id.to_string()),
+            src.workload = self.src.1.as_deref().map(to_value),
+            src.namespace = tl.source_workload_namespace.to_value(),
+            src.identity = tl.source_principal.as_ref().filter(|_| mtls).map(to_value_owned),
 
             dst.addr = %self.dst.0,
             dst.hbone_addr = self.hbone_target.map(display),
-            dst.service = tl.destination_service.display(),
-            dst.workload = self.dst.1.as_deref().map(display),
-            dst.namespace = tl.destination_workload_namespace.display(),
-            dst.identity = tl.destination_principal.as_ref().filter(|_| mtls).map(|id| id.to_string()),
+            dst.service = tl.destination_service.to_value(),
+            dst.workload = self.dst.1.as_deref().map(to_value),
+            dst.namespace = tl.destination_workload_namespace.to_value(),
+            dst.identity = tl.destination_principal.as_ref().filter(|_| mtls).map(to_value_owned),
 
             direction = if tl.reporter == Reporter::source {
                 "outbound"
@@ -539,3 +507,12 @@ impl ConnectionResult {
         );
     }
 }
+
+fn to_value_owned<T: ToString>(t: T) -> impl Value {
+    t.to_string()
+}
+
+fn to_value<T: AsRef<str>>(t: &T) -> impl Value + '_ {
+    let v: &str = t.as_ref();
+    v
+}
diff --git a/src/state.rs b/src/state.rs
@@ -355,7 +355,7 @@ impl ProxyState {
 
 /// Wrapper around [ProxyState] that provides additional methods for requesting information
 /// on-demand.
-#[derive(serde::Serialize, Debug, Clone)]
+#[derive(serde::Serialize, Clone)]
 pub struct DemandProxyState {
     #[serde(flatten)]
     state: Arc<RwLock<ProxyState>>,