From e48c4ac8ab1162d7cebd9015287e6bbc39004e3f Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Tue, 9 Jul 2024 11:52:37 +0200 Subject: [PATCH 01/10] perf: don't recreate meter and instruments on every calls Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- .../plugins/telemetry/config_new/cost/mod.rs | 53 +- .../telemetry/config_new/graphql/mod.rs | 80 +-- .../telemetry/config_new/instruments.rs | 669 +++++++++++++++--- apollo-router/src/plugins/telemetry/mod.rs | 91 ++- apollo-router/src/plugins/test.rs | 2 +- 5 files changed, 707 insertions(+), 188 deletions(-) diff --git a/apollo-router/src/plugins/telemetry/config_new/cost/mod.rs b/apollo-router/src/plugins/telemetry/config_new/cost/mod.rs index df3dcf7b0d..503b191904 100644 --- a/apollo-router/src/plugins/telemetry/config_new/cost/mod.rs +++ b/apollo-router/src/plugins/telemetry/config_new/cost/mod.rs @@ -1,3 +1,4 @@ +use std::collections::HashMap; use std::sync::Arc; use opentelemetry::metrics::MeterProvider; @@ -9,6 +10,7 @@ use serde::Deserialize; use tower::BoxError; use super::instruments::Increment; +use super::instruments::StaticInstrument; use crate::metrics; use crate::plugins::demand_control::CostContext; use crate::plugins::telemetry::config::AttributeValue; @@ -115,7 +117,28 @@ pub(crate) struct CostInstrumentsConfig { } impl CostInstrumentsConfig { - pub(crate) fn to_instruments(&self) -> CostInstruments { + pub(crate) fn new_static_instruments(&self) -> HashMap { + let meter = metrics::meter_provider() + .meter(crate::plugins::telemetry::config_new::instruments::METER_NAME); + + [( + COST_ESTIMATED.to_string(), + StaticInstrument::Histogram(meter.f64_histogram(COST_ESTIMATED).with_description("Estimated cost of the operation using the currently configured cost model").init()), + ),( + COST_ACTUAL.to_string(), + StaticInstrument::Histogram(meter.f64_histogram(COST_ACTUAL).with_description("Actual cost of the operation using the currently configured cost model").init()), + ),( + COST_DELTA.to_string(), + StaticInstrument::Histogram(meter.f64_histogram(COST_DELTA).with_description("Delta between the estimated and actual cost of the operation using the currently configured cost model").init()), + )] + .into_iter() + .collect() + } + + pub(crate) fn to_instruments( + &self, + static_instruments: Arc>, + ) -> CostInstruments { let cost_estimated = self.cost_estimated.is_enabled().then(|| { Self::histogram( COST_ESTIMATED, @@ -123,6 +146,7 @@ impl CostInstrumentsConfig { SupergraphSelector::Cost { cost: CostValue::Estimated, }, + &static_instruments, ) }); @@ -133,6 +157,7 @@ impl CostInstrumentsConfig { SupergraphSelector::Cost { cost: CostValue::Actual, }, + &static_instruments, ) }); @@ -143,6 +168,7 @@ impl CostInstrumentsConfig { SupergraphSelector::Cost { cost: CostValue::Delta, }, + &static_instruments, ) }); CostInstruments { @@ -156,9 +182,8 @@ impl CostInstrumentsConfig { name: &'static str, config: &DefaultedStandardInstrument>, selector: SupergraphSelector, + static_instruments: &Arc>, ) -> CustomHistogram { - let meter = metrics::meter_provider() - .meter(crate::plugins::telemetry::config_new::instruments::METER_NAME); let mut nb_attributes = 0; let selectors = match config { DefaultedStandardInstrument::Bool(_) | DefaultedStandardInstrument::Unset => None, @@ -172,7 +197,13 @@ impl CostInstrumentsConfig { inner: Mutex::new(CustomHistogramInner { increment: Increment::EventCustom(None), condition: Condition::True, - histogram: Some(meter.f64_histogram(name).init()), + histogram: Some( + static_instruments + .get(name) + .expect("cannot get static instrument for cost; this should not happen") + .as_histogram() + .expect("cannot convert instrument to histogram for cost; this should not happen").clone(), + ), attributes: Vec::with_capacity(nb_attributes), selector: Some(Arc::new(selector)), selectors, @@ -307,6 +338,8 @@ pub(crate) fn add_cost_attributes(context: &Context, custom_attributes: &mut Vec #[cfg(test)] mod test { + use std::sync::Arc; + use crate::context::OPERATION_NAME; use crate::plugins::demand_control::CostContext; use crate::plugins::telemetry::config_new::cost::CostInstruments; @@ -318,7 +351,7 @@ mod test { #[test] fn test_default_estimated() { let config = config(include_str!("fixtures/cost_estimated.router.yaml")); - let instruments = config.to_instruments(); + let instruments = config.to_instruments(Arc::new(config.new_static_instruments())); make_request(&instruments); assert_histogram_sum!("cost.estimated", 100.0); @@ -330,7 +363,7 @@ mod test { #[test] fn test_default_actual() { let config = config(include_str!("fixtures/cost_actual.router.yaml")); - let instruments = config.to_instruments(); + let instruments = config.to_instruments(Arc::new(config.new_static_instruments())); make_request(&instruments); assert_histogram_sum!("cost.actual", 10.0); @@ -342,7 +375,7 @@ mod test { #[test] fn test_default_delta() { let config = config(include_str!("fixtures/cost_delta.router.yaml")); - let instruments = config.to_instruments(); + let instruments = config.to_instruments(Arc::new(config.new_static_instruments())); make_request(&instruments); assert_histogram_sum!("cost.delta", 90.0); @@ -356,7 +389,7 @@ mod test { let config = config(include_str!( "fixtures/cost_estimated_with_attributes.router.yaml" )); - let instruments = config.to_instruments(); + let instruments = config.to_instruments(Arc::new(config.new_static_instruments())); make_request(&instruments); assert_histogram_sum!("cost.estimated", 100.0, cost.result = "COST_TOO_EXPENSIVE"); @@ -370,7 +403,7 @@ mod test { let config = config(include_str!( "fixtures/cost_actual_with_attributes.router.yaml" )); - let instruments = config.to_instruments(); + let instruments = config.to_instruments(Arc::new(config.new_static_instruments())); make_request(&instruments); assert_histogram_sum!("cost.actual", 10.0, cost.result = "COST_TOO_EXPENSIVE"); @@ -384,7 +417,7 @@ mod test { let config = config(include_str!( "fixtures/cost_delta_with_attributes.router.yaml" )); - let instruments = config.to_instruments(); + let instruments = config.to_instruments(Arc::new(config.new_static_instruments())); make_request(&instruments); assert_histogram_sum!( diff --git a/apollo-router/src/plugins/telemetry/config_new/graphql/mod.rs b/apollo-router/src/plugins/telemetry/config_new/graphql/mod.rs index 653e906384..0e082e93d5 100644 --- a/apollo-router/src/plugins/telemetry/config_new/graphql/mod.rs +++ b/apollo-router/src/plugins/telemetry/config_new/graphql/mod.rs @@ -1,32 +1,20 @@ -use std::sync::Arc; - use apollo_compiler::ast::NamedType; use apollo_compiler::executable::Field; use apollo_compiler::ExecutableDocument; -use opentelemetry::metrics::MeterProvider; -use parking_lot::Mutex; use schemars::JsonSchema; use serde::Deserialize; use serde_json_bytes::Value; use tower::BoxError; use super::instruments::CustomCounter; -use super::instruments::CustomCounterInner; use super::instruments::CustomInstruments; -use super::instruments::Increment; -use super::instruments::InstrumentsConfig; -use super::instruments::METER_NAME; use crate::graphql::ResponseVisitor; -use crate::metrics; use crate::plugins::telemetry::config_new::attributes::DefaultAttributeRequirementLevel; -use crate::plugins::telemetry::config_new::conditions::Condition; use crate::plugins::telemetry::config_new::extendable::Extendable; use crate::plugins::telemetry::config_new::graphql::attributes::GraphQLAttributes; use crate::plugins::telemetry::config_new::graphql::selectors::GraphQLSelector; use crate::plugins::telemetry::config_new::graphql::selectors::GraphQLValue; -use crate::plugins::telemetry::config_new::graphql::selectors::ListLength; use crate::plugins::telemetry::config_new::instruments::CustomHistogram; -use crate::plugins::telemetry::config_new::instruments::CustomHistogramInner; use crate::plugins::telemetry::config_new::instruments::DefaultedStandardInstrument; use crate::plugins::telemetry::config_new::instruments::Instrumented; use crate::plugins::telemetry::config_new::DefaultForLevel; @@ -37,8 +25,8 @@ use crate::Context; pub(crate) mod attributes; pub(crate) mod selectors; -static FIELD_LENGTH: &str = "graphql.field.list.length"; -static FIELD_EXECUTION: &str = "graphql.field.execution"; +pub(crate) const FIELD_LENGTH: &str = "graphql.field.list.length"; +pub(crate) const FIELD_EXECUTION: &str = "graphql.field.execution"; #[derive(Deserialize, JsonSchema, Clone, Default, Debug)] #[serde(deny_unknown_fields, default)] @@ -98,67 +86,6 @@ pub(crate) struct GraphQLInstruments { pub(crate) custom: GraphQLCustomInstruments, } -impl From<&InstrumentsConfig> for GraphQLInstruments { - fn from(value: &InstrumentsConfig) -> Self { - let meter = metrics::meter_provider().meter(METER_NAME); - GraphQLInstruments { - list_length: value.graphql.attributes.list_length.is_enabled().then(|| { - let mut nb_attributes = 0; - let selectors = match &value.graphql.attributes.list_length { - DefaultedStandardInstrument::Bool(_) | DefaultedStandardInstrument::Unset => { - None - } - DefaultedStandardInstrument::Extendable { attributes } => { - nb_attributes = attributes.custom.len(); - Some(attributes.clone()) - } - }; - CustomHistogram { - inner: Mutex::new(CustomHistogramInner { - increment: Increment::FieldCustom(None), - condition: Condition::True, - histogram: Some(meter.f64_histogram(FIELD_LENGTH).init()), - attributes: Vec::with_capacity(nb_attributes), - selector: Some(Arc::new(GraphQLSelector::ListLength { - list_length: ListLength::Value, - })), - selectors, - updated: false, - }), - } - }), - field_execution: value - .graphql - .attributes - .field_execution - .is_enabled() - .then(|| { - let mut nb_attributes = 0; - let selectors = match &value.graphql.attributes.field_execution { - DefaultedStandardInstrument::Bool(_) - | DefaultedStandardInstrument::Unset => None, - DefaultedStandardInstrument::Extendable { attributes } => { - nb_attributes = attributes.custom.len(); - Some(attributes.clone()) - } - }; - CustomCounter { - inner: Mutex::new(CustomCounterInner { - increment: Increment::FieldUnit, - condition: Condition::True, - counter: Some(meter.f64_counter(FIELD_EXECUTION).init()), - attributes: Vec::with_capacity(nb_attributes), - selector: None, - selectors, - incremented: false, - }), - } - }), - custom: CustomInstruments::new(&value.graphql.custom), - } - } -} - impl Instrumented for GraphQLInstruments { type Request = supergraph::Request; type Response = supergraph::Response; @@ -327,12 +254,11 @@ pub(crate) mod test { .build() .unwrap(); - let harness = PluginTestHarness::::builder() + let harness: PluginTestHarness = PluginTestHarness::::builder() .config(include_str!("fixtures/field_length_enabled.router.yaml")) .schema(schema_str) .build() .await; - harness .call_supergraph(request, |req| { let response: serde_json::Value = serde_json::from_str(include_str!( diff --git a/apollo-router/src/plugins/telemetry/config_new/instruments.rs b/apollo-router/src/plugins/telemetry/config_new/instruments.rs index 2d615d2950..f9fa46176e 100644 --- a/apollo-router/src/plugins/telemetry/config_new/instruments.rs +++ b/apollo-router/src/plugins/telemetry/config_new/instruments.rs @@ -21,6 +21,10 @@ use tokio::time::Instant; use tower::BoxError; use super::attributes::HttpServerAttributes; +use super::graphql::selectors::ListLength; +use super::graphql::GraphQLInstruments; +use super::graphql::FIELD_EXECUTION; +use super::graphql::FIELD_LENGTH; use super::DefaultForLevel; use super::Selector; use crate::metrics; @@ -79,6 +83,15 @@ pub(crate) struct InstrumentsConfig { >, } +const HTTP_SERVER_REQUEST_DURATION_METRIC: &str = "http.server.request.duration"; +const HTTP_SERVER_REQUEST_BODY_SIZE_METRIC: &str = "http.server.request.body.size"; +const HTTP_SERVER_RESPONSE_BODY_SIZE_METRIC: &str = "http.server.response.body.size"; +const HTTP_SERVER_ACTIVE_REQUESTS: &str = "http.server.active_requests"; + +const HTTP_CLIENT_REQUEST_DURATION_METRIC: &str = "http.client.request.duration"; +const HTTP_CLIENT_REQUEST_BODY_SIZE_METRIC: &str = "http.client.request.body.size"; +const HTTP_CLIENT_RESPONSE_BODY_SIZE_METRIC: &str = "http.client.response.body.size"; + impl InstrumentsConfig { /// Update the defaults for spans configuration regarding the `default_attribute_requirement_level` pub(crate) fn update_defaults(&mut self) { @@ -93,8 +106,118 @@ impl InstrumentsConfig { .defaults_for_levels(self.default_requirement_level, TelemetryDataKind::Metrics); } - pub(crate) fn new_router_instruments(&self) -> RouterInstruments { + pub(crate) fn new_static_router_instruments(&self) -> HashMap { let meter = metrics::meter_provider().meter(METER_NAME); + let mut static_instruments = HashMap::with_capacity(self.router.custom.len()); + + if self + .router + .attributes + .http_server_request_duration + .is_enabled() + { + static_instruments.insert( + HTTP_SERVER_REQUEST_DURATION_METRIC.to_string(), + StaticInstrument::Histogram( + meter + .f64_histogram(HTTP_SERVER_REQUEST_DURATION_METRIC) + .with_unit(Unit::new("s")) + .with_description("Duration of HTTP server requests.") + .init(), + ), + ); + } + + if self + .router + .attributes + .http_server_request_body_size + .is_enabled() + { + static_instruments.insert( + HTTP_SERVER_REQUEST_BODY_SIZE_METRIC.to_string(), + StaticInstrument::Histogram( + meter + .f64_histogram(HTTP_SERVER_REQUEST_BODY_SIZE_METRIC) + .with_unit(Unit::new("By")) + .with_description("Size of HTTP server request bodies.") + .init(), + ), + ); + } + + if self + .router + .attributes + .http_server_response_body_size + .is_enabled() + { + static_instruments.insert( + HTTP_SERVER_RESPONSE_BODY_SIZE_METRIC.to_string(), + StaticInstrument::Histogram( + meter + .f64_histogram(HTTP_SERVER_RESPONSE_BODY_SIZE_METRIC) + .with_unit(Unit::new("By")) + .with_description("Size of HTTP server response bodies.") + .init(), + ), + ); + } + + if self + .router + .attributes + .http_server_active_requests + .is_enabled() + { + static_instruments.insert( + HTTP_SERVER_ACTIVE_REQUESTS.to_string(), + StaticInstrument::UpDownCounterI64( + meter + .i64_up_down_counter(HTTP_SERVER_ACTIVE_REQUESTS) + .with_unit(Unit::new("request")) + .with_description("Number of active HTTP server requests.") + .init(), + ), + ); + } + + for (instrument_name, instrument) in &self.router.custom { + match instrument.ty { + InstrumentType::Counter => { + static_instruments.insert( + instrument_name.clone(), + StaticInstrument::CounterF64( + meter + .f64_counter(instrument_name.clone()) + .with_description(instrument.description.clone()) + .with_unit(Unit::new(instrument.unit.clone())) + .init(), + ), + ); + } + InstrumentType::Histogram => { + static_instruments.insert( + instrument_name.clone(), + StaticInstrument::Histogram( + meter + .f64_histogram(instrument_name.clone()) + .with_description(instrument.description.clone()) + .with_unit(Unit::new(instrument.unit.clone())) + .init(), + ), + ); + } + } + } + + static_instruments + } + + pub(crate) fn new_router_instruments( + &self, + static_instruments: Arc>, + ) -> RouterInstruments { let http_server_request_duration = self .router .attributes @@ -105,11 +228,16 @@ impl InstrumentsConfig { increment: Increment::Duration(Instant::now()), condition: Condition::True, histogram: Some( - meter - .f64_histogram("http.server.request.duration") - .with_unit(Unit::new("s")) - .with_description("Duration of HTTP server requests.") - .init(), + static_instruments + .get(HTTP_SERVER_REQUEST_DURATION_METRIC) + .expect( + "cannot get static instrument for router; this should not happen", + ) + .as_histogram() + .cloned() + .expect( + "cannot convert instrument to histogram for router; this should not happen", + ), ), attributes: Vec::new(), selector: None, @@ -143,11 +271,15 @@ impl InstrumentsConfig { increment: Increment::Custom(None), condition: Condition::True, histogram: Some( - meter - .f64_histogram("http.server.request.body.size") - .with_unit(Unit::new("By")) - .with_description("Size of HTTP server request bodies.") - .init(), + static_instruments + .get(HTTP_SERVER_REQUEST_BODY_SIZE_METRIC) + .expect( + "cannot get static instrument for router; this should not happen", + ) + .as_histogram() + .cloned().expect( + "cannot convert instrument to histogram for router; this should not happen", + ) ), attributes: Vec::with_capacity(nb_attributes), selector: Some(Arc::new(RouterSelector::RequestHeader { @@ -181,11 +313,16 @@ impl InstrumentsConfig { increment: Increment::Custom(None), condition: Condition::True, histogram: Some( - meter - .f64_histogram("http.server.response.body.size") - .with_unit(Unit::new("By")) - .with_description("Size of HTTP server response bodies.") - .init(), + static_instruments + .get(HTTP_SERVER_RESPONSE_BODY_SIZE_METRIC) + .expect( + "cannot get static instrument for router; this should not happen", + ) + .as_histogram() + .cloned() + .expect( + "cannot convert instrument to histogram for router; this should not happen", + ) ), attributes: Vec::with_capacity(nb_attributes), selector: Some(Arc::new(RouterSelector::ResponseHeader { @@ -206,11 +343,16 @@ impl InstrumentsConfig { .then(|| ActiveRequestsCounter { inner: Mutex::new(ActiveRequestsCounterInner { counter: Some( - meter - .i64_up_down_counter("http.server.active_requests") - .with_unit(Unit::new("request")) - .with_description("Number of active HTTP server requests.") - .init(), + static_instruments + .get(HTTP_SERVER_ACTIVE_REQUESTS) + .expect( + "cannot get static instrument for router; this should not happen", + ) + .as_up_down_counter_i64() + .cloned() + .expect( + "cannot convert instrument to up and down counter for router; this should not happen", + ), ), attrs_config: match &self.router.attributes.http_server_active_requests { DefaultedStandardInstrument::Bool(_) @@ -227,19 +369,155 @@ impl InstrumentsConfig { http_server_request_body_size, http_server_response_body_size, http_server_active_requests, - custom: CustomInstruments::new(&self.router.custom), + custom: CustomInstruments::new(&self.router.custom, static_instruments), + } + } + + pub(crate) fn new_static_supergraph_instruments(&self) -> HashMap { + let meter = metrics::meter_provider().meter(METER_NAME); + + let mut static_instruments = HashMap::with_capacity(self.supergraph.custom.len()); + for (instrument_name, instrument) in &self.supergraph.custom { + match instrument.ty { + InstrumentType::Counter => { + static_instruments.insert( + instrument_name.clone(), + StaticInstrument::CounterF64( + meter + .f64_counter(instrument_name.clone()) + .with_description(instrument.description.clone()) + .with_unit(Unit::new(instrument.unit.clone())) + .init(), + ), + ); + } + InstrumentType::Histogram => { + static_instruments.insert( + instrument_name.clone(), + StaticInstrument::Histogram( + meter + .f64_histogram(instrument_name.clone()) + .with_description(instrument.description.clone()) + .with_unit(Unit::new(instrument.unit.clone())) + .init(), + ), + ); + } + } } + static_instruments.extend(self.supergraph.attributes.cost.new_static_instruments()); + + static_instruments } - pub(crate) fn new_supergraph_instruments(&self) -> SupergraphInstruments { + pub(crate) fn new_supergraph_instruments( + &self, + static_instruments: Arc>, + ) -> SupergraphInstruments { SupergraphInstruments { - cost: self.supergraph.attributes.cost.to_instruments(), - custom: CustomInstruments::new(&self.supergraph.custom), + cost: self + .supergraph + .attributes + .cost + .to_instruments(static_instruments.clone()), + custom: CustomInstruments::new(&self.supergraph.custom, static_instruments), } } - pub(crate) fn new_subgraph_instruments(&self) -> SubgraphInstruments { + pub(crate) fn new_static_subgraph_instruments(&self) -> HashMap { let meter = metrics::meter_provider().meter(METER_NAME); + let mut static_instruments = HashMap::with_capacity(self.subgraph.custom.len()); + + if self + .subgraph + .attributes + .http_client_request_duration + .is_enabled() + { + static_instruments.insert( + HTTP_CLIENT_REQUEST_DURATION_METRIC.to_string(), + StaticInstrument::Histogram( + meter + .f64_histogram(HTTP_CLIENT_REQUEST_DURATION_METRIC) + .with_unit(Unit::new("s")) + .with_description("Duration of HTTP client requests.") + .init(), + ), + ); + } + + if self + .subgraph + .attributes + .http_client_request_body_size + .is_enabled() + { + static_instruments.insert( + HTTP_CLIENT_REQUEST_BODY_SIZE_METRIC.to_string(), + StaticInstrument::Histogram( + meter + .f64_histogram(HTTP_CLIENT_REQUEST_BODY_SIZE_METRIC) + .with_unit(Unit::new("By")) + .with_description("Size of HTTP client request bodies.") + .init(), + ), + ); + } + + if self + .subgraph + .attributes + .http_client_response_body_size + .is_enabled() + { + static_instruments.insert( + HTTP_CLIENT_RESPONSE_BODY_SIZE_METRIC.to_string(), + StaticInstrument::Histogram( + meter + .f64_histogram(HTTP_CLIENT_RESPONSE_BODY_SIZE_METRIC) + .with_unit(Unit::new("By")) + .with_description("Size of HTTP client response bodies.") + .init(), + ), + ); + } + + for (instrument_name, instrument) in &self.subgraph.custom { + match instrument.ty { + InstrumentType::Counter => { + static_instruments.insert( + instrument_name.clone(), + StaticInstrument::CounterF64( + meter + .f64_counter(instrument_name.clone()) + .with_description(instrument.description.clone()) + .with_unit(Unit::new(instrument.unit.clone())) + .init(), + ), + ); + } + InstrumentType::Histogram => { + static_instruments.insert( + instrument_name.clone(), + StaticInstrument::Histogram( + meter + .f64_histogram(instrument_name.clone()) + .with_description(instrument.description.clone()) + .with_unit(Unit::new(instrument.unit.clone())) + .init(), + ), + ); + } + } + } + + static_instruments + } + + pub(crate) fn new_subgraph_instruments( + &self, + static_instruments: Arc>, + ) -> SubgraphInstruments { let http_client_request_duration = self.subgraph .attributes @@ -259,12 +537,16 @@ impl InstrumentsConfig { inner: Mutex::new(CustomHistogramInner { increment: Increment::Duration(Instant::now()), condition: Condition::True, - histogram: Some( - meter - .f64_histogram("http.client.request.duration") - .with_unit(Unit::new("s")) - .with_description("Duration of HTTP client requests.") - .init(), + histogram: Some(static_instruments + .get(HTTP_CLIENT_REQUEST_DURATION_METRIC) + .expect( + "cannot get static instrument for subgraph; this should not happen", + ) + .as_histogram() + .cloned() + .expect( + "cannot convert instrument to histogram for subgraph; this should not happen", + ) ), attributes: Vec::with_capacity(nb_attributes), selector: None, @@ -292,12 +574,16 @@ impl InstrumentsConfig { inner: Mutex::new(CustomHistogramInner { increment: Increment::Custom(None), condition: Condition::True, - histogram: Some( - meter - .f64_histogram("http.client.request.body.size") - .with_unit(Unit::new("By")) - .with_description("Size of HTTP client request bodies.") - .init(), + histogram: Some(static_instruments + .get(HTTP_CLIENT_REQUEST_BODY_SIZE_METRIC) + .expect( + "cannot get static instrument for subgraph; this should not happen", + ) + .as_histogram() + .cloned() + .expect( + "cannot convert instrument to histogram for subgraph; this should not happen", + ) ), attributes: Vec::with_capacity(nb_attributes), selector: Some(Arc::new(SubgraphSelector::SubgraphRequestHeader { @@ -329,12 +615,16 @@ impl InstrumentsConfig { inner: Mutex::new(CustomHistogramInner { increment: Increment::Custom(None), condition: Condition::True, - histogram: Some( - meter - .f64_histogram("http.client.response.body.size") - .with_unit(Unit::new("By")) - .with_description("Size of HTTP client response bodies.") - .init(), + histogram: Some(static_instruments + .get(HTTP_CLIENT_RESPONSE_BODY_SIZE_METRIC) + .expect( + "cannot get static instrument for subgraph; this should not happen", + ) + .as_histogram() + .cloned() + .expect( + "cannot convert instrument to histogram for subgraph; this should not happen", + ) ), attributes: Vec::with_capacity(nb_attributes), selector: Some(Arc::new(SubgraphSelector::SubgraphResponseHeader { @@ -351,7 +641,160 @@ impl InstrumentsConfig { http_client_request_duration, http_client_request_body_size, http_client_response_body_size, - custom: CustomInstruments::new(&self.subgraph.custom), + custom: CustomInstruments::new(&self.subgraph.custom, static_instruments), + } + } + + pub(crate) fn new_static_graphql_instruments(&self) -> HashMap { + let meter = metrics::meter_provider().meter(METER_NAME); + let mut static_instruments = HashMap::with_capacity(self.graphql.custom.len()); + if self.graphql.attributes.list_length.is_enabled() { + static_instruments.insert( + FIELD_LENGTH.to_string(), + StaticInstrument::Histogram( + meter + .f64_histogram(FIELD_LENGTH) + .with_description("Length of a selected field in the GraphQL response") + .init(), + ), + ); + } + + if self.graphql.attributes.field_execution.is_enabled() { + static_instruments.insert( + FIELD_EXECUTION.to_string(), + StaticInstrument::CounterF64( + meter + .f64_counter(FIELD_EXECUTION) + .with_description("Number of times a field is used.") + .init(), + ), + ); + } + + for (instrument_name, instrument) in &self.graphql.custom { + match instrument.ty { + InstrumentType::Counter => { + static_instruments.insert( + instrument_name.clone(), + StaticInstrument::CounterF64( + meter + .f64_counter(instrument_name.clone()) + .with_description(instrument.description.clone()) + .with_unit(Unit::new(instrument.unit.clone())) + .init(), + ), + ); + } + InstrumentType::Histogram => { + static_instruments.insert( + instrument_name.clone(), + StaticInstrument::Histogram( + meter + .f64_histogram(instrument_name.clone()) + .with_description(instrument.description.clone()) + .with_unit(Unit::new(instrument.unit.clone())) + .init(), + ), + ); + } + } + } + + static_instruments + } + + pub(crate) fn new_graphql_instruments( + &self, + static_instruments: Arc>, + ) -> GraphQLInstruments { + let meter = metrics::meter_provider().meter(METER_NAME); + GraphQLInstruments { + list_length: self.graphql.attributes.list_length.is_enabled().then(|| { + let mut nb_attributes = 0; + let selectors = match &self.graphql.attributes.list_length { + DefaultedStandardInstrument::Bool(_) | DefaultedStandardInstrument::Unset => { + None + } + DefaultedStandardInstrument::Extendable { attributes } => { + nb_attributes = attributes.custom.len(); + Some(attributes.clone()) + } + }; + CustomHistogram { + inner: Mutex::new(CustomHistogramInner { + increment: Increment::FieldCustom(None), + condition: Condition::True, + histogram: Some(meter.f64_histogram(FIELD_LENGTH).init()), + attributes: Vec::with_capacity(nb_attributes), + selector: Some(Arc::new(GraphQLSelector::ListLength { + list_length: ListLength::Value, + })), + selectors, + updated: false, + }), + } + }), + field_execution: self + .graphql + .attributes + .field_execution + .is_enabled() + .then(|| { + let mut nb_attributes = 0; + let selectors = match &self.graphql.attributes.field_execution { + DefaultedStandardInstrument::Bool(_) + | DefaultedStandardInstrument::Unset => None, + DefaultedStandardInstrument::Extendable { attributes } => { + nb_attributes = attributes.custom.len(); + Some(attributes.clone()) + } + }; + CustomCounter { + inner: Mutex::new(CustomCounterInner { + increment: Increment::FieldUnit, + condition: Condition::True, + counter: Some(meter.f64_counter(FIELD_EXECUTION).init()), + attributes: Vec::with_capacity(nb_attributes), + selector: None, + selectors, + incremented: false, + }), + } + }), + custom: CustomInstruments::new(&self.graphql.custom, static_instruments), + } + } +} + +pub(crate) enum StaticInstrument { + CounterF64(Counter), + UpDownCounterI64(UpDownCounter), + Histogram(Histogram), +} + +impl StaticInstrument { + pub(crate) fn as_counter_f64(&self) -> Option<&Counter> { + if let Self::CounterF64(v) = self { + Some(v) + } else { + None + } + } + + pub(crate) fn as_up_down_counter_i64(&self) -> Option<&UpDownCounter> { + if let Self::UpDownCounterI64(v) = self { + Some(v) + } else { + None + } + } + + pub(crate) fn as_histogram(&self) -> Option<&Histogram> { + if let Self::Histogram(v) = self { + Some(v) + } else { + None } } } @@ -824,10 +1267,10 @@ where { pub(crate) fn new( config: &HashMap>, + static_instruments: Arc>, ) -> Self { let mut counters = Vec::new(); let mut histograms = Vec::new(); - let meter = metrics::meter_provider().meter(METER_NAME); for (instrument_name, instrument) in config { match instrument.ty { @@ -857,25 +1300,32 @@ where } }, }; - let counter = CustomCounterInner { - increment, - condition: instrument.condition.clone(), - counter: Some( - meter - .f64_counter(instrument_name.clone()) - .with_description(instrument.description.clone()) - .with_unit(Unit::new(instrument.unit.clone())) - .init(), - ), - attributes: Vec::new(), - selector, - selectors: Some(instrument.attributes.clone()), - incremented: false, - }; - - counters.push(CustomCounter { - inner: Mutex::new(counter), - }) + match static_instruments + .get(instrument_name) + .expect( + "cannot get static instrument for supergraph; this should not happen", + ) + .as_counter_f64() + .cloned() + { + Some(counter) => { + let counter = CustomCounterInner { + increment, + condition: instrument.condition.clone(), + counter: Some(counter), + attributes: Vec::new(), + selector, + selectors: Some(instrument.attributes.clone()), + incremented: false, + }; + counters.push(CustomCounter { + inner: Mutex::new(counter), + }) + } + None => { + ::tracing::error!("cannot convert static instrument into a counter, this is an error; please fill an issue on GitHub"); + } + } } InstrumentType::Histogram => { let (selector, increment) = match (&instrument.value).into() { @@ -903,25 +1353,34 @@ where } }, }; - let histogram = CustomHistogramInner { - increment, - condition: instrument.condition.clone(), - histogram: Some( - meter - .f64_histogram(instrument_name.clone()) - .with_description(instrument.description.clone()) - .with_unit(Unit::new(instrument.unit.clone())) - .init(), - ), - attributes: Vec::new(), - selector, - selectors: Some(instrument.attributes.clone()), - updated: false, - }; - histograms.push(CustomHistogram { - inner: Mutex::new(histogram), - }) + match static_instruments + .get(instrument_name) + .expect( + "cannot get static instrument for supergraph; this should not happen", + ) + .as_histogram() + .cloned() + { + Some(histogram) => { + let histogram = CustomHistogramInner { + increment, + condition: instrument.condition.clone(), + histogram: Some(histogram), + attributes: Vec::new(), + selector, + selectors: Some(instrument.attributes.clone()), + updated: false, + }; + + histograms.push(CustomHistogram { + inner: Mutex::new(histogram), + }); + } + None => { + ::tracing::error!("cannot convert static instrument into a histogram, this is an error; please fill an issue on GitHub"); + } + } } } } @@ -2258,7 +2717,10 @@ mod tests { let mut router_instruments = None; let mut supergraph_instruments = None; let mut subgraph_instruments = None; - let graphql_instruments: GraphQLInstruments = (&config).into(); + let graphql_instruments: GraphQLInstruments = config + .new_graphql_instruments(Arc::new( + config.new_static_graphql_instruments(), + )); let context = Context::new(); for event in request { match event { @@ -2276,7 +2738,9 @@ mod tests { .body(body) .build() .unwrap(); - router_instruments = Some(config.new_router_instruments()); + router_instruments = Some(config.new_router_instruments( + Arc::new(config.new_static_router_instruments()), + )); router_instruments .as_mut() .expect("router instruments") @@ -2312,7 +2776,9 @@ mod tests { headers, } => { supergraph_instruments = - Some(config.new_supergraph_instruments()); + Some(config.new_supergraph_instruments(Arc::new( + config.new_static_supergraph_instruments(), + ))); let mut request = supergraph::Request::fake_builder() .context(context.clone()) @@ -2364,7 +2830,9 @@ mod tests { extensions, headers, } => { - subgraph_instruments = Some(config.new_subgraph_instruments()); + subgraph_instruments = Some(config.new_subgraph_instruments( + Arc::new(config.new_static_subgraph_instruments()), + )); let graphql_request = graphql::Request::fake_builder() .query(query) .and_operation_name(operation_name) @@ -2653,7 +3121,8 @@ mod tests { ) .unwrap(); - let router_instruments = config.new_router_instruments(); + let router_instruments = + config.new_router_instruments(Arc::new(config.new_static_router_instruments())); let router_req = RouterRequest::fake_builder() .header("conditional-custom", "X") .header("x-my-header-count", "55") @@ -2691,7 +3160,8 @@ mod tests { "acme.my_attribute" = "TEST" ); - let router_instruments = config.new_router_instruments(); + let router_instruments = + config.new_router_instruments(Arc::new(config.new_static_router_instruments())); let router_req = RouterRequest::fake_builder() .header("content-length", "35") .header("x-my-header-count", "5") @@ -2732,7 +3202,8 @@ mod tests { "acme.my_attribute" = "unknown" ); - let router_instruments = config.new_router_instruments(); + let router_instruments = + config.new_router_instruments(Arc::new(config.new_static_router_instruments())); let router_req = RouterRequest::fake_builder() .header("content-length", "35") .header("content-type", "application/graphql") @@ -2761,7 +3232,8 @@ mod tests { "http.response.status_code" = 400 ); - let router_instruments = config.new_router_instruments(); + let router_instruments = + config.new_router_instruments(Arc::new(config.new_static_router_instruments())); let router_req = RouterRequest::fake_builder() .header("content-length", "35") .header("content-type", "application/graphql") @@ -2899,7 +3371,10 @@ mod tests { ) .unwrap(); - let custom_instruments = SupergraphCustomInstruments::new(&config.supergraph.custom); + let custom_instruments = SupergraphCustomInstruments::new( + &config.supergraph.custom, + Arc::new(config.new_static_supergraph_instruments()), + ); let context = crate::context::Context::new(); let _ = context.insert(OPERATION_KIND, "query".to_string()).unwrap(); let context_with_error = crate::context::Context::new(); @@ -2964,7 +3439,10 @@ mod tests { ); assert_counter!("acme.request.on_graphql_data", 500.0, response.data = 500); - let custom_instruments = SupergraphCustomInstruments::new(&config.supergraph.custom); + let custom_instruments = SupergraphCustomInstruments::new( + &config.supergraph.custom, + Arc::new(config.new_static_supergraph_instruments()), + ); let supergraph_req = supergraph::Request::fake_builder() .header("content-length", "35") .header("x-my-header-count", "5") @@ -3018,7 +3496,10 @@ mod tests { ); assert_counter!("acme.request.on_graphql_data", 1000.0, response.data = 500); - let custom_instruments = SupergraphCustomInstruments::new(&config.supergraph.custom); + let custom_instruments = SupergraphCustomInstruments::new( + &config.supergraph.custom, + Arc::new(config.new_static_supergraph_instruments()), + ); let supergraph_req = supergraph::Request::fake_builder() .header("content-length", "35") .header("content-type", "application/graphql") diff --git a/apollo-router/src/plugins/telemetry/mod.rs b/apollo-router/src/plugins/telemetry/mod.rs index 14dd43407c..23f86b7289 100644 --- a/apollo-router/src/plugins/telemetry/mod.rs +++ b/apollo-router/src/plugins/telemetry/mod.rs @@ -9,6 +9,7 @@ use std::time::Instant; use ::tracing::info_span; use ::tracing::Span; use axum::headers::HeaderName; +use config_new::instruments::StaticInstrument; use config_new::Selectors; use dashmap::DashMap; use futures::future::ready; @@ -42,6 +43,7 @@ use opentelemetry::KeyValue; use opentelemetry_api::trace::TraceId; use opentelemetry_semantic_conventions::trace::HTTP_REQUEST_METHOD; use parking_lot::Mutex; +use parking_lot::RwLock; use rand::Rng; use router_bridge::planner::UsageReporting; use serde_json_bytes::json; @@ -197,7 +199,10 @@ pub(crate) struct Telemetry { apollo_metrics_sender: apollo_exporter::Sender, field_level_instrumentation_ratio: f64, sampling_filter_ratio: SamplerOption, - + pub(crate) graphql_custom_instruments: RwLock>>, + router_custom_instruments: RwLock>>, + supergraph_custom_instruments: RwLock>>, + subgraph_custom_instruments: RwLock>>, activation: Mutex, } @@ -285,6 +290,47 @@ impl Plugin for Telemetry { ::tracing::warn!("telemetry.instrumentation.spans.mode is currently set to 'deprecated', either explicitly or via defaulting. Set telemetry.instrumentation.spans.mode explicitly in your router.yaml to 'spec_compliant' for log and span attributes that follow OpenTelemetry semantic conventions. This option will be defaulted to 'spec_compliant' in a future release and eventually removed altogether"); } + let graphql_custom_instruments = if cfg!(test) { + RwLock::new(Arc::new( + config + .instrumentation + .instruments + .new_static_graphql_instruments(), + )) + } else { + RwLock::default() + }; + let router_custom_instruments = if cfg!(test) { + RwLock::new(Arc::new( + config + .instrumentation + .instruments + .new_static_router_instruments(), + )) + } else { + RwLock::default() + }; + let supergraph_custom_instruments = if cfg!(test) { + RwLock::new(Arc::new( + config + .instrumentation + .instruments + .new_static_supergraph_instruments(), + )) + } else { + RwLock::default() + }; + let subgraph_custom_instruments = if cfg!(test) { + RwLock::new(Arc::new( + config + .instrumentation + .instruments + .new_static_subgraph_instruments(), + )) + } else { + RwLock::default() + }; + Ok(Telemetry { custom_endpoints: metrics_builder.custom_endpoints, apollo_metrics_sender: metrics_builder.apollo_metrics_sender, @@ -302,6 +348,10 @@ impl Plugin for Telemetry { .map(FilterMeterProvider::public), is_active: false, }), + graphql_custom_instruments, + router_custom_instruments, + supergraph_custom_instruments, + subgraph_custom_instruments, sampling_filter_ratio, config: Arc::new(config), }) @@ -316,6 +366,7 @@ impl Plugin for Telemetry { matches!(config.instrumentation.spans.mode, SpanMode::Deprecated); let field_level_instrumentation_ratio = self.field_level_instrumentation_ratio; let metrics_sender = self.apollo_metrics_sender.clone(); + let static_router_instruments = self.router_custom_instruments.read().clone(); ServiceBuilder::new() .map_response(move |response: router::Response| { @@ -410,7 +461,7 @@ impl Plugin for Telemetry { let custom_instruments: RouterInstruments = config_request .instrumentation .instruments - .new_router_instruments(); + .new_router_instruments(static_router_instruments.clone()); custom_instruments.on_request(request); let custom_events: RouterEvents = @@ -537,6 +588,8 @@ impl Plugin for Telemetry { let config_map_res_first = config.clone(); let config_map_res = config.clone(); let field_level_instrumentation_ratio = self.field_level_instrumentation_ratio; + let static_supergraph_instruments = self.supergraph_custom_instruments.read().clone(); + let static_graphql_instruments = self.graphql_custom_instruments.read().clone(); ServiceBuilder::new() .instrument(move |supergraph_req: &SupergraphRequest| span_mode.create_supergraph( &config_instrument.apollo, @@ -601,11 +654,11 @@ impl Plugin for Telemetry { let custom_instruments = config .instrumentation .instruments - .new_supergraph_instruments(); + .new_supergraph_instruments(static_supergraph_instruments.clone()); custom_instruments.on_request(req); - let custom_graphql_instruments:GraphQLInstruments = (&config + let custom_graphql_instruments: GraphQLInstruments = config .instrumentation - .instruments).into(); + .instruments.new_graphql_instruments(static_graphql_instruments.clone()); custom_graphql_instruments.on_request(req); let supergraph_events = config.instrumentation.events.new_supergraph_events(); @@ -700,6 +753,7 @@ impl Plugin for Telemetry { let subgraph_metrics_conf_resp = subgraph_metrics_conf_req.clone(); let subgraph_name = ByteString::from(name); let name = name.to_owned(); + let static_subgraph_instruments = self.subgraph_custom_instruments.read().clone(); ServiceBuilder::new() .instrument(move |req: &SubgraphRequest| span_mode.create_subgraph(name.as_str(), req)) .map_request(move |req: SubgraphRequest| request_ftv1(req)) @@ -720,7 +774,7 @@ impl Plugin for Telemetry { let custom_instruments = config .instrumentation .instruments - .new_subgraph_instruments(); + .new_subgraph_instruments(static_subgraph_instruments.clone()); custom_instruments.on_request(sub_request); let custom_events = config.instrumentation.events.new_subgraph_events(); custom_events.on_request(sub_request); @@ -839,6 +893,31 @@ impl Telemetry { activation.reload_metrics(); + *self.graphql_custom_instruments.write() = Arc::new( + self.config + .instrumentation + .instruments + .new_static_graphql_instruments(), + ); + *self.router_custom_instruments.write() = Arc::new( + self.config + .instrumentation + .instruments + .new_static_router_instruments(), + ); + *self.supergraph_custom_instruments.write() = Arc::new( + self.config + .instrumentation + .instruments + .new_static_supergraph_instruments(), + ); + *self.subgraph_custom_instruments.write() = Arc::new( + self.config + .instrumentation + .instruments + .new_static_subgraph_instruments(), + ); + reload_fmt(create_fmt_layer(&self.config)); activation.is_active = true; } diff --git a/apollo-router/src/plugins/test.rs b/apollo-router/src/plugins/test.rs index c31ae9acc7..cc8240fcf3 100644 --- a/apollo-router/src/plugins/test.rs +++ b/apollo-router/src/plugins/test.rs @@ -62,7 +62,7 @@ use crate::Notify; /// /// pub(crate) struct PluginTestHarness { - plugin: Box, + pub(crate) plugin: Box, phantom: std::marker::PhantomData, } #[buildstructor::buildstructor] From fd8631f413d1785f0870566f05effdc0b3439f92 Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Wed, 17 Jul 2024 12:04:04 +0200 Subject: [PATCH 02/10] add support for cache instruments Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- .../plugins/telemetry/config_new/cache/mod.rs | 54 +---------- .../graphql/field.execution/metrics.snap | 1 + .../graphql/field.length/metrics.snap | 1 + .../telemetry/config_new/instruments.rs | 96 ++++++++++++++++++- apollo-router/src/plugins/telemetry/mod.rs | 25 ++++- 5 files changed, 118 insertions(+), 59 deletions(-) diff --git a/apollo-router/src/plugins/telemetry/config_new/cache/mod.rs b/apollo-router/src/plugins/telemetry/config_new/cache/mod.rs index f1d01f2393..adc172911b 100644 --- a/apollo-router/src/plugins/telemetry/config_new/cache/mod.rs +++ b/apollo-router/src/plugins/telemetry/config_new/cache/mod.rs @@ -1,29 +1,19 @@ use std::sync::Arc; use attributes::CacheAttributes; -use opentelemetry::metrics::MeterProvider; -use opentelemetry::metrics::Unit; use opentelemetry::Key; use opentelemetry::KeyValue; -use parking_lot::Mutex; use schemars::JsonSchema; use serde::Deserialize; use tower::BoxError; use super::instruments::CustomCounter; -use super::instruments::CustomCounterInner; -use super::instruments::Increment; -use super::instruments::InstrumentsConfig; -use super::instruments::METER_NAME; -use super::selectors::CacheKind; use super::selectors::SubgraphSelector; -use crate::metrics; use crate::plugins::cache::entity::CacheHitMiss; use crate::plugins::cache::entity::CacheSubgraph; use crate::plugins::cache::metrics::CacheMetricContextKey; use crate::plugins::telemetry::config::AttributeValue; use crate::plugins::telemetry::config_new::attributes::DefaultAttributeRequirementLevel; -use crate::plugins::telemetry::config_new::conditions::Condition; use crate::plugins::telemetry::config_new::extendable::Extendable; use crate::plugins::telemetry::config_new::instruments::DefaultedStandardInstrument; use crate::plugins::telemetry::config_new::instruments::Instrumented; @@ -33,7 +23,7 @@ use crate::services::subgraph; pub(crate) mod attributes; -static CACHE_METRIC: &str = "apollo.router.operations.entity.cache"; +pub(crate) const CACHE_METRIC: &str = "apollo.router.operations.entity.cache"; const ENTITY_TYPE: Key = Key::from_static_str("entity.type"); const CACHE_HIT: Key = Key::from_static_str("cache.hit"); @@ -63,48 +53,6 @@ pub(crate) struct CacheInstruments { >, } -impl From<&InstrumentsConfig> for CacheInstruments { - fn from(value: &InstrumentsConfig) -> Self { - let meter = metrics::meter_provider().meter(METER_NAME); - CacheInstruments { - cache_hit: value.cache.attributes.cache.is_enabled().then(|| { - let mut nb_attributes = 0; - let selectors = match &value.cache.attributes.cache { - DefaultedStandardInstrument::Bool(_) | DefaultedStandardInstrument::Unset => { - None - } - DefaultedStandardInstrument::Extendable { attributes } => { - nb_attributes = attributes.custom.len(); - Some(attributes.clone()) - } - }; - CustomCounter { - inner: Mutex::new(CustomCounterInner { - increment: Increment::Custom(None), - condition: Condition::True, - counter: Some( - meter - .f64_counter(CACHE_METRIC) - .with_unit(Unit::new("ops")) - .with_description( - "Entity cache hit/miss operations at the subgraph level", - ) - .init(), - ), - attributes: Vec::with_capacity(nb_attributes), - selector: Some(Arc::new(SubgraphSelector::Cache { - cache: CacheKind::Hit, - entity_type: None, - })), - selectors, - incremented: false, - }), - } - }), - } - } -} - impl Instrumented for CacheInstruments { type Request = subgraph::Request; type Response = subgraph::Response; diff --git a/apollo-router/src/plugins/telemetry/config_new/fixtures/graphql/field.execution/metrics.snap b/apollo-router/src/plugins/telemetry/config_new/fixtures/graphql/field.execution/metrics.snap index d1214d2f8f..0b3e04d9ef 100644 --- a/apollo-router/src/plugins/telemetry/config_new/fixtures/graphql/field.execution/metrics.snap +++ b/apollo-router/src/plugins/telemetry/config_new/fixtures/graphql/field.execution/metrics.snap @@ -10,6 +10,7 @@ info: field.execution: true --- - name: graphql.field.execution + description: Number of times a field is used. data: datapoints: - value: 1 diff --git a/apollo-router/src/plugins/telemetry/config_new/fixtures/graphql/field.length/metrics.snap b/apollo-router/src/plugins/telemetry/config_new/fixtures/graphql/field.length/metrics.snap index cce830a861..d3e0270014 100644 --- a/apollo-router/src/plugins/telemetry/config_new/fixtures/graphql/field.length/metrics.snap +++ b/apollo-router/src/plugins/telemetry/config_new/fixtures/graphql/field.length/metrics.snap @@ -11,6 +11,7 @@ info: list.length: true --- - name: graphql.field.list.length + description: Length of a selected field in the GraphQL response data: datapoints: - sum: 3 diff --git a/apollo-router/src/plugins/telemetry/config_new/instruments.rs b/apollo-router/src/plugins/telemetry/config_new/instruments.rs index bf4c9c802f..aa8adbe50f 100644 --- a/apollo-router/src/plugins/telemetry/config_new/instruments.rs +++ b/apollo-router/src/plugins/telemetry/config_new/instruments.rs @@ -22,11 +22,14 @@ use tower::BoxError; use super::attributes::HttpServerAttributes; use super::cache::attributes::CacheAttributes; +use super::cache::CacheInstruments; use super::cache::CacheInstrumentsConfig; +use super::cache::CACHE_METRIC; use super::graphql::selectors::ListLength; use super::graphql::GraphQLInstruments; use super::graphql::FIELD_EXECUTION; use super::graphql::FIELD_LENGTH; +use super::selectors::CacheKind; use super::DefaultForLevel; use super::Selector; use crate::metrics; @@ -715,7 +718,6 @@ impl InstrumentsConfig { &self, static_instruments: Arc>, ) -> GraphQLInstruments { - let meter = metrics::meter_provider().meter(METER_NAME); GraphQLInstruments { list_length: self.graphql.attributes.list_length.is_enabled().then(|| { let mut nb_attributes = 0; @@ -732,7 +734,17 @@ impl InstrumentsConfig { inner: Mutex::new(CustomHistogramInner { increment: Increment::FieldCustom(None), condition: Condition::True, - histogram: Some(meter.f64_histogram(FIELD_LENGTH).init()), + histogram: Some(static_instruments + .get(FIELD_LENGTH) + .expect( + "cannot get static instrument for graphql; this should not happen", + ) + .as_histogram() + .cloned() + .expect( + "cannot convert instrument to counter for graphql; this should not happen", + ) + ), attributes: Vec::with_capacity(nb_attributes), selector: Some(Arc::new(GraphQLSelector::ListLength { list_length: ListLength::Value, @@ -761,7 +773,17 @@ impl InstrumentsConfig { inner: Mutex::new(CustomCounterInner { increment: Increment::FieldUnit, condition: Condition::True, - counter: Some(meter.f64_counter(FIELD_EXECUTION).init()), + counter: Some(static_instruments + .get(FIELD_EXECUTION) + .expect( + "cannot get static instrument for graphql; this should not happen", + ) + .as_counter_f64() + .cloned() + .expect( + "cannot convert instrument to counter for graphql; this should not happen", + ) + ), attributes: Vec::with_capacity(nb_attributes), selector: None, selectors, @@ -772,8 +794,72 @@ impl InstrumentsConfig { custom: CustomInstruments::new(&self.graphql.custom, static_instruments), } } + + pub(crate) fn new_static_cache_instruments(&self) -> HashMap { + let meter = metrics::meter_provider().meter(METER_NAME); + let mut static_instruments: HashMap = HashMap::new(); + if self.cache.attributes.cache.is_enabled() { + static_instruments.insert( + CACHE_METRIC.to_string(), + StaticInstrument::CounterF64( + meter + .f64_counter(CACHE_METRIC) + .with_unit(Unit::new("ops")) + .with_description("Entity cache hit/miss operations at the subgraph level") + .init(), + ), + ); + } + + static_instruments + } + + pub(crate) fn new_cache_instruments( + &self, + static_instruments: Arc>, + ) -> CacheInstruments { + CacheInstruments { + cache_hit: self.cache.attributes.cache.is_enabled().then(|| { + let mut nb_attributes = 0; + let selectors = match &self.cache.attributes.cache { + DefaultedStandardInstrument::Bool(_) | DefaultedStandardInstrument::Unset => { + None + } + DefaultedStandardInstrument::Extendable { attributes } => { + nb_attributes = attributes.custom.len(); + Some(attributes.clone()) + } + }; + CustomCounter { + inner: Mutex::new(CustomCounterInner { + increment: Increment::Custom(None), + condition: Condition::True, + counter: Some(dbg!(&static_instruments) + .get(CACHE_METRIC) + .expect( + "cannot get static instrument for cache; this should not happen", + ) + .as_counter_f64() + .cloned() + .expect( + "cannot convert instrument to counter for cache; this should not happen", + ) + ), + attributes: Vec::with_capacity(nb_attributes), + selector: Some(Arc::new(SubgraphSelector::Cache { + cache: CacheKind::Hit, + entity_type: None, + })), + selectors, + incremented: false, + }), + } + }), + } + } } +#[derive(Debug)] pub(crate) enum StaticInstrument { CounterF64(Counter), UpDownCounterI64(UpDownCounter), @@ -2873,7 +2959,9 @@ mod tests { subgraph_instruments = Some(config.new_subgraph_instruments( Arc::new(config.new_static_subgraph_instruments()), )); - cache_instruments = Some((&config).into()); + cache_instruments = Some(config.new_cache_instruments( + Arc::new(config.new_static_cache_instruments()), + )); let graphql_request = graphql::Request::fake_builder() .query(query) .and_operation_name(operation_name) diff --git a/apollo-router/src/plugins/telemetry/mod.rs b/apollo-router/src/plugins/telemetry/mod.rs index 2921f41f81..c69e9cc525 100644 --- a/apollo-router/src/plugins/telemetry/mod.rs +++ b/apollo-router/src/plugins/telemetry/mod.rs @@ -204,6 +204,7 @@ pub(crate) struct Telemetry { router_custom_instruments: RwLock>>, supergraph_custom_instruments: RwLock>>, subgraph_custom_instruments: RwLock>>, + cache_custom_instruments: RwLock>>, activation: Mutex, } @@ -320,6 +321,16 @@ impl Plugin for Telemetry { } else { RwLock::default() }; + let cache_custom_instruments = if cfg!(test) { + RwLock::new(Arc::new( + config + .instrumentation + .instruments + .new_static_cache_instruments(), + )) + } else { + RwLock::default() + }; Ok(Telemetry { custom_endpoints: metrics_builder.custom_endpoints, @@ -342,6 +353,7 @@ impl Plugin for Telemetry { router_custom_instruments, supergraph_custom_instruments, subgraph_custom_instruments, + cache_custom_instruments, sampling_filter_ratio, config: Arc::new(config), }) @@ -744,6 +756,7 @@ impl Plugin for Telemetry { let subgraph_name = ByteString::from(name); let name = name.to_owned(); let static_subgraph_instruments = self.subgraph_custom_instruments.read().clone(); + let static_cache_instruments = self.cache_custom_instruments.read().clone(); ServiceBuilder::new() .instrument(move |req: &SubgraphRequest| span_mode.create_subgraph(name.as_str(), req)) .map_request(move |req: SubgraphRequest| request_ftv1(req)) @@ -769,8 +782,10 @@ impl Plugin for Telemetry { let custom_events = config.instrumentation.events.new_subgraph_events(); custom_events.on_request(sub_request); - let custom_cache_instruments: CacheInstruments = - (&config.instrumentation.instruments).into(); + let custom_cache_instruments: CacheInstruments = config + .instrumentation + .instruments + .new_cache_instruments(static_cache_instruments.clone()); custom_cache_instruments.on_request(sub_request); ( @@ -921,6 +936,12 @@ impl Telemetry { .instruments .new_static_subgraph_instruments(), ); + *self.cache_custom_instruments.write() = Arc::new( + self.config + .instrumentation + .instruments + .new_static_cache_instruments(), + ); reload_fmt(create_fmt_layer(&self.config)); activation.is_active = true; From d03608c3ae157227d021c89d97537a4cb80e48f9 Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Wed, 17 Jul 2024 17:40:08 +0200 Subject: [PATCH 03/10] delete dbg Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- apollo-router/src/plugins/telemetry/config_new/instruments.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apollo-router/src/plugins/telemetry/config_new/instruments.rs b/apollo-router/src/plugins/telemetry/config_new/instruments.rs index aa8adbe50f..7b872aa802 100644 --- a/apollo-router/src/plugins/telemetry/config_new/instruments.rs +++ b/apollo-router/src/plugins/telemetry/config_new/instruments.rs @@ -834,7 +834,7 @@ impl InstrumentsConfig { inner: Mutex::new(CustomCounterInner { increment: Increment::Custom(None), condition: Condition::True, - counter: Some(dbg!(&static_instruments) + counter: Some(static_instruments .get(CACHE_METRIC) .expect( "cannot get static instrument for cache; this should not happen", From d4bc6b8101bf75eab6dd6dfa59020d5b319bc164 Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Thu, 18 Jul 2024 11:17:00 +0200 Subject: [PATCH 04/10] check with oncecell Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- apollo-router/src/plugins/telemetry/mod.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/apollo-router/src/plugins/telemetry/mod.rs b/apollo-router/src/plugins/telemetry/mod.rs index c69e9cc525..a045a3c155 100644 --- a/apollo-router/src/plugins/telemetry/mod.rs +++ b/apollo-router/src/plugins/telemetry/mod.rs @@ -203,7 +203,7 @@ pub(crate) struct Telemetry { pub(crate) graphql_custom_instruments: RwLock>>, router_custom_instruments: RwLock>>, supergraph_custom_instruments: RwLock>>, - subgraph_custom_instruments: RwLock>>, + subgraph_custom_instruments: OnceCell>>, cache_custom_instruments: RwLock>>, activation: Mutex, } @@ -312,14 +312,14 @@ impl Plugin for Telemetry { RwLock::default() }; let subgraph_custom_instruments = if cfg!(test) { - RwLock::new(Arc::new( + OnceCell::with_value(Arc::new( config .instrumentation .instruments .new_static_subgraph_instruments(), )) } else { - RwLock::default() + OnceCell::new() }; let cache_custom_instruments = if cfg!(test) { RwLock::new(Arc::new( @@ -755,7 +755,11 @@ impl Plugin for Telemetry { let subgraph_metrics_conf_resp = subgraph_metrics_conf_req.clone(); let subgraph_name = ByteString::from(name); let name = name.to_owned(); - let static_subgraph_instruments = self.subgraph_custom_instruments.read().clone(); + let static_subgraph_instruments = self + .subgraph_custom_instruments + .get() + .expect("must be set in validate method") + .clone(); let static_cache_instruments = self.cache_custom_instruments.read().clone(); ServiceBuilder::new() .instrument(move |req: &SubgraphRequest| span_mode.create_subgraph(name.as_str(), req)) @@ -930,12 +934,12 @@ impl Telemetry { .instruments .new_static_supergraph_instruments(), ); - *self.subgraph_custom_instruments.write() = Arc::new( + let _ = self.subgraph_custom_instruments.set(Arc::new( self.config .instrumentation .instruments .new_static_subgraph_instruments(), - ); + )); *self.cache_custom_instruments.write() = Arc::new( self.config .instrumentation From fda645f28f4ce096770f5fbf2cf0e7f16a95d15a Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Mon, 22 Jul 2024 10:14:35 +0200 Subject: [PATCH 05/10] use once_cell Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- apollo-router/src/plugins/telemetry/mod.rs | 65 +++++++++++++--------- 1 file changed, 40 insertions(+), 25 deletions(-) diff --git a/apollo-router/src/plugins/telemetry/mod.rs b/apollo-router/src/plugins/telemetry/mod.rs index a045a3c155..c3963ffd2f 100644 --- a/apollo-router/src/plugins/telemetry/mod.rs +++ b/apollo-router/src/plugins/telemetry/mod.rs @@ -44,7 +44,6 @@ use opentelemetry::KeyValue; use opentelemetry_api::trace::TraceId; use opentelemetry_semantic_conventions::trace::HTTP_REQUEST_METHOD; use parking_lot::Mutex; -use parking_lot::RwLock; use rand::Rng; use router_bridge::planner::UsageReporting; use serde_json_bytes::json; @@ -200,11 +199,11 @@ pub(crate) struct Telemetry { apollo_metrics_sender: apollo_exporter::Sender, field_level_instrumentation_ratio: f64, sampling_filter_ratio: SamplerOption, - pub(crate) graphql_custom_instruments: RwLock>>, - router_custom_instruments: RwLock>>, - supergraph_custom_instruments: RwLock>>, + pub(crate) graphql_custom_instruments: OnceCell>>, + router_custom_instruments: OnceCell>>, + supergraph_custom_instruments: OnceCell>>, subgraph_custom_instruments: OnceCell>>, - cache_custom_instruments: RwLock>>, + cache_custom_instruments: OnceCell>>, activation: Mutex, } @@ -282,34 +281,34 @@ impl Plugin for Telemetry { } let graphql_custom_instruments = if cfg!(test) { - RwLock::new(Arc::new( + OnceCell::with_value(Arc::new( config .instrumentation .instruments .new_static_graphql_instruments(), )) } else { - RwLock::default() + OnceCell::new() }; let router_custom_instruments = if cfg!(test) { - RwLock::new(Arc::new( + OnceCell::with_value(Arc::new( config .instrumentation .instruments .new_static_router_instruments(), )) } else { - RwLock::default() + OnceCell::new() }; let supergraph_custom_instruments = if cfg!(test) { - RwLock::new(Arc::new( + OnceCell::with_value(Arc::new( config .instrumentation .instruments .new_static_supergraph_instruments(), )) } else { - RwLock::default() + OnceCell::new() }; let subgraph_custom_instruments = if cfg!(test) { OnceCell::with_value(Arc::new( @@ -322,14 +321,14 @@ impl Plugin for Telemetry { OnceCell::new() }; let cache_custom_instruments = if cfg!(test) { - RwLock::new(Arc::new( + OnceCell::with_value(Arc::new( config .instrumentation .instruments .new_static_cache_instruments(), )) } else { - RwLock::default() + OnceCell::new() }; Ok(Telemetry { @@ -368,7 +367,11 @@ impl Plugin for Telemetry { matches!(config.instrumentation.spans.mode, SpanMode::Deprecated); let field_level_instrumentation_ratio = self.field_level_instrumentation_ratio; let metrics_sender = self.apollo_metrics_sender.clone(); - let static_router_instruments = self.router_custom_instruments.read().clone(); + let static_router_instruments = self + .router_custom_instruments + .get() + .expect("must be set in validate method") + .clone(); ServiceBuilder::new() .map_response(move |response: router::Response| { @@ -590,8 +593,16 @@ impl Plugin for Telemetry { let config_map_res_first = config.clone(); let config_map_res = config.clone(); let field_level_instrumentation_ratio = self.field_level_instrumentation_ratio; - let static_supergraph_instruments = self.supergraph_custom_instruments.read().clone(); - let static_graphql_instruments = self.graphql_custom_instruments.read().clone(); + let static_supergraph_instruments = self + .supergraph_custom_instruments + .get() + .expect("must be set in validate method") + .clone(); + let static_graphql_instruments = self + .graphql_custom_instruments + .get() + .expect("must be set in validate method") + .clone(); ServiceBuilder::new() .instrument(move |supergraph_req: &SupergraphRequest| span_mode.create_supergraph( &config_instrument.apollo, @@ -760,7 +771,11 @@ impl Plugin for Telemetry { .get() .expect("must be set in validate method") .clone(); - let static_cache_instruments = self.cache_custom_instruments.read().clone(); + let static_cache_instruments = self + .cache_custom_instruments + .get() + .expect("must be set in validate method") + .clone(); ServiceBuilder::new() .instrument(move |req: &SubgraphRequest| span_mode.create_subgraph(name.as_str(), req)) .map_request(move |req: SubgraphRequest| request_ftv1(req)) @@ -916,36 +931,36 @@ impl Telemetry { activation.reload_metrics(); - *self.graphql_custom_instruments.write() = Arc::new( + let _ = self.graphql_custom_instruments.set(Arc::new( self.config .instrumentation .instruments .new_static_graphql_instruments(), - ); - *self.router_custom_instruments.write() = Arc::new( + )); + let _ = self.router_custom_instruments.set(Arc::new( self.config .instrumentation .instruments .new_static_router_instruments(), - ); - *self.supergraph_custom_instruments.write() = Arc::new( + )); + let _ = self.supergraph_custom_instruments.set(Arc::new( self.config .instrumentation .instruments .new_static_supergraph_instruments(), - ); + )); let _ = self.subgraph_custom_instruments.set(Arc::new( self.config .instrumentation .instruments .new_static_subgraph_instruments(), )); - *self.cache_custom_instruments.write() = Arc::new( + let _ = self.cache_custom_instruments.set(Arc::new( self.config .instrumentation .instruments .new_static_cache_instruments(), - ); + )); reload_fmt(create_fmt_layer(&self.config)); activation.is_active = true; From 5e36626dbe959bbbffbc541bb28dbde327b3d3b1 Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Mon, 22 Jul 2024 10:35:55 +0200 Subject: [PATCH 06/10] Revert "use once_cell" This reverts commit fda645f28f4ce096770f5fbf2cf0e7f16a95d15a. --- apollo-router/src/plugins/telemetry/mod.rs | 65 +++++++++------------- 1 file changed, 25 insertions(+), 40 deletions(-) diff --git a/apollo-router/src/plugins/telemetry/mod.rs b/apollo-router/src/plugins/telemetry/mod.rs index c3963ffd2f..a045a3c155 100644 --- a/apollo-router/src/plugins/telemetry/mod.rs +++ b/apollo-router/src/plugins/telemetry/mod.rs @@ -44,6 +44,7 @@ use opentelemetry::KeyValue; use opentelemetry_api::trace::TraceId; use opentelemetry_semantic_conventions::trace::HTTP_REQUEST_METHOD; use parking_lot::Mutex; +use parking_lot::RwLock; use rand::Rng; use router_bridge::planner::UsageReporting; use serde_json_bytes::json; @@ -199,11 +200,11 @@ pub(crate) struct Telemetry { apollo_metrics_sender: apollo_exporter::Sender, field_level_instrumentation_ratio: f64, sampling_filter_ratio: SamplerOption, - pub(crate) graphql_custom_instruments: OnceCell>>, - router_custom_instruments: OnceCell>>, - supergraph_custom_instruments: OnceCell>>, + pub(crate) graphql_custom_instruments: RwLock>>, + router_custom_instruments: RwLock>>, + supergraph_custom_instruments: RwLock>>, subgraph_custom_instruments: OnceCell>>, - cache_custom_instruments: OnceCell>>, + cache_custom_instruments: RwLock>>, activation: Mutex, } @@ -281,34 +282,34 @@ impl Plugin for Telemetry { } let graphql_custom_instruments = if cfg!(test) { - OnceCell::with_value(Arc::new( + RwLock::new(Arc::new( config .instrumentation .instruments .new_static_graphql_instruments(), )) } else { - OnceCell::new() + RwLock::default() }; let router_custom_instruments = if cfg!(test) { - OnceCell::with_value(Arc::new( + RwLock::new(Arc::new( config .instrumentation .instruments .new_static_router_instruments(), )) } else { - OnceCell::new() + RwLock::default() }; let supergraph_custom_instruments = if cfg!(test) { - OnceCell::with_value(Arc::new( + RwLock::new(Arc::new( config .instrumentation .instruments .new_static_supergraph_instruments(), )) } else { - OnceCell::new() + RwLock::default() }; let subgraph_custom_instruments = if cfg!(test) { OnceCell::with_value(Arc::new( @@ -321,14 +322,14 @@ impl Plugin for Telemetry { OnceCell::new() }; let cache_custom_instruments = if cfg!(test) { - OnceCell::with_value(Arc::new( + RwLock::new(Arc::new( config .instrumentation .instruments .new_static_cache_instruments(), )) } else { - OnceCell::new() + RwLock::default() }; Ok(Telemetry { @@ -367,11 +368,7 @@ impl Plugin for Telemetry { matches!(config.instrumentation.spans.mode, SpanMode::Deprecated); let field_level_instrumentation_ratio = self.field_level_instrumentation_ratio; let metrics_sender = self.apollo_metrics_sender.clone(); - let static_router_instruments = self - .router_custom_instruments - .get() - .expect("must be set in validate method") - .clone(); + let static_router_instruments = self.router_custom_instruments.read().clone(); ServiceBuilder::new() .map_response(move |response: router::Response| { @@ -593,16 +590,8 @@ impl Plugin for Telemetry { let config_map_res_first = config.clone(); let config_map_res = config.clone(); let field_level_instrumentation_ratio = self.field_level_instrumentation_ratio; - let static_supergraph_instruments = self - .supergraph_custom_instruments - .get() - .expect("must be set in validate method") - .clone(); - let static_graphql_instruments = self - .graphql_custom_instruments - .get() - .expect("must be set in validate method") - .clone(); + let static_supergraph_instruments = self.supergraph_custom_instruments.read().clone(); + let static_graphql_instruments = self.graphql_custom_instruments.read().clone(); ServiceBuilder::new() .instrument(move |supergraph_req: &SupergraphRequest| span_mode.create_supergraph( &config_instrument.apollo, @@ -771,11 +760,7 @@ impl Plugin for Telemetry { .get() .expect("must be set in validate method") .clone(); - let static_cache_instruments = self - .cache_custom_instruments - .get() - .expect("must be set in validate method") - .clone(); + let static_cache_instruments = self.cache_custom_instruments.read().clone(); ServiceBuilder::new() .instrument(move |req: &SubgraphRequest| span_mode.create_subgraph(name.as_str(), req)) .map_request(move |req: SubgraphRequest| request_ftv1(req)) @@ -931,36 +916,36 @@ impl Telemetry { activation.reload_metrics(); - let _ = self.graphql_custom_instruments.set(Arc::new( + *self.graphql_custom_instruments.write() = Arc::new( self.config .instrumentation .instruments .new_static_graphql_instruments(), - )); - let _ = self.router_custom_instruments.set(Arc::new( + ); + *self.router_custom_instruments.write() = Arc::new( self.config .instrumentation .instruments .new_static_router_instruments(), - )); - let _ = self.supergraph_custom_instruments.set(Arc::new( + ); + *self.supergraph_custom_instruments.write() = Arc::new( self.config .instrumentation .instruments .new_static_supergraph_instruments(), - )); + ); let _ = self.subgraph_custom_instruments.set(Arc::new( self.config .instrumentation .instruments .new_static_subgraph_instruments(), )); - let _ = self.cache_custom_instruments.set(Arc::new( + *self.cache_custom_instruments.write() = Arc::new( self.config .instrumentation .instruments .new_static_cache_instruments(), - )); + ); reload_fmt(create_fmt_layer(&self.config)); activation.is_active = true; From cb0cd21b5cd40300272564103fc58983f7735e53 Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Mon, 22 Jul 2024 10:35:58 +0200 Subject: [PATCH 07/10] Revert "check with oncecell" This reverts commit d4bc6b8101bf75eab6dd6dfa59020d5b319bc164. --- apollo-router/src/plugins/telemetry/mod.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/apollo-router/src/plugins/telemetry/mod.rs b/apollo-router/src/plugins/telemetry/mod.rs index a045a3c155..c69e9cc525 100644 --- a/apollo-router/src/plugins/telemetry/mod.rs +++ b/apollo-router/src/plugins/telemetry/mod.rs @@ -203,7 +203,7 @@ pub(crate) struct Telemetry { pub(crate) graphql_custom_instruments: RwLock>>, router_custom_instruments: RwLock>>, supergraph_custom_instruments: RwLock>>, - subgraph_custom_instruments: OnceCell>>, + subgraph_custom_instruments: RwLock>>, cache_custom_instruments: RwLock>>, activation: Mutex, } @@ -312,14 +312,14 @@ impl Plugin for Telemetry { RwLock::default() }; let subgraph_custom_instruments = if cfg!(test) { - OnceCell::with_value(Arc::new( + RwLock::new(Arc::new( config .instrumentation .instruments .new_static_subgraph_instruments(), )) } else { - OnceCell::new() + RwLock::default() }; let cache_custom_instruments = if cfg!(test) { RwLock::new(Arc::new( @@ -755,11 +755,7 @@ impl Plugin for Telemetry { let subgraph_metrics_conf_resp = subgraph_metrics_conf_req.clone(); let subgraph_name = ByteString::from(name); let name = name.to_owned(); - let static_subgraph_instruments = self - .subgraph_custom_instruments - .get() - .expect("must be set in validate method") - .clone(); + let static_subgraph_instruments = self.subgraph_custom_instruments.read().clone(); let static_cache_instruments = self.cache_custom_instruments.read().clone(); ServiceBuilder::new() .instrument(move |req: &SubgraphRequest| span_mode.create_subgraph(name.as_str(), req)) @@ -934,12 +930,12 @@ impl Telemetry { .instruments .new_static_supergraph_instruments(), ); - let _ = self.subgraph_custom_instruments.set(Arc::new( + *self.subgraph_custom_instruments.write() = Arc::new( self.config .instrumentation .instruments .new_static_subgraph_instruments(), - )); + ); *self.cache_custom_instruments.write() = Arc::new( self.config .instrumentation From 376d32483644f2708de9b09f5a52fa659d922cff Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Mon, 22 Jul 2024 10:37:30 +0200 Subject: [PATCH 08/10] delete useless change Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- apollo-router/src/plugins/test.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apollo-router/src/plugins/test.rs b/apollo-router/src/plugins/test.rs index cc8240fcf3..c31ae9acc7 100644 --- a/apollo-router/src/plugins/test.rs +++ b/apollo-router/src/plugins/test.rs @@ -62,7 +62,7 @@ use crate::Notify; /// /// pub(crate) struct PluginTestHarness { - pub(crate) plugin: Box, + plugin: Box, phantom: std::marker::PhantomData, } #[buildstructor::buildstructor] From c9e911e41604712bee5ba1a4d553daf291867675 Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Mon, 22 Jul 2024 10:40:31 +0200 Subject: [PATCH 09/10] changelog Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- .changesets/maint_bnjjj_improve_perf_custom_telemetry.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changesets/maint_bnjjj_improve_perf_custom_telemetry.md diff --git a/.changesets/maint_bnjjj_improve_perf_custom_telemetry.md b/.changesets/maint_bnjjj_improve_perf_custom_telemetry.md new file mode 100644 index 0000000000..cc27c908d5 --- /dev/null +++ b/.changesets/maint_bnjjj_improve_perf_custom_telemetry.md @@ -0,0 +1,5 @@ +### Improve performance, don't re-create meter and instruments on every calls in Telemetry ([PR #5629](https://github.com/apollographql/router/pull/5629)) + +The creation of otel instruments using a regex is no longer part of the hot path. Now we create these instruments when starting the telemetry plugin and not in every serives. + +By [@bnjjj](https://github.com/bnjjj) in https://github.com/apollographql/router/pull/5629 \ No newline at end of file From c4a8b14787030df5190bcecc59b4d53fdeb99dfb Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Tue, 23 Jul 2024 10:58:41 +0200 Subject: [PATCH 10/10] refactor Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- .../telemetry/config_new/instruments.rs | 34 ++--- apollo-router/src/plugins/telemetry/mod.rs | 129 ++++++------------ 2 files changed, 61 insertions(+), 102 deletions(-) diff --git a/apollo-router/src/plugins/telemetry/config_new/instruments.rs b/apollo-router/src/plugins/telemetry/config_new/instruments.rs index 7b872aa802..3b112a6f2d 100644 --- a/apollo-router/src/plugins/telemetry/config_new/instruments.rs +++ b/apollo-router/src/plugins/telemetry/config_new/instruments.rs @@ -116,7 +116,7 @@ impl InstrumentsConfig { .defaults_for_levels(self.default_requirement_level, TelemetryDataKind::Metrics); } - pub(crate) fn new_static_router_instruments(&self) -> HashMap { + pub(crate) fn new_builtin_router_instruments(&self) -> HashMap { let meter = metrics::meter_provider().meter(METER_NAME); let mut static_instruments = HashMap::with_capacity(self.router.custom.len()); @@ -383,7 +383,7 @@ impl InstrumentsConfig { } } - pub(crate) fn new_static_supergraph_instruments(&self) -> HashMap { + pub(crate) fn new_builtin_supergraph_instruments(&self) -> HashMap { let meter = metrics::meter_provider().meter(METER_NAME); let mut static_instruments = HashMap::with_capacity(self.supergraph.custom.len()); @@ -434,7 +434,7 @@ impl InstrumentsConfig { } } - pub(crate) fn new_static_subgraph_instruments(&self) -> HashMap { + pub(crate) fn new_builtin_subgraph_instruments(&self) -> HashMap { let meter = metrics::meter_provider().meter(METER_NAME); let mut static_instruments = HashMap::with_capacity(self.subgraph.custom.len()); @@ -655,7 +655,7 @@ impl InstrumentsConfig { } } - pub(crate) fn new_static_graphql_instruments(&self) -> HashMap { + pub(crate) fn new_builtin_graphql_instruments(&self) -> HashMap { let meter = metrics::meter_provider().meter(METER_NAME); let mut static_instruments = HashMap::with_capacity(self.graphql.custom.len()); if self.graphql.attributes.list_length.is_enabled() { @@ -795,7 +795,7 @@ impl InstrumentsConfig { } } - pub(crate) fn new_static_cache_instruments(&self) -> HashMap { + pub(crate) fn new_builtin_cache_instruments(&self) -> HashMap { let meter = metrics::meter_provider().meter(METER_NAME); let mut static_instruments: HashMap = HashMap::new(); if self.cache.attributes.cache.is_enabled() { @@ -2845,7 +2845,7 @@ mod tests { let mut cache_instruments: Option = None; let graphql_instruments: GraphQLInstruments = config .new_graphql_instruments(Arc::new( - config.new_static_graphql_instruments(), + config.new_builtin_graphql_instruments(), )); let context = Context::new(); for event in request { @@ -2865,7 +2865,7 @@ mod tests { .build() .unwrap(); router_instruments = Some(config.new_router_instruments( - Arc::new(config.new_static_router_instruments()), + Arc::new(config.new_builtin_router_instruments()), )); router_instruments .as_mut() @@ -2903,7 +2903,7 @@ mod tests { } => { supergraph_instruments = Some(config.new_supergraph_instruments(Arc::new( - config.new_static_supergraph_instruments(), + config.new_builtin_supergraph_instruments(), ))); let mut request = supergraph::Request::fake_builder() @@ -2957,10 +2957,10 @@ mod tests { headers, } => { subgraph_instruments = Some(config.new_subgraph_instruments( - Arc::new(config.new_static_subgraph_instruments()), + Arc::new(config.new_builtin_subgraph_instruments()), )); cache_instruments = Some(config.new_cache_instruments( - Arc::new(config.new_static_cache_instruments()), + Arc::new(config.new_builtin_cache_instruments()), )); let graphql_request = graphql::Request::fake_builder() .query(query) @@ -3258,7 +3258,7 @@ mod tests { .unwrap(); let router_instruments = - config.new_router_instruments(Arc::new(config.new_static_router_instruments())); + config.new_router_instruments(Arc::new(config.new_builtin_router_instruments())); let router_req = RouterRequest::fake_builder() .header("conditional-custom", "X") .header("x-my-header-count", "55") @@ -3297,7 +3297,7 @@ mod tests { ); let router_instruments = - config.new_router_instruments(Arc::new(config.new_static_router_instruments())); + config.new_router_instruments(Arc::new(config.new_builtin_router_instruments())); let router_req = RouterRequest::fake_builder() .header("content-length", "35") .header("x-my-header-count", "5") @@ -3339,7 +3339,7 @@ mod tests { ); let router_instruments = - config.new_router_instruments(Arc::new(config.new_static_router_instruments())); + config.new_router_instruments(Arc::new(config.new_builtin_router_instruments())); let router_req = RouterRequest::fake_builder() .header("content-length", "35") .header("content-type", "application/graphql") @@ -3369,7 +3369,7 @@ mod tests { ); let router_instruments = - config.new_router_instruments(Arc::new(config.new_static_router_instruments())); + config.new_router_instruments(Arc::new(config.new_builtin_router_instruments())); let router_req = RouterRequest::fake_builder() .header("content-length", "35") .header("content-type", "application/graphql") @@ -3509,7 +3509,7 @@ mod tests { let custom_instruments = SupergraphCustomInstruments::new( &config.supergraph.custom, - Arc::new(config.new_static_supergraph_instruments()), + Arc::new(config.new_builtin_supergraph_instruments()), ); let context = crate::context::Context::new(); let _ = context.insert(OPERATION_KIND, "query".to_string()).unwrap(); @@ -3577,7 +3577,7 @@ mod tests { let custom_instruments = SupergraphCustomInstruments::new( &config.supergraph.custom, - Arc::new(config.new_static_supergraph_instruments()), + Arc::new(config.new_builtin_supergraph_instruments()), ); let supergraph_req = supergraph::Request::fake_builder() .header("content-length", "35") @@ -3634,7 +3634,7 @@ mod tests { let custom_instruments = SupergraphCustomInstruments::new( &config.supergraph.custom, - Arc::new(config.new_static_supergraph_instruments()), + Arc::new(config.new_builtin_supergraph_instruments()), ); let supergraph_req = supergraph::Request::fake_builder() .header("content-length", "35") diff --git a/apollo-router/src/plugins/telemetry/mod.rs b/apollo-router/src/plugins/telemetry/mod.rs index c69e9cc525..5e1c8aee92 100644 --- a/apollo-router/src/plugins/telemetry/mod.rs +++ b/apollo-router/src/plugins/telemetry/mod.rs @@ -10,6 +10,7 @@ use ::tracing::info_span; use ::tracing::Span; use axum::headers::HeaderName; use config_new::cache::CacheInstruments; +use config_new::instruments::InstrumentsConfig; use config_new::instruments::StaticInstrument; use config_new::Selectors; use dashmap::DashMap; @@ -258,6 +259,24 @@ impl Drop for Telemetry { } } +struct BuiltinInstruments { + graphql_custom_instruments: Arc>, + router_custom_instruments: Arc>, + supergraph_custom_instruments: Arc>, + subgraph_custom_instruments: Arc>, + cache_custom_instruments: Arc>, +} + +fn create_builtin_instruments(config: &InstrumentsConfig) -> BuiltinInstruments { + BuiltinInstruments { + graphql_custom_instruments: Arc::new(config.new_builtin_graphql_instruments()), + router_custom_instruments: Arc::new(config.new_builtin_router_instruments()), + supergraph_custom_instruments: Arc::new(config.new_builtin_supergraph_instruments()), + subgraph_custom_instruments: Arc::new(config.new_builtin_subgraph_instruments()), + cache_custom_instruments: Arc::new(config.new_builtin_cache_instruments()), + } +} + #[async_trait::async_trait] impl Plugin for Telemetry { type Config = config::Conf; @@ -281,56 +300,13 @@ impl Plugin for Telemetry { ::tracing::warn!("telemetry.instrumentation.spans.mode is currently set to 'deprecated', either explicitly or via defaulting. Set telemetry.instrumentation.spans.mode explicitly in your router.yaml to 'spec_compliant' for log and span attributes that follow OpenTelemetry semantic conventions. This option will be defaulted to 'spec_compliant' in a future release and eventually removed altogether"); } - let graphql_custom_instruments = if cfg!(test) { - RwLock::new(Arc::new( - config - .instrumentation - .instruments - .new_static_graphql_instruments(), - )) - } else { - RwLock::default() - }; - let router_custom_instruments = if cfg!(test) { - RwLock::new(Arc::new( - config - .instrumentation - .instruments - .new_static_router_instruments(), - )) - } else { - RwLock::default() - }; - let supergraph_custom_instruments = if cfg!(test) { - RwLock::new(Arc::new( - config - .instrumentation - .instruments - .new_static_supergraph_instruments(), - )) - } else { - RwLock::default() - }; - let subgraph_custom_instruments = if cfg!(test) { - RwLock::new(Arc::new( - config - .instrumentation - .instruments - .new_static_subgraph_instruments(), - )) - } else { - RwLock::default() - }; - let cache_custom_instruments = if cfg!(test) { - RwLock::new(Arc::new( - config - .instrumentation - .instruments - .new_static_cache_instruments(), - )) - } else { - RwLock::default() - }; + let BuiltinInstruments { + graphql_custom_instruments, + router_custom_instruments, + supergraph_custom_instruments, + subgraph_custom_instruments, + cache_custom_instruments, + } = create_builtin_instruments(&config.instrumentation.instruments); Ok(Telemetry { custom_endpoints: metrics_builder.custom_endpoints, @@ -349,11 +325,11 @@ impl Plugin for Telemetry { .map(FilterMeterProvider::public), is_active: false, }), - graphql_custom_instruments, - router_custom_instruments, - supergraph_custom_instruments, - subgraph_custom_instruments, - cache_custom_instruments, + graphql_custom_instruments: RwLock::new(graphql_custom_instruments), + router_custom_instruments: RwLock::new(router_custom_instruments), + supergraph_custom_instruments: RwLock::new(supergraph_custom_instruments), + subgraph_custom_instruments: RwLock::new(subgraph_custom_instruments), + cache_custom_instruments: RwLock::new(cache_custom_instruments), sampling_filter_ratio, config: Arc::new(config), }) @@ -912,36 +888,19 @@ impl Telemetry { activation.reload_metrics(); - *self.graphql_custom_instruments.write() = Arc::new( - self.config - .instrumentation - .instruments - .new_static_graphql_instruments(), - ); - *self.router_custom_instruments.write() = Arc::new( - self.config - .instrumentation - .instruments - .new_static_router_instruments(), - ); - *self.supergraph_custom_instruments.write() = Arc::new( - self.config - .instrumentation - .instruments - .new_static_supergraph_instruments(), - ); - *self.subgraph_custom_instruments.write() = Arc::new( - self.config - .instrumentation - .instruments - .new_static_subgraph_instruments(), - ); - *self.cache_custom_instruments.write() = Arc::new( - self.config - .instrumentation - .instruments - .new_static_cache_instruments(), - ); + let BuiltinInstruments { + graphql_custom_instruments, + router_custom_instruments, + supergraph_custom_instruments, + subgraph_custom_instruments, + cache_custom_instruments, + } = create_builtin_instruments(&self.config.instrumentation.instruments); + + *self.graphql_custom_instruments.write() = graphql_custom_instruments; + *self.router_custom_instruments.write() = router_custom_instruments; + *self.supergraph_custom_instruments.write() = supergraph_custom_instruments; + *self.subgraph_custom_instruments.write() = subgraph_custom_instruments; + *self.cache_custom_instruments.write() = cache_custom_instruments; reload_fmt(create_fmt_layer(&self.config)); activation.is_active = true;