Optimize graphql instruments (#5375)

Co-authored-by: bryn <bryn@apollographql.com>

.changesets/fix_bryn_optimize_graphq_instruments.md

@@ -0,0 +1,7 @@
+### Optimize graphql instruments ([PR #5375](https://github.com/apollographql/router/pull/5375))
+
+When processing selectors for graphql instruments we need to make sure we perform no heap allocation otherwise performance is affected.
+
+This PR removes Vec allocations that were being performed on every field yielding a significant improvement in performance. 
+
+By [@BrynCooke](https://github.com/BrynCooke) in https://github.com/apollographql/router/pull/5375

apollo-router/src/plugins/telemetry/config_new/extendable.rs

@@ -239,19 +239,19 @@ where
 
     fn on_response_field(
         &self,
+        attrs: &mut Vec<KeyValue>,
         ty: &apollo_compiler::executable::NamedType,
         field: &apollo_compiler::executable::Field,
         value: &serde_json_bytes::Value,
         ctx: &Context,
-    ) -> Vec<KeyValue> {
-        let mut attrs = self.attributes.on_response_field(ty, field, value, ctx);
+    ) {
+        self.attributes
+            .on_response_field(attrs, ty, field, value, ctx);
         let custom_attributes = self.custom.iter().filter_map(|(key, v)| {
             v.on_response_field(ty, field, value, ctx)
                 .map(|v| KeyValue::new(key.clone(), v))
         });
         attrs.extend(custom_attributes);
-
-        attrs
     }
 }
 

apollo-router/src/plugins/telemetry/config_new/graphql/attributes.rs

@@ -75,12 +75,12 @@ impl Selectors for GraphQLAttributes {
 
     fn on_response_field(
         &self,
+        attrs: &mut Vec<KeyValue>,
         ty: &NamedType,
         field: &Field,
         value: &Value,
         ctx: &Context,
-    ) -> Vec<KeyValue> {
-        let mut attrs = Vec::with_capacity(4);
+    ) {
         if let Some(true) = self.field_name {
             if let Some(name) = (GraphQLSelector::FieldName {
                 field_name: FieldName::String,
@@ -127,7 +127,6 @@ impl Selectors for GraphQLAttributes {
                 attrs.push(KeyValue::new("graphql.operation.name", length));
             }
         }
-        attrs
     }
 }
 
@@ -167,7 +166,8 @@ mod test {
         };
         let ctx = Context::default();
         let _ = ctx.insert(OPERATION_NAME, "operation_name".to_string());
-        let result = attributes.on_response_field(ty(), field(), &json!(true), &ctx);
+        let mut result = Default::default();
+        attributes.on_response_field(&mut result, ty(), field(), &json!(true), &ctx);
         assert_eq!(result.len(), 4);
         assert_eq!(result[0].key.as_str(), "graphql.field.name");
         assert_eq!(result[0].value.as_str(), "field_name");
@@ -190,8 +190,14 @@ mod test {
         };
         let ctx = Context::default();
         let _ = ctx.insert(OPERATION_NAME, "operation_name".to_string());
-        let result =
-            attributes.on_response_field(ty(), field(), &json!(vec![true, true, true]), &ctx);
+        let mut result = Default::default();
+        attributes.on_response_field(
+            &mut result,
+            ty(),
+            field(),
+            &json!(vec![true, true, true]),
+            &ctx,
+        );
         assert_eq!(result.len(), 5);
         assert_eq!(result[0].key.as_str(), "graphql.field.name");
         assert_eq!(result[0].value.as_str(), "field_name");

apollo-router/src/plugins/telemetry/config_new/instruments.rs

@@ -1378,17 +1378,6 @@ where
             return;
         }
 
-        // Response field may be called multiple times so we don't extend inner.attributes
-        let mut attrs = inner.attributes.clone();
-        if let Some(selectors) = inner.selectors.as_ref() {
-            attrs.extend(
-                selectors
-                    .on_response_field(ty, field, value, ctx)
-                    .into_iter()
-                    .collect::<Vec<_>>(),
-            );
-        }
-
         if let Some(selected_value) = inner
             .selector
             .as_ref()
@@ -1428,8 +1417,23 @@ where
             }
         };
 
+        // Response field may be called multiple times
+        // But there's no need for us to create a new vec each time, we can just extend the existing one and then reset it after
+        let original_length = inner.attributes.len();
+        if inner.counter.is_some() && increment.is_some() {
+            // Only get the attributes from the selectors if we are actually going to increment the histogram
+            // Cloning selectors should not have to happen
+            let selectors = inner.selectors.clone();
+            let attributes = &mut inner.attributes;
+            if let Some(selectors) = selectors {
+                selectors.on_response_field(attributes, ty, field, value, ctx);
+            }
+        }
+
         if let (Some(counter), Some(increment)) = (&inner.counter, increment) {
-            counter.add(increment, &attrs);
+            counter.add(increment, &inner.attributes);
+            // Reset the attributes to the original length, this will discard the new attributes added from selectors.
+            inner.attributes.truncate(original_length);
         }
     }
 }
@@ -1786,17 +1790,6 @@ where
             return;
         }
 
-        // Response field may be called multiple times so we don't extend inner.attributes
-        let mut attrs = inner.attributes.clone();
-        if let Some(selectors) = inner.selectors.as_ref() {
-            attrs.extend(
-                selectors
-                    .on_response_field(ty, field, value, ctx)
-                    .into_iter()
-                    .collect::<Vec<_>>(),
-            );
-        }
-
         if let Some(selected_value) = inner
             .selector
             .as_ref()
@@ -1836,8 +1829,23 @@ where
             }
         };
 
+        // Response field may be called multiple times
+        // But there's no need for us to create a new vec each time, we can just extend the existing one and then reset it after
+        let original_length = inner.attributes.len();
+        if inner.histogram.is_some() && increment.is_some() {
+            // Only get the attributes from the selectors if we are actually going to increment the histogram
+            // Cloning selectors should not have to happen
+            let selectors = inner.selectors.clone();
+            let attributes = &mut inner.attributes;
+            if let Some(selectors) = selectors {
+                selectors.on_response_field(attributes, ty, field, value, ctx);
+            }
+        }
+
         if let (Some(histogram), Some(increment)) = (&inner.histogram, increment) {
-            histogram.record(increment, &attrs);
+            histogram.record(increment, &inner.attributes);
+            // Reset the attributes to the original length, this will discard the new attributes added from selectors.
+            inner.attributes.truncate(original_length);
         }
     }
 }

apollo-router/src/plugins/telemetry/config_new/mod.rs

@@ -41,12 +41,12 @@ pub(crate) trait Selectors {
     fn on_error(&self, error: &BoxError, ctx: &Context) -> Vec<KeyValue>;
     fn on_response_field(
         &self,
+        _attrs: &mut Vec<KeyValue>,
         _ty: &apollo_compiler::executable::NamedType,
         _field: &apollo_compiler::executable::Field,
         _value: &serde_json_bytes::Value,
         _ctx: &Context,
-    ) -> Vec<KeyValue> {
-        Vec::with_capacity(0)
+    ) {
     }
 }