From 911185cf4cd1453ace7923f00d78ec3f45b191f6 Mon Sep 17 00:00:00 2001 From: Bryn Cooke Date: Wed, 6 Sep 2023 11:36:49 +0100 Subject: [PATCH] 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 --- .changesets/fix_bryn_fix_metrics_typing.md | 6 + .../axum_factory/axum_http_server_factory.rs | 6 +- apollo-router/src/axum_factory/listeners.rs | 4 +- apollo-router/src/configuration/metrics.rs | 54 +++- ...etrics__test__metrics@apq.router.yaml.snap | 6 +- ...st__metrics@authorization.router.yaml.snap | 4 +- ...@authorization_directives.router.yaml.snap | 4 +- ...test__metrics@coprocessor.router.yaml.snap | 12 +- ...s__test__metrics@entities.router.yaml.snap | 18 +- ...ics__test__metrics@limits.router.yaml.snap | 16 +- ...metrics@persisted_queries.router.yaml.snap | 6 +- ...st__metrics@subscriptions.router.yaml.snap | 10 +- ...__test__metrics@telemetry.router.yaml.snap | 12 +- ...__metrics@traffic_shaping.router.yaml.snap | 16 +- .../src/plugins/telemetry/metrics/layer.rs | 251 +++++++++++++++--- apollo-router/src/plugins/telemetry/mod.rs | 4 +- .../plugins/traffic_shaping/timeout/future.rs | 2 +- .../src/query_planner/bridge_query_planner.rs | 12 +- apollo-router/src/uplink/mod.rs | 4 +- 19 files changed, 339 insertions(+), 108 deletions(-) create mode 100644 .changesets/fix_bryn_fix_metrics_typing.md 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 );