The metrics layer was soercing all metrics attributes to string. This…

… is now fixed.

In addition, logic that would have printed to stderr has been removed, and instead metrics values and attributes that are the wrong type are now silently ignored.

Fixes #3687

apollo-router/src/configuration/metrics.rs

@@ -5,6 +5,7 @@ use std::time::Duration;
 
 use jsonpath_rust::JsonPathInst;
 use paste::paste;
+use serde::Serialize;
 use serde_json::Value;
 use tokio::sync::OwnedSemaphorePermit;
 
@@ -16,7 +17,39 @@ pub(crate) struct MetricsHandle {
 
 pub(crate) struct Metrics {
     yaml: Value,
-    metrics: HashMap<String, (u64, HashMap<String, String>)>,
+    metrics: HashMap<String, (u64, HashMap<String, AttributeValue>)>,
+}
+
+enum AttributeValue {
+    Bool(bool),
+    U64(u64),
+    I64(i64),
+    F64(f64),
+    String(String),
+}
+
+impl Serialize for AttributeValue {
+    fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
+        match self {
+            AttributeValue::Bool(value) => serializer.serialize_bool(*value),
+            AttributeValue::U64(value) => serializer.serialize_u64(*value),
+            AttributeValue::I64(value) => serializer.serialize_i64(*value),
+            AttributeValue::F64(value) => serializer.serialize_f64(*value),
+            AttributeValue::String(value) => serializer.serialize_str(value),
+        }
+    }
+}
+
+impl AttributeValue {
+    fn dyn_value(self: &AttributeValue) -> &dyn tracing::Value {
+        match self {
+            AttributeValue::Bool(value) => value as &dyn tracing::Value,
+            AttributeValue::U64(value) => value as &dyn tracing::Value,
+            AttributeValue::I64(value) => value as &dyn tracing::Value,
+            AttributeValue::F64(value) => value as &dyn tracing::Value,
+            AttributeValue::String(value) => value as &dyn tracing::Value,
+        }
+    }
 }
 
 impl Metrics {
@@ -98,12 +131,19 @@ impl Metrics {
                             let attr_name = stringify!([<$($attr __ )+>]).to_string();
                             match JsonPathInst::from_str($attr_path).expect("json path must be valid").find_slice(value).into_iter().next().as_deref() {
                                 // If the value is an object we can only state that it is set, but not what it is set to.
-                                Some(Value::Object(_value)) => {attributes.insert(attr_name, "true".to_string());},
-                                Some(Value::Array(value)) if !value.is_empty() => {attributes.insert(attr_name, "true".to_string());},
+                                Some(Value::Object(_value)) => {attributes.insert(attr_name, AttributeValue::Bool(true));},
+                                Some(Value::Array(value)) if !value.is_empty() => {attributes.insert(attr_name, AttributeValue::Bool(true));},
                                 // Scalars can be logged as is.
-                                Some(value) => {attributes.insert(attr_name, value.to_string());},
+                                Some(Value::Number(value)) if value.is_f64() => {attributes.insert(attr_name, AttributeValue::F64(value.as_f64().expect("checked, qed")));},
+                                Some(Value::Number(value)) if value.is_i64() => {attributes.insert(attr_name, AttributeValue::I64(value.as_i64().expect("checked, qed")));},
+                                Some(Value::Number(value)) => {attributes.insert(attr_name, AttributeValue::U64(value.as_u64().expect("checked, qed")));},
+                                Some(Value::String(value)) => {attributes.insert(attr_name, AttributeValue::String(value.clone()));},
+                                Some(Value::Bool(value)) => {attributes.insert(attr_name, AttributeValue::Bool(*value));},
+
                                 // If the value is not set we don't specify the attribute.
-                                None => {attributes.insert(attr_name, "false".to_string());},
+                                None => {attributes.insert(attr_name, AttributeValue::Bool(false));},
+
+                                _ => {},
                             };)+
                             (1, attributes)
                         }
