perf: don't re-create meter and instruments on every calls

.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

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;

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,14 +117,36 @@ pub(crate) struct CostInstrumentsConfig {
 }
 
 impl CostInstrumentsConfig {
-    pub(crate) fn to_instruments(&self) -> CostInstruments {
+    pub(crate) fn new_static_instruments(&self) -> HashMap<String, StaticInstrument> {
+        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<HashMap<String, StaticInstrument>>,
+    ) -> CostInstruments {
         let cost_estimated = self.cost_estimated.is_enabled().then(|| {
             Self::histogram(
                 COST_ESTIMATED,
                 &self.cost_estimated,
                 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<Extendable<SupergraphAttributes, SupergraphSelector>>,
         selector: SupergraphSelector,
+        static_instruments: &Arc<HashMap<String, StaticInstrument>>,
     ) -> CustomHistogram<Request, Response, SupergraphAttributes, SupergraphSelector> {
-        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!(

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

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

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::<Telemetry>::builder()
+            let harness: PluginTestHarness<Telemetry> = PluginTestHarness::<Telemetry>::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!(