diff --git a/.changesets/fix_bryn_fix_metrics_typing.md b/.changesets/fix_bryn_fix_metrics_typing.md new file mode 100644 index 0000000000..ab4d6eef03 --- /dev/null +++ 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 index 1ef5fc452f..0b414f25b1 100644 --- 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, ) -> 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::() { 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 index 24160afc22..9237fd68db 100644 --- 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 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..f80b5552f9 100644 --- a/apollo-router/src/plugins/telemetry/metrics/layer.rs +++ b/apollo-router/src/plugins/telemetry/metrics/layer.rs @@ -24,7 +24,13 @@ use super::METRIC_PREFIX_HISTOGRAM; use super::METRIC_PREFIX_MONOTONIC_COUNTER; use super::METRIC_PREFIX_VALUE; -const I64_MAX: u64 = i64::MAX as u64; +macro_rules! log_and_panic_in_debug_build { + ($($tokens:tt)+) => {{ + tracing::debug!($($tokens)+); + #[cfg(debug_assertions)] + panic!("metric type error, see DEBUG log for details. Release builds will not panic but will still emit a debug log message"); + }}; +} #[derive(Default)] pub(crate) struct Instruments { @@ -159,69 +165,247 @@ pub(crate) struct MetricVisitor<'a> { pub(crate) metric: Option<(&'static str, InstrumentType)>, pub(crate) custom_attributes: Vec, pub(crate) meter: &'a Meter, + attributes_ignored: bool, +} + +impl<'a> MetricVisitor<'a> { + fn set_metric(&mut self, name: &'static str, instrument_type: InstrumentType) { + self.metric = Some((name, instrument_type)); + if self.attributes_ignored { + log_and_panic_in_debug_build!( + metric_name = name, + "metric attributes must be declared after the metric value. Some attributes have been ignored" + ); + } + } } 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.set_metric(metric_name, InstrumentType::CounterF64(value)); + } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_COUNTER) { + self.set_metric(metric_name, InstrumentType::UpDownCounterF64(value)); + } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_HISTOGRAM) { + self.set_metric(metric_name, InstrumentType::HistogramF64(value)); + } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_VALUE) { + log_and_panic_in_debug_build!( + metric_name, + "gauge must be u64. This metric will be ignored" + ); + } else if self.metric.is_some() { self.custom_attributes.push(KeyValue::new( Key::from_static_str(field.name()), - Value::from(format!("{value:?}")), + Value::from(value), )); + } else { + self.attributes_ignored = true } } - 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) { + log_and_panic_in_debug_build!( + metric_name, + "monotonic counter must be u64 or f64. This metric will be ignored" + ); + } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_COUNTER) { + self.set_metric(metric_name, InstrumentType::UpDownCounterI64(value)); + } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_HISTOGRAM) { + self.set_metric(metric_name, InstrumentType::HistogramI64(value)); + } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_VALUE) { + log_and_panic_in_debug_build!( + metric_name, + "gauge must be u64. This metric will be ignored" + ); + } else if self.metric.is_some() { + self.custom_attributes.push(KeyValue::new( + Key::from_static_str(field.name()), + Value::from(value), + )); + } else { + self.attributes_ignored = true + } } 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))); + self.set_metric(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." - ); - } + log_and_panic_in_debug_build!( + metric_name, + "counter must be i64. This metric will be ignored" + ); } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_HISTOGRAM) { - self.metric = Some((metric_name, InstrumentType::HistogramU64(value))); + self.set_metric(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))); + self.set_metric(metric_name, InstrumentType::GaugeU64(value)); + } else if self.metric.is_some() { + log_and_panic_in_debug_build!( + name = field.name(), + "metric attribute must be i64, f64, string or bool. This attribute will be ignored" + ); } else { - self.record_debug(field, &value); + self.attributes_ignored = true } } - fn record_f64(&mut self, field: &Field, value: f64) { + fn record_i128(&mut self, field: &Field, _value: i128) { if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_MONOTONIC_COUNTER) { - self.metric = Some((metric_name, InstrumentType::CounterF64(value))); + log_and_panic_in_debug_build!( + metric_name, + "monotonic counter must be u64 or f64. This metric will be ignored" + ); } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_COUNTER) { - self.metric = Some((metric_name, InstrumentType::UpDownCounterF64(value))); + log_and_panic_in_debug_build!( + metric_name, + "counter must be i64. This metric will be ignored" + ); } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_HISTOGRAM) { - self.metric = Some((metric_name, InstrumentType::HistogramF64(value))); + log_and_panic_in_debug_build!( + metric_name, + "histogram must be u64, i64 or f64. This metric will be ignored" + ); + } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_VALUE) { + log_and_panic_in_debug_build!( + metric_name, + "gauge must be u64. This metric will be ignored" + ); + } else if self.metric.is_some() { + log_and_panic_in_debug_build!( + name = field.name(), + "metric attribute must be i64, f64, string or bool. This attribute will be ignored" + ); } else { - self.record_debug(field, &value); + self.attributes_ignored = true } } - fn record_i64(&mut self, field: &Field, value: i64) { + fn record_u128(&mut self, field: &Field, _value: u128) { if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_MONOTONIC_COUNTER) { - self.metric = Some((metric_name, InstrumentType::CounterU64(value as u64))); + log_and_panic_in_debug_build!( + metric_name, + "monotonic counter must be u64 or f64. This metric will be ignored" + ); } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_COUNTER) { - self.metric = Some((metric_name, InstrumentType::UpDownCounterI64(value))); + log_and_panic_in_debug_build!( + metric_name, + "counter must be i64. This metric will be ignored" + ); } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_HISTOGRAM) { - self.metric = Some((metric_name, InstrumentType::HistogramI64(value))); + log_and_panic_in_debug_build!( + metric_name, + "histogram must be u64, i64 or f64. This metric will be ignored" + ); + } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_VALUE) { + log_and_panic_in_debug_build!( + metric_name, + "gauge must be u64. This metric will be ignored" + ); + } else if self.metric.is_some() { + log_and_panic_in_debug_build!( + name = field.name(), + "metric attribute must be i64, f64, string or bool. This attribute will be ignored" + ); } else { - self.record_debug(field, &value); + self.attributes_ignored = true + } + } + + fn record_bool(&mut self, field: &Field, value: bool) { + if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_MONOTONIC_COUNTER) { + log_and_panic_in_debug_build!( + metric_name, + "monotonic counter must be u64 or f64. This metric will be ignored" + ); + } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_COUNTER) { + log_and_panic_in_debug_build!( + metric_name, + "counter must be i64. This metric will be ignored" + ); + } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_HISTOGRAM) { + log_and_panic_in_debug_build!( + metric_name, + "histogram must be u64, i64 or f64. This metric will be ignored" + ); + } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_VALUE) { + log_and_panic_in_debug_build!( + metric_name, + "gauge must be u64. This metric will be ignored" + ); + } else if self.metric.is_some() { + self.custom_attributes.push(KeyValue::new( + Key::from_static_str(field.name()), + Value::from(value), + )); + } else { + self.attributes_ignored = true + } + } + + fn record_str(&mut self, field: &Field, value: &str) { + if field.name() != "message" { + if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_MONOTONIC_COUNTER) { + log_and_panic_in_debug_build!( + metric_name, + "monotonic counter must be u64 or f64. This metric will be ignored" + ); + } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_COUNTER) { + log_and_panic_in_debug_build!( + metric_name, + "counter must be i64. This metric will be ignored" + ); + } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_HISTOGRAM) { + log_and_panic_in_debug_build!( + metric_name, + "histogram must be u64, i64 or f64. This metric will be ignored" + ); + } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_VALUE) { + log_and_panic_in_debug_build!( + metric_name, + "gauge must be u64. This metric will be ignored" + ); + } else if self.metric.is_some() { + self.custom_attributes.push(KeyValue::new( + Key::from_static_str(field.name()), + Value::from(value.to_string()), + )); + } else { + self.attributes_ignored = true + } + } + } + + fn record_debug(&mut self, field: &Field, value: &dyn fmt::Debug) { + if field.name() != "message" { + if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_MONOTONIC_COUNTER) { + log_and_panic_in_debug_build!( + metric_name, + "monotonic counter must be u64 or f64. This metric will be ignored" + ); + } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_COUNTER) { + log_and_panic_in_debug_build!( + metric_name, + "counter must be i64. This metric will be ignored" + ); + } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_HISTOGRAM) { + log_and_panic_in_debug_build!( + metric_name, + "histogram must be u64, i64 or f64. This metric will be ignored" + ); + } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_VALUE) { + log_and_panic_in_debug_build!( + metric_name, + "gauge must be u64. This metric will be ignored" + ); + } else if self.metric.is_some() { + self.custom_attributes.push(KeyValue::new( + Key::from_static_str(field.name()), + Value::from(format!("{value:?}")), + )); + } else { + self.attributes_ignored = true + } } } } @@ -265,6 +449,7 @@ where meter: &self.meter, metric: None, custom_attributes: Vec::new(), + attributes_ignored: false, }; event.record(&mut metric_visitor); metric_visitor.finish(); diff --git a/apollo-router/src/plugins/telemetry/mod.rs b/apollo-router/src/plugins/telemetry/mod.rs index c96157e4e4..0f46df3750 100644 --- a/apollo-router/src/plugins/telemetry/mod.rs +++ b/apollo-router/src/plugins/telemetry/mod.rs @@ -853,7 +853,7 @@ impl Telemetry { } ::tracing::info!( monotonic_counter.apollo.router.operations = 1u64, - http.response.status_code = parts.status.as_u16(), + http.response.status_code = parts.status.as_u16() as i64, ); let response = http::Response::from_parts( parts, @@ -869,7 +869,7 @@ impl Telemetry { ::tracing::info!( monotonic_counter.apollo.router.operations = 1u64, - http.response.status_code = 500, + http.response.status_code = 500i64, ); Err(err) } diff --git a/apollo-router/src/plugins/traffic_shaping/timeout/future.rs b/apollo-router/src/plugins/traffic_shaping/timeout/future.rs index 924fe6b215..8a390b393e 100644 --- a/apollo-router/src/plugins/traffic_shaping/timeout/future.rs +++ b/apollo-router/src/plugins/traffic_shaping/timeout/future.rs @@ -49,7 +49,7 @@ where match Pin::new(&mut this.sleep).poll(cx) { Poll::Pending => Poll::Pending, Poll::Ready(_) => { - tracing::info!(monotonic_counter.apollo_router_timeout = 1,); + tracing::info!(monotonic_counter.apollo_router_timeout = 1u64,); Poll::Ready(Err(Elapsed::new().into())) } } diff --git a/apollo-router/src/query_planner/bridge_query_planner.rs b/apollo-router/src/query_planner/bridge_query_planner.rs index e4929420de..7805d127f9 100644 --- a/apollo-router/src/query_planner/bridge_query_planner.rs +++ b/apollo-router/src/query_planner/bridge_query_planner.rs @@ -91,7 +91,7 @@ impl BridgeQueryPlanner { if has_validation_errors && !schema.has_errors() { tracing::warn!( - monotonic_counter.apollo.router.validation = 1, + monotonic_counter.apollo.router.validation = 1u64, validation.source = VALIDATION_SOURCE_SCHEMA, validation.result = VALIDATION_FALSE_NEGATIVE, "validation mismatch: JS query planner reported a schema validation error, but apollo-rs did not" @@ -106,7 +106,7 @@ impl BridgeQueryPlanner { if configuration.experimental_graphql_validation_mode == GraphQLValidationMode::Both { if schema.has_errors() { tracing::warn!( - monotonic_counter.apollo.router.validation = 1, + monotonic_counter.apollo.router.validation = 1u64, validation.source = VALIDATION_SOURCE_SCHEMA, validation.result = VALIDATION_FALSE_POSITIVE, "validation mismatch: apollo-rs reported a schema validation error, but JS query planner did not" @@ -114,7 +114,7 @@ impl BridgeQueryPlanner { } else { // false_negative was an early return so we know it was correct here tracing::info!( - monotonic_counter.apollo.router.validation = 1, + monotonic_counter.apollo.router.validation = 1u64, validation.source = VALIDATION_SOURCE_SCHEMA, validation.result = VALIDATION_MATCH ); @@ -286,7 +286,7 @@ impl BridgeQueryPlanner { match (is_validation_error, &selections.validation_error) { (false, Some(_)) => { tracing::warn!( - monotonic_counter.apollo.router.validation = 1, + monotonic_counter.apollo.router.validation = 1u64, validation.source = VALIDATION_SOURCE_OPERATION, validation.result = VALIDATION_FALSE_POSITIVE, "validation mismatch: JS query planner did not report query validation error, but apollo-rs did" @@ -294,7 +294,7 @@ impl BridgeQueryPlanner { } (true, None) => { tracing::warn!( - monotonic_counter.apollo.router.validation = 1, + monotonic_counter.apollo.router.validation = 1u64, validation.source = VALIDATION_SOURCE_OPERATION, validation.result = VALIDATION_FALSE_NEGATIVE, "validation mismatch: apollo-rs did not report query validation error, but JS query planner did" @@ -302,7 +302,7 @@ impl BridgeQueryPlanner { } // if JS and Rust implementations agree, we return the JS result for now. _ => tracing::info!( - monotonic_counter.apollo.router.validation = 1, + monotonic_counter.apollo.router.validation = 1u64, validation.source = VALIDATION_SOURCE_OPERATION, validation.result = VALIDATION_MATCH, ), diff --git a/apollo-router/src/uplink/mod.rs b/apollo-router/src/uplink/mod.rs index b72ef91935..2fb38ef4d1 100644 --- a/apollo-router/src/uplink/mod.rs +++ b/apollo-router/src/uplink/mod.rs @@ -196,7 +196,7 @@ where match fetch::(&client, &query_body, &mut endpoints.iter()).await { Ok(response) => { tracing::info!( - counter.apollo_router_uplink_fetch_count_total = 1, + monotonic_counter.apollo_router_uplink_fetch_count_total = 1u64, status = "success", query ); @@ -245,7 +245,7 @@ where } Err(err) => { tracing::info!( - counter.apollo_router_uplink_fetch_count_total = 1, + monotonic_counter.apollo_router_uplink_fetch_count_total = 1u64, status = "failure", query );