@@ -113,7 +153,7 @@ impl Metrics {
                             let mut attributes = HashMap::new();
                             $(
                                 let attr_name = stringify!([<$($attr __ )+>]).to_string();
-                                attributes.insert(attr_name, "false".to_string());
+                                attributes.insert(attr_name, AttributeValue::Bool(false));
                             )+
                             (0, attributes)
                         }
@@ -122,7 +162,7 @@ impl Metrics {
 
                 // Now log the metric
                 paste!{
-                    tracing::info!($($metric).+ = metric.0, $($($attr).+ = metric.1.get(stringify!([<$($attr __ )+>])).expect("attribute must be in map")),+);
+                    tracing::info!($($metric).+ = metric.0, $($($attr).+ = metric.1.get(stringify!([<$($attr __ )+>])).expect("attribute must be in map").dyn_value()),+);
                 }
             };
         }

apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@apq.router.yaml.snap

@@ -4,7 +4,7 @@ expression: "&metrics.metrics"
 ---
 value.apollo.router.config.apq:
   - 1
-  - opt__router__cache__in_memory__: "true"
-    opt__router__cache__redis__: "true"
-    opt__subgraph__: "true"
+  - opt__router__cache__in_memory__: true
+    opt__router__cache__redis__: true
+    opt__subgraph__: true
 

apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@authorization.router.yaml.snap

@@ -4,6 +4,6 @@ expression: "&metrics.metrics"
 ---
 value.apollo.router.config.authorization:
   - 1
-  - opt__directives__: "false"
-    opt__require_authentication__: "true"
+  - opt__directives__: false
+    opt__require_authentication__: true
 

apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@authorization_directives.router.yaml.snap

@@ -4,6 +4,6 @@ expression: "&metrics.metrics"
 ---
 value.apollo.router.config.authorization:
   - 1
-  - opt__directives__: "true"
-    opt__require_authentication__: "false"
+  - opt__directives__: true
+    opt__require_authentication__: false
 

apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@coprocessor.router.yaml.snap

@@ -4,10 +4,10 @@ expression: "&metrics.metrics"
 ---
 value.apollo.router.config.coprocessor:
   - 1
-  - opt__router__request__: "true"
-    opt__router__response__: "true"
-    opt__subgraph__request__: "true"
-    opt__subgraph__response__: "true"
-    opt__supergraph__request__: "false"
-    opt__supergraph__response__: "false"
+  - opt__router__request__: true
+    opt__router__response__: true
+    opt__subgraph__request__: true
+    opt__subgraph__response__: true
+    opt__supergraph__request__: false
+    opt__supergraph__response__: false
 

apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@entities.router.yaml.snap

@@ -4,15 +4,15 @@ expression: "&metrics.metrics"
 ---
 value.apollo.router.config.entities:
   - 1
-  - opt__cache__: "true"
+  - opt__cache__: true
 value.apollo.router.config.traffic_shaping:
   - 1
-  - opt__router__rate_limit__: "false"
-    opt__router__timout__: "false"
-    opt__subgraph__compression__: "false"
-    opt__subgraph__deduplicate_query__: "false"
-    opt__subgraph__http2__: "false"
-    opt__subgraph__rate_limit__: "false"
-    opt__subgraph__retry__: "false"
-    opt__subgraph__timeout__: "false"
+  - opt__router__rate_limit__: false
+    opt__router__timout__: false
+    opt__subgraph__compression__: false
+    opt__subgraph__deduplicate_query__: false
+    opt__subgraph__http2__: false
+    opt__subgraph__rate_limit__: false
+    opt__subgraph__retry__: false
+    opt__subgraph__timeout__: false
 

apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@limits.router.yaml.snap

@@ -4,12 +4,12 @@ expression: "&metrics.metrics"
 ---
 value.apollo.router.config.limits:
   - 1
-  - opt__operation__max_aliases__: "true"
-    opt__operation__max_depth__: "true"
-    opt__operation__max_height__: "true"
-    opt__operation__max_root_fields__: "true"
-    opt__operation__warn_only__: "true"
-    opt__parser__max_recursion__: "true"
-    opt__parser__max_tokens__: "true"
-    opt__request__max_size__: "true"
+  - opt__operation__max_aliases__: true
+    opt__operation__max_depth__: true
+    opt__operation__max_height__: true
+    opt__operation__max_root_fields__: true
+    opt__operation__warn_only__: true
+    opt__parser__max_recursion__: true
+    opt__parser__max_tokens__: true
+    opt__request__max_size__: true
 

apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@persisted_queries.router.yaml.snap

@@ -4,7 +4,7 @@ expression: "&metrics.metrics"
 ---
 value.apollo.router.config.persisted_queries:
   - 1
-  - opt__log_unknown__: "true"
-    opt__safelist__enabled__: "true"
-    opt__safelist__require_id__: "true"
+  - opt__log_unknown__: true
+    opt__safelist__enabled__: true
+    opt__safelist__require_id__: true
 

apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@subscriptions.router.yaml.snap

@@ -4,9 +4,9 @@ expression: "&metrics.metrics"
 ---
 value.apollo.router.config.subscriptions:
   - 1
-  - opt__deduplication__: "false"
-    opt__max_opened__: "true"
-    opt__mode__callback__: "true"
-    opt__mode__passthrough__: "true"
-    opt__queue_capacity__: "true"
+  - opt__deduplication__: false
+    opt__max_opened__: true
+    opt__mode__callback__: true
+    opt__mode__passthrough__: true
+    opt__queue_capacity__: true
 

apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@telemetry.router.yaml.snap

@@ -4,10 +4,10 @@ expression: "&metrics.metrics"
 ---
 value.apollo.router.config.telemetry:
   - 1
-  - opt__metrics__otlp__: "true"
-    opt__metrics__prometheus__: "true"
-    opt__tracing__datadog__: "true"
-    opt__tracing__jaeger__: "true"
-    opt__tracing__otlp__: "true"
-    opt__tracing__zipkin__: "true"
+  - opt__metrics__otlp__: true
+    opt__metrics__prometheus__: true
+    opt__tracing__datadog__: true
+    opt__tracing__jaeger__: true
+    opt__tracing__otlp__: true
+    opt__tracing__zipkin__: true
 

apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@traffic_shaping.router.yaml.snap

@@ -4,12 +4,12 @@ expression: "&metrics.metrics"
 ---
 value.apollo.router.config.traffic_shaping:
   - 1
-  - opt__router__rate_limit__: "true"
-    opt__router__timout__: "true"
-    opt__subgraph__compression__: "true"
-    opt__subgraph__deduplicate_query__: "true"
-    opt__subgraph__http2__: "true"
-    opt__subgraph__rate_limit__: "true"
-    opt__subgraph__retry__: "true"
-    opt__subgraph__timeout__: "true"
+  - opt__router__rate_limit__: true
+    opt__router__timout__: true
+    opt__subgraph__compression__: true
+    opt__subgraph__deduplicate_query__: true
+    opt__subgraph__http2__: true
+    opt__subgraph__rate_limit__: true
+    opt__subgraph__retry__: true
+    opt__subgraph__timeout__: true
 

apollo-router/src/plugins/telemetry/metrics/layer.rs

@@ -24,8 +24,6 @@ use super::METRIC_PREFIX_HISTOGRAM;
 use super::METRIC_PREFIX_MONOTONIC_COUNTER;
 use super::METRIC_PREFIX_VALUE;
 
-const I64_MAX: u64 = i64::MAX as u64;
-
 #[derive(Default)]
 pub(crate) struct Instruments {
     u64_counter: MetricsMap<Counter<u64>>,
@@ -162,66 +160,75 @@ pub(crate) struct MetricVisitor<'a> {
 }
 
 impl<'a> Visit for MetricVisitor<'a> {
-    fn record_debug(&mut self, field: &Field, value: &dyn fmt::Debug) {
-        // Do not display the log content
-        if field.name() != "message" {
+    fn record_f64(&mut self, field: &Field, value: f64) {
+        if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_MONOTONIC_COUNTER) {
+            self.metric = Some((metric_name, InstrumentType::CounterF64(value)));
+        } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_COUNTER) {
+            self.metric = Some((metric_name, InstrumentType::UpDownCounterF64(value)));
+        } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_HISTOGRAM) {
+            self.metric = Some((metric_name, InstrumentType::HistogramF64(value)));
+        } else {
             self.custom_attributes.push(KeyValue::new(
                 Key::from_static_str(field.name()),
-                Value::from(format!("{value:?}")),
+                Value::from(value),
             ));
         }
     }
 
