Align on_graphql_error selector return values with subgraph_on_graphql_error

.changesets/feat_caroline_consistent_on_error.md

@@ -0,0 +1,5 @@
+### Align `on_graphql_error` selector return values with `subgraph_on_graphql_error` ([PR #7676](https://github.com/apollographql/router/pull/7676))
+
+The `on_graphql_error` selector will now return `true` or `false`, in alignment with the `subgraph_on_graphql_error` selector. Previously, the selector would return `true` or `None`.
+
+By [@carodewig](https://github.com/carodewig) in https://github.com/apollographql/router/pull/7676

apollo-router/src/plugins/telemetry/config_new/fixtures/router/attribute.on_graphql_error/metrics.snap

@@ -18,6 +18,10 @@ info:
   unit: s
   data:
     datapoints:
+      - sum: 0.1
+        count: 1
+        attributes:
+          on.graphql.error: false
       - sum: 0.1
         count: 1
         attributes:

apollo-router/src/plugins/telemetry/config_new/fixtures/router/attribute.on_graphql_error/test.yaml

@@ -12,4 +12,16 @@ events:
     - router_response:
         body: |
           hello
-        status: 200
+        status: 200
+  - - router_request:
+        uri: "/hello"
+        method: GET
+        body: |
+          hello
+    - context:
+        map:
+          "apollo::telemetry::contains_graphql_error": false
+    - router_response:
+        body: |
+          hello
+        status: 200

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

@@ -290,13 +290,12 @@ impl Selector for RouterSelector {
                 baggage, default, ..
             } => get_baggage(baggage).or_else(|| default.maybe_to_otel_value()),
             RouterSelector::OnGraphQLError { on_graphql_error } if *on_graphql_error => {
-                if response.context.get_json_value(CONTAINS_GRAPHQL_ERROR)
-                    == Some(serde_json_bytes::Value::Bool(true))
-                {
-                    Some(opentelemetry::Value::Bool(true))
-                } else {
-                    None
-                }
+                let contains_error = response
+                    .context
+                    .get_json_value(CONTAINS_GRAPHQL_ERROR)
+                    .and_then(|value| value.as_bool())
+                    .unwrap_or_default();
+                Some(opentelemetry::Value::Bool(contains_error))
             }
             RouterSelector::Static(val) => Some(val.clone().into()),
             RouterSelector::StaticField { r#static } => Some(r#static.clone().into()),

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

@@ -231,7 +231,7 @@ mod test {
     }
 
     #[test]
-    fn test_router_request_custom_attribute_not_on_graphql_error() {
+    fn test_router_request_custom_attribute_not_on_graphql_error_context_false() {
         let mut spans = RouterSpans::default();
         spans.attributes.custom.insert(
             "test".to_string(),
@@ -266,6 +266,41 @@ mod test {
         );
     }
 
+    #[test]
+    fn test_router_request_custom_attribute_not_on_graphql_error_context_missing() {
+        let mut spans = RouterSpans::default();
+        spans.attributes.custom.insert(
+            "test".to_string(),
+            Conditional {
+                selector: RouterSelector::ResponseHeader {
+                    response_header: "my-header".to_string(),
+                    redact: None,
+                    default: None,
+                },
+                condition: Some(Arc::new(Mutex::new(Condition::Eq([
+                    SelectorOrValue::Value(AttributeValue::Bool(true)),
+                    SelectorOrValue::Selector(RouterSelector::OnGraphQLError {
+                        on_graphql_error: true,
+                    }),
+                ])))),
+                value: Arc::new(Default::default()),
+            },
+        );
+        let context = Context::new();
+        let values = spans.attributes.on_response(
+            &router::Response::fake_builder()
+                .header("my-header", "test_val")
+                .context(context)
+                .build()
+                .unwrap(),
+        );
+        assert!(
+            !values
+                .iter()
+                .any(|key_val| key_val.key == opentelemetry::Key::from_static_str("test"))
+        );
+    }
+
     #[test]
     fn test_router_request_custom_attribute_condition_true() {
         let mut spans = RouterSpans::default();

apollo-router/src/plugins/telemetry/config_new/supergraph/selectors.rs

@@ -371,13 +371,12 @@ impl Selector for SupergraphSelector {
                 .and_then(|v| v.maybe_to_otel_value())
                 .or_else(|| default.maybe_to_otel_value()),
             SupergraphSelector::OnGraphQLError { on_graphql_error } if *on_graphql_error => {
-                if response.context.get_json_value(CONTAINS_GRAPHQL_ERROR)
-                    == Some(serde_json_bytes::Value::Bool(true))
-                {
-                    Some(opentelemetry::Value::Bool(true))
-                } else {
-                    None
-                }
+                let contains_error = response
+                    .context
+                    .get_json_value(CONTAINS_GRAPHQL_ERROR)
+                    .and_then(|value| value.as_bool())
+                    .unwrap_or_default();
+                Some(opentelemetry::Value::Bool(contains_error))
             }
             SupergraphSelector::OperationName {
                 operation_name,
@@ -464,13 +463,11 @@ impl Selector for SupergraphSelector {
                     .map(opentelemetry::Value::from),
             },
             SupergraphSelector::OnGraphQLError { on_graphql_error } if *on_graphql_error => {
-                if ctx.get_json_value(CONTAINS_GRAPHQL_ERROR)
-                    == Some(serde_json_bytes::Value::Bool(true))
-                {
-                    Some(opentelemetry::Value::Bool(true))
-                } else {
-                    None
-                }
+                let contains_error = ctx
+                    .get_json_value(CONTAINS_GRAPHQL_ERROR)
+                    .and_then(|value| value.as_bool())
+                    .unwrap_or_default();
+                Some(opentelemetry::Value::Bool(contains_error))
             }
             SupergraphSelector::OperationName {
                 operation_name,