From 5b696a0ec6c686365e819c6d4c26949a5013c0f9 Mon Sep 17 00:00:00 2001 From: carodewig <16093297+carodewig@users.noreply.github.com> Date: Wed, 11 Jun 2025 16:13:54 -0400 Subject: [PATCH 1/4] rework selector --- .../telemetry/config_new/router/selectors.rs | 13 +++++----- .../config_new/supergraph/selectors.rs | 25 ++++++++----------- 2 files changed, 17 insertions(+), 21 deletions(-) diff --git a/apollo-router/src/plugins/telemetry/config_new/router/selectors.rs b/apollo-router/src/plugins/telemetry/config_new/router/selectors.rs index dd567d3a9c..741f2a7f7d 100644 --- a/apollo-router/src/plugins/telemetry/config_new/router/selectors.rs +++ b/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()), diff --git a/apollo-router/src/plugins/telemetry/config_new/supergraph/selectors.rs b/apollo-router/src/plugins/telemetry/config_new/supergraph/selectors.rs index 96943b5c57..ad54a09c0d 100644 --- a/apollo-router/src/plugins/telemetry/config_new/supergraph/selectors.rs +++ b/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, From d55edd317044d5f9b36118c0f1d64bc4a2f3ae1a Mon Sep 17 00:00:00 2001 From: carodewig <16093297+carodewig@users.noreply.github.com> Date: Wed, 11 Jun 2025 16:14:19 -0400 Subject: [PATCH 2/4] Update tests to reflect CONTAINS_GRAPHQL_ERROR false and missing --- .../telemetry/config_new/router/spans.rs | 37 ++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/apollo-router/src/plugins/telemetry/config_new/router/spans.rs b/apollo-router/src/plugins/telemetry/config_new/router/spans.rs index 2b0e8f9be6..385fb98c78 100644 --- a/apollo-router/src/plugins/telemetry/config_new/router/spans.rs +++ b/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(); From d0e3b2f40b609dbbd6c43e1d36958c7a2b71118e Mon Sep 17 00:00:00 2001 From: carodewig <16093297+carodewig@users.noreply.github.com> Date: Thu, 12 Jun 2025 10:30:13 -0400 Subject: [PATCH 3/4] Create feat_caroline_consistent_on_error.md --- .changesets/feat_caroline_consistent_on_error.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changesets/feat_caroline_consistent_on_error.md diff --git a/.changesets/feat_caroline_consistent_on_error.md b/.changesets/feat_caroline_consistent_on_error.md new file mode 100644 index 0000000000..9e3b679021 --- /dev/null +++ b/.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 From 50a90d52af1057366973c48b4668f48026c1c0f4 Mon Sep 17 00:00:00 2001 From: carodewig <16093297+carodewig@users.noreply.github.com> Date: Tue, 17 Jun 2025 14:55:00 -0400 Subject: [PATCH 4/4] Additional test case --- .../router/attribute.on_graphql_error/metrics.snap | 4 ++++ .../router/attribute.on_graphql_error/test.yaml | 14 +++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/apollo-router/src/plugins/telemetry/config_new/fixtures/router/attribute.on_graphql_error/metrics.snap b/apollo-router/src/plugins/telemetry/config_new/fixtures/router/attribute.on_graphql_error/metrics.snap index 3190810cc8..e3b9d96d37 100644 --- a/apollo-router/src/plugins/telemetry/config_new/fixtures/router/attribute.on_graphql_error/metrics.snap +++ b/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: diff --git a/apollo-router/src/plugins/telemetry/config_new/fixtures/router/attribute.on_graphql_error/test.yaml b/apollo-router/src/plugins/telemetry/config_new/fixtures/router/attribute.on_graphql_error/test.yaml index c61052951f..7ae7588c0a 100644 --- a/apollo-router/src/plugins/telemetry/config_new/fixtures/router/attribute.on_graphql_error/test.yaml +++ b/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 \ No newline at end of file + status: 200 + - - router_request: + uri: "/hello" + method: GET + body: | + hello + - context: + map: + "apollo::telemetry::contains_graphql_error": false + - router_response: + body: | + hello + status: 200