-    fn record_str(&mut self, field: &Field, value: &str) {
-        self.custom_attributes.push(KeyValue::new(
-            Key::from_static_str(field.name()),
-            Value::from(value.to_string()),
-        ));
+    fn record_i64(&mut self, field: &Field, value: i64) {
+        if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_MONOTONIC_COUNTER) {
+            tracing::error!(metric_name, "monotonic counter must be u64");
+        } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_COUNTER) {
+            self.metric = Some((metric_name, InstrumentType::UpDownCounterI64(value)));
+        } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_HISTOGRAM) {
+            self.metric = Some((metric_name, InstrumentType::HistogramI64(value)));
+        } else {
+            self.custom_attributes.push(KeyValue::new(
+                Key::from_static_str(field.name()),
+                Value::from(value),
+            ));
+        }
     }
 
     fn record_u64(&mut self, field: &Field, value: u64) {
         if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_MONOTONIC_COUNTER) {
             self.metric = Some((metric_name, InstrumentType::CounterU64(value)));
-        } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_COUNTER) {
-            if value <= I64_MAX {
-                self.metric = Some((metric_name, InstrumentType::UpDownCounterI64(value as i64)));
-            } else {
-                eprintln!(
-                    "[tracing-opentelemetry]: Received Counter metric, but \
-                    provided u64: {value} is greater than i64::MAX. Ignoring \
-                    this metric."
-                );
-            }
         } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_HISTOGRAM) {
             self.metric = Some((metric_name, InstrumentType::HistogramU64(value)));
         } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_VALUE) {
             self.metric = Some((metric_name, InstrumentType::GaugeU64(value)));
-        } else {
-            self.record_debug(field, &value);
         }
     }
 
-    fn record_f64(&mut self, field: &Field, value: f64) {
-        if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_MONOTONIC_COUNTER) {
-            self.metric = Some((metric_name, InstrumentType::CounterF64(value)));
-        } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_COUNTER) {
-            self.metric = Some((metric_name, InstrumentType::UpDownCounterF64(value)));
-        } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_HISTOGRAM) {
-            self.metric = Some((metric_name, InstrumentType::HistogramF64(value)));
-        } else {
-            self.record_debug(field, &value);
-        }
+    fn record_i128(&mut self, field: &Field, _value: i128) {
+        tracing::error!(name = field.name(), "metric attribute cannot be i128");
     }
 
-    fn record_i64(&mut self, field: &Field, value: i64) {
-        if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_MONOTONIC_COUNTER) {
-            self.metric = Some((metric_name, InstrumentType::CounterU64(value as u64)));
-        } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_COUNTER) {
-            self.metric = Some((metric_name, InstrumentType::UpDownCounterI64(value)));
-        } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_HISTOGRAM) {
-            self.metric = Some((metric_name, InstrumentType::HistogramI64(value)));
-        } else {
-            self.record_debug(field, &value);
+    fn record_u128(&mut self, field: &Field, _value: u128) {
+        tracing::error!(name = field.name(), "metric attribute cannot be u128");
+    }
+
+    fn record_bool(&mut self, field: &Field, value: bool) {
+        self.custom_attributes.push(KeyValue::new(
+            Key::from_static_str(field.name()),
+            Value::from(value),
+        ));
+    }
+
+    fn record_str(&mut self, field: &Field, value: &str) {
+        self.custom_attributes.push(KeyValue::new(
+            Key::from_static_str(field.name()),
+            Value::from(value.to_string()),
+        ));
+    }
+
+    fn record_debug(&mut self, field: &Field, value: &dyn fmt::Debug) {
+        // Do not display the log content
+        if field.name() != "message" {
+            self.custom_attributes.push(KeyValue::new(
+                Key::from_static_str(field.name()),
+                Value::from(format!("{value:?}")),
+            ));
         }
     }
 }