fix(telemetry): improve support of conditions at the request level, e…

…specially for events (#5759)

Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>

.changesets/feat_candle_exhale_deodorant_weeds.md

@@ -0,0 +1,26 @@
+### Add warnings for invalid configuration on custom telemetry ([PR #5759](https://github.com/apollographql/router/issues/5759))
+
+For example sometimes if you have configuration like this:
+
+```yaml
+telemetry:
+  instrumentation:
+    events:
+      subgraph:
+        my.event:
+          message: "Auditing Router Event"
+          level: info
+          on: request
+          attributes:
+            subgraph.response.status:
+              subgraph_response_status: code # This is a first warning because you can't access to the response if you're at the request stage
+          condition:
+            eq:
+            - subgraph_name # Another warning because instead of writing subgraph_name: true which is the selector, you're asking for a comparison between 2 strings ("subgraph_name" and "product")
+            - product
+```
+
+This configuration is syntaxically correct but wouldn't probably do what you would like to. I put comments to highlight 2 mistakes in this example.
+Before it was silently computed, now you'll get warning when starting the router.
+
+By [@bnjjj](https://github.com/bnjjj) in https://github.com/apollographql/router/pull/5759

.changesets/fix_bnjjj_fix_5702.md

@@ -0,0 +1,21 @@
+### Improve support of conditions at the request level, especially for events ([Issue #5702](https://github.com/apollographql/router/issues/5702))
+
+`exists` condition is now properly handled with events, this configuration will now work:
+
+```yaml
+telemetry:
+  instrumentation:
+    events:
+      supergraph:
+        my.event:
+          message: "Auditing Router Event"
+          level: info
+          on: request
+          attributes:
+            graphql.operation.name: true
+          condition:
+            exists:
+              operation_name: string
+```
+
+By [@bnjjj](https://github.com/bnjjj) in https://github.com/apollographql/router/pull/5759

apollo-router/src/plugins/telemetry/config.rs

@@ -94,6 +94,14 @@ pub(crate) struct Instrumentation {
     pub(crate) instruments: config_new::instruments::InstrumentsConfig,
 }
 
+impl Instrumentation {
+    pub(crate) fn validate(&self) -> Result<(), String> {
+        self.events.validate()?;
+        self.instruments.validate()?;
+        self.spans.validate()
+    }
+}
+
 /// Metrics configuration
 #[derive(Clone, Default, Debug, Deserialize, JsonSchema)]
 #[serde(deny_unknown_fields, default)]

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

@@ -149,6 +149,18 @@ where
     }
 }
 
+impl<Att, Request, Response, EventResponse> Conditional<Att>
+where
+    Att: Selector<Request = Request, Response = Response, EventResponse = EventResponse>,
+{
+    pub(crate) fn validate(&self) -> Result<(), String> {
+        match &self.condition {
+            Some(cond) => cond.lock().validate(None),
+            None => Ok(()),
+        }
+    }
+}
+
 impl<Att, Request, Response, EventResponse> Selector for Conditional<Att>
 where
     Att: Selector<Request = Request, Response = Response, EventResponse = EventResponse>,
@@ -334,6 +346,10 @@ where
             _ => None,
         }
     }
+
+    fn is_active(&self, stage: super::Stage) -> bool {
+        self.selector.is_active(stage)
+    }
 }
 
 /// Custom Deserializer for attributes that will deserialize into a custom field if possible, but otherwise into one of the pre-defined attributes.

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

@@ -3,6 +3,7 @@ use schemars::JsonSchema;
 use serde::Deserialize;
 use tower::BoxError;
 
+use super::Stage;
 use crate::plugins::telemetry::config::AttributeValue;
 use crate::plugins::telemetry::config_new::Selector;
 use crate::Context;
