diff --git a/apollo-router/src/configuration/metrics.rs b/apollo-router/src/configuration/metrics.rs index 0e8d5e74fb..eb459b92e9 100644 --- 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)>, + metrics: HashMap)>, +} + +enum AttributeValue { + Bool(bool), + U64(u64), + I64(i64), + F64(f64), + String(String), +} + +impl Serialize for AttributeValue { + fn serialize(&self, serializer: S) -> Result { + 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/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@apq.router.yaml.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@apq.router.yaml.snap index bf5efaf603..9108dfc7a1 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@apq.router.yaml.snap +++ b/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 diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@authorization.router.yaml.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@authorization.router.yaml.snap index 11f9160614..e45a4962f7 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@authorization.router.yaml.snap +++ b/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 diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@authorization_directives.router.yaml.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@authorization_directives.router.yaml.snap index 61b5d4c144..38462ec606 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@authorization_directives.router.yaml.snap +++ b/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 diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@coprocessor.router.yaml.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@coprocessor.router.yaml.snap index b5eb1df764..bdc1a7899b 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@coprocessor.router.yaml.snap +++ b/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 diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@entities.router.yaml.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@entities.router.yaml.snap index 1bce92d5c8..e4fe10d957 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@entities.router.yaml.snap +++ b/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 diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@limits.router.yaml.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@limits.router.yaml.snap index 53807bab66..055f60152d 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@limits.router.yaml.snap +++ b/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 diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@persisted_queries.router.yaml.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@persisted_queries.router.yaml.snap index 507f9c756f..72b803ca49 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@persisted_queries.router.yaml.snap +++ b/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 diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@subscriptions.router.yaml.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@subscriptions.router.yaml.snap index 3709a1603d..a019d34928 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@subscriptions.router.yaml.snap +++ b/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 diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@telemetry.router.yaml.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@telemetry.router.yaml.snap index 7e02cf7f31..8ea0c00cab 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@telemetry.router.yaml.snap +++ b/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 diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@traffic_shaping.router.yaml.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@traffic_shaping.router.yaml.snap index 1cdb685e7d..ab53cd0460 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@traffic_shaping.router.yaml.snap +++ b/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 diff --git a/apollo-router/src/plugins/telemetry/metrics/layer.rs b/apollo-router/src/plugins/telemetry/metrics/layer.rs index c195891d7b..c5b5e6bd8c 100644 --- a/apollo-router/src/plugins/telemetry/metrics/layer.rs +++ b/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>, @@ -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:?}")), + )); } } }