Fix metrics attribute types (#3724)

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. Fixes: #3687  **Checklist** Complete the checklist (and note appropriate exceptions) before a final PR is raised. - [ ] Changes are compatible[^1] - [ ] Documentation[^2] completed - [ ] Performance impact assessed and acceptable - Tests added and passing[^3] - [ ] Unit Tests - [ ] Integration Tests - [ ] Manual Tests **Exceptions** *Note any exceptions here* **Notes** [^1]. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. [^2]. Configuration is an important part of many changes. Where applicable please try to document configuration examples. [^3]. Tick whichever testing boxes are applicable. If you are adding Manual Tests: - please document the manual testing (extensively) in the Exceptions. - please raise a separate issue to automate the test and label it (or ask for it to be labeled) as `manual test` --------- Co-authored-by: bryn <[email protected]>
apollographql · Sep 12, 2023 · 911185c · 911185c
1 parent cb8d618
commit 911185c
Show file tree

Hide file tree

Showing 19 changed files with 339 additions and 108 deletions.
diff --git a/.changesets/fix_bryn_fix_metrics_typing.md b/.changesets/fix_bryn_fix_metrics_typing.md
@@ -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
diff --git a/apollo-router/src/axum_factory/axum_http_server_factory.rs b/apollo-router/src/axum_factory/axum_http_server_factory.rs
@@ -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

diff --git a/apollo-router/src/axum_factory/listeners.rs b/apollo-router/src/axum_factory/listeners.rs
@@ -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
                                 );
 

diff --git a/apollo-router/src/configuration/metrics.rs b/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()),+);
                 }
             };
         }

diff --git a/...ation/snapshots/apollo_router__configuration__metrics__test__metrics@apq.router.yaml.snap b/...ation/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
 
diff --git a/...shots/apollo_router__configuration__metrics__test__metrics@authorization.router.yaml.snap b/...shots/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
 
diff --git a/...o_router__configuration__metrics__test__metrics@authorization_directives.router.yaml.snap b/...o_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
 
diff --git a/...apshots/apollo_router__configuration__metrics__test__metrics@coprocessor.router.yaml.snap b/...apshots/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
 
diff --git a/.../snapshots/apollo_router__configuration__metrics__test__metrics@entities.router.yaml.snap b/.../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
 
diff --git a/...on/snapshots/apollo_router__configuration__metrics__test__metrics@limits.router.yaml.snap b/...on/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
 
diff --git a/...s/apollo_router__configuration__metrics__test__metrics@persisted_queries.router.yaml.snap b/...s/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
 
diff --git a/...shots/apollo_router__configuration__metrics__test__metrics@subscriptions.router.yaml.snap b/...shots/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
 
diff --git a/...snapshots/apollo_router__configuration__metrics__test__metrics@telemetry.router.yaml.snap b/...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
 
diff --git a/...ots/apollo_router__configuration__metrics__test__metrics@traffic_shaping.router.yaml.snap b/...ots/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