@@ -55,28 +56,98 @@ impl<T> Condition<T>
 where
     T: Selector,
 {
-    pub(crate) fn evaluate_request(&mut self, request: &T::Request) -> Option<bool> {
+    /// restricted_stage is Some if this condiiton will only applies at a specific stage like for events for example
+    pub(crate) fn validate(&self, restricted_stage: Option<Stage>) -> Result<(), String> {
         match self {
-            Condition::Eq(eq) => match (eq[0].on_request(request), eq[1].on_request(request)) {
-                (None, None) => None,
-                (None, Some(right)) => {
-                    eq[1] = SelectorOrValue::Value(right.into());
-                    None
+            Condition::Eq(arr) | Condition::Gt(arr) | Condition::Lt(arr) => match (&arr[0], &arr[1]) {
+                (SelectorOrValue::Value(val1), SelectorOrValue::Value(val2)) => {
+                    Err(format!("trying to compare 2 values ('{val1}' and '{val2}'), usually it's a syntax error because you want to use a specific selector and a value in a condition"))
                 }
-                (Some(left), None) => {
-                    eq[0] = SelectorOrValue::Value(left.into());
-                    None
-                }
-                (Some(left), Some(right)) => {
-                    if left == right {
-                        *self = Condition::True;
-                        Some(true)
-                    } else {
-                        Some(false)
+                (SelectorOrValue::Value(_), SelectorOrValue::Selector(sel)) | (SelectorOrValue::Selector(sel), SelectorOrValue::Value(_)) => {
+                    // Special condition for events
+                    if let Some(Stage::Request) = &restricted_stage {
+                        if !sel.is_active(Stage::Request) {
+                            return Err(format!("selector {sel:?} is only valid for request stage, this log event will never trigger"));
+                        }
+                    }
+                    Ok(())
+                },
+                (SelectorOrValue::Selector(sel1), SelectorOrValue::Selector(sel2)) => {
+                    // Special condition for events
+                    if let Some(Stage::Request) = &restricted_stage {
+                        if !sel1.is_active(Stage::Request) {
+                            return Err(format!("selector {sel1:?} is only valid for request stage, this log event will never trigger"));
+                        }
+                        if !sel2.is_active(Stage::Request) {
+                            return Err(format!("selector {sel2:?} is only valid for request stage, this log event will never trigger"));
+                        }
                     }
+                    Ok(())
+                },
+            },
+            Condition::Exists(sel) => {
+                match restricted_stage {
+                    Some(stage) => {
+                        if sel.is_active(stage) {
+                            Ok(())
+                        } else {
+                            Err(format!("the 'exists' condition use a selector applied at the wrong stage, this condition will be executed at the {} stage", stage))
+                        }
+                    },
+                    None => Ok(())
                 }
             },
+            Condition::All(all) => {
+                for cond in all {
+                    cond.validate(restricted_stage)?;
+                }
+
+                Ok(())
+            },
+            Condition::Any(any) => {
+                for cond in any {
+                    cond.validate(restricted_stage)?;
+                }
+
+                Ok(())
+            },
+            Condition::Not(cond) => cond.validate(restricted_stage),
+            Condition::True | Condition::False => Ok(()),
+        }
+    }
+
+    pub(crate) fn evaluate_request(&mut self, request: &T::Request) -> Option<bool> {
+        match self {
+            Condition::Eq(eq) => {
+                if !eq[0].is_active(Stage::Request) && !eq[1].is_active(Stage::Request) {
+                    // Nothing to compute here
+                    return None;
+                }
+                match (eq[0].on_request(request), eq[1].on_request(request)) {
+                    (None, None) => None,
+                    (None, Some(right)) => {
+                        eq[1] = SelectorOrValue::Value(right.into());
+                        None
+                    }
+                    (Some(left), None) => {
+                        eq[0] = SelectorOrValue::Value(left.into());
+                        None
+                    }
+                    (Some(left), Some(right)) => {
+                        if left == right {
+                            *self = Condition::True;
+                            Some(true)
+                        } else {
+                            Some(false)
+                        }
+                    }
+                }
+            }
             Condition::Gt(gt) => {
+                if !gt[0].is_active(Stage::Request) && !gt[1].is_active(Stage::Request) {
+                    // Nothing to compute here
+                    return None;
+                }
                 let left_att = gt[0].on_request(request).map(AttributeValue::from);
                 let right_att = gt[1].on_request(request).map(AttributeValue::from);
                 match (left_att, right_att) {
@@ -112,6 +183,10 @@ where
                 }
             }
             Condition::Lt(lt) => {
+                if !lt[0].is_active(Stage::Request) && !lt[1].is_active(Stage::Request) {
+                    // Nothing to compute here
+                    return None;
+                }
                 let left_att = lt[0].on_request(request).map(AttributeValue::from);
                 let right_att = lt[1].on_request(request).map(AttributeValue::from);
                 match (left_att, right_att) {
@@ -147,9 +222,13 @@ where
                 }
             }
             Condition::Exists(exist) => {
-                if exist.on_request(request).is_some() {
-                    *self = Condition::True;
-                    Some(true)
+                if exist.is_active(Stage::Request) {
+                    if exist.on_request(request).is_some() {
+                        *self = Condition::True;
+                        Some(true)
+                    } else {
+                        Some(false)
+                    }
                 } else {
                     None
                 }
@@ -331,6 +410,7 @@ where
             Condition::False => false,
         }
     }
+
     pub(crate) fn evaluate_drop(&self) -> Option<bool> {
         match self {
             Condition::Eq(eq) => match (eq[0].on_drop(), eq[1].on_drop()) {
@@ -478,6 +558,13 @@ where
             SelectorOrValue::Selector(selector) => selector.on_drop(),
         }
     }
+
+    fn is_active(&self, stage: super::Stage) -> bool {
+        match self {
+            SelectorOrValue::Value(_) => true,
+            SelectorOrValue::Selector(selector) => selector.is_active(stage),
+        }
+    }
 }
 
 #[cfg(test)]
@@ -494,8 +581,10 @@ mod test {
     use crate::plugins::telemetry::config_new::test::field;
     use crate::plugins::telemetry::config_new::test::ty;
     use crate::plugins::telemetry::config_new::Selector;
+    use crate::plugins::telemetry::config_new::Stage;
     use crate::Context;
 
+    #[derive(Debug)]
     enum TestSelector {
         Req,
         Resp,
@@ -567,11 +656,22 @@ mod test {
                 _ => None,
             }
         }
+
+        fn is_active(&self, stage: crate::plugins::telemetry::config_new::Stage) -> bool {
+            match self {
+                Req => matches!(stage, Stage::Request),
+                Resp => matches!(
+                    stage,
+                    Stage::Response | Stage::ResponseEvent | Stage::ResponseField
+                ),
+                Static(_) => true,
+            }
+        }
     }
 
     #[test]
     fn test_condition_exist() {
-        assert_eq!(exists(Req).req(None), None);
+        assert_eq!(exists(Req).req(None), Some(false));
         assert_eq!(exists(Req).req(Some(1i64)), Some(true));
         assert!(!exists(Resp).resp(None));
         assert!(exists(Resp).resp(Some(1i64)));
@@ -747,7 +847,8 @@ mod test {
         assert_eq!(lt(Req, Req).req(None), None);
 
         assert_eq!(exists(Req).req(Some(1i64)), Some(true));
-        assert_eq!(exists(Req).req(None), None);
+        assert_eq!(exists(Req).req(None), Some(false));
+        assert!(!exists(Resp).resp(None));
 
         assert_eq!(all(eq(1, 1), eq(1, Req)).req(Some(1i64)), Some(true));
         assert_eq!(all(eq(1, 1), eq(1, Req)).req(None), None);
@@ -760,6 +861,22 @@ mod test {
         assert!(eq(Resp, "error").error(Some("error")));
     }
 
+    #[test]
+    fn test_condition_validate() {
+        assert!(eq(Req, 1).validate(Some(Stage::Request)).is_ok());
+        assert!(eq(Req, 1).validate(Some(Stage::Response)).is_ok());
+        assert!(eq(1, Req).validate(Some(Stage::Request)).is_ok());
+        assert!(eq(1, Req).validate(Some(Stage::Response)).is_ok());
+        assert!(eq(Resp, 1).validate(Some(Stage::Request)).is_err());
+        assert!(eq(Resp, 1).validate(None).is_ok());
+        assert!(eq(1, Resp).validate(None).is_ok());
+        assert!(eq(1, Resp).validate(Some(Stage::Request)).is_err());
+        assert!(exists(Resp).validate(Some(Stage::Request)).is_err());
+        assert!(exists(Req).validate(None).is_ok());
+        assert!(exists(Req).validate(Some(Stage::Request)).is_ok());
+        assert!(exists(Resp).validate(None).is_ok());
+    }
+
     #[test]
     fn test_evaluate_drop() {
         assert!(eq(Req, 1).evaluate_drop().is_none());