apollographql · BrynCooke · Sep 6, 2023 · Sep 1, 2023 · Sep 4, 2023 · Sep 4, 2023
@@ -0,0 +1,6 @@
+### Fix metrics attribute types ([Issue #3687](https://github.com/apollographql/router/issues/3687))
+
+Metrics attributes were being coerced to strings. This is now fixed.
+In addition, the logic around types accepted as metrics attributes has been simplified. It will log and ignore values of the wrong type.
+
+By [@BrynCooke](https://github.com/BrynCooke) in https://github.com/apollographql/router/pull/3724
@@ -500,7 +500,7 @@ async fn handle_graphql(
     service: router::BoxService,
     http_request: Request<Body>,
 ) -> impl IntoResponse {
-    tracing::info!(counter.apollo_router_session_count_active = 1,);
+    tracing::info!(counter.apollo_router_session_count_active = 1i64,);
 
     let request: router::Request = http_request.into();
     let context = request.context.clone();
@@ -518,7 +518,7 @@ async fn handle_graphql(
 
     match res {
         Err(e) => {
-            tracing::info!(counter.apollo_router_session_count_active = -1,);
+            tracing::info!(counter.apollo_router_session_count_active = -1i64,);
             if let Some(source_err) = e.source() {
                 if source_err.is::<RateLimited>() {
                     return RateLimited::new().into_response();
@@ -541,7 +541,7 @@ async fn handle_graphql(
                 .into_response()
         }
         Ok(response) => {
-            tracing::info!(counter.apollo_router_session_count_active = -1,);
+            tracing::info!(counter.apollo_router_session_count_active = -1i64,);
             let (mut parts, body) = response.response.into_parts();
 
             let opt_compressor = accept_encoding

@@ -218,7 +218,7 @@ pub(super) fn serve_router_on_listen_addr(
                             }
 
                             tracing::info!(
-                                counter.apollo_router_session_count_total = 1,
+                                counter.apollo_router_session_count_total = 1i64,
                                 listener = &address
                             );
 
@@ -312,7 +312,7 @@ pub(super) fn serve_router_on_listen_addr(
                                 }
 
                                 tracing::info!(
-                                    counter.apollo_router_session_count_total = -1,
+                                    counter.apollo_router_session_count_total = -1i64,
                                     listener = &address
                                 );
 

@@ -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()),+);
                 }
             };
         }

@@ -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
 
@@ -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
 
@@ -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
 
@@ -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
 
@@ -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
 
@@ -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
 
@@ -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
 
@@ -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
 
@@ -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
 
@@ -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
 
@@ -627,7 +627,7 @@ fn authenticate(
             monotonic_counter.apollo_authentication_success_count = 1u64,
             kind = %AUTHENTICATION_KIND
         );
-        tracing::info!(monotonic_counter.apollo.router.operations.jwt = 1);
+        tracing::info!(monotonic_counter.apollo.router.operations.jwt = 1u64);
         return Ok(ControlFlow::Continue(request));
     }