From baaaa61fb47cad68b1218bf763c8e72a8253511a Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Mon, 29 Jul 2024 17:23:51 +0200 Subject: [PATCH 01/13] wip Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- .../telemetry/config_new/conditional.rs | 4 + .../telemetry/config_new/conditions.rs | 11 ++ .../telemetry/config_new/graphql/selectors.rs | 5 + .../src/plugins/telemetry/config_new/mod.rs | 11 ++ .../plugins/telemetry/config_new/selectors.rs | 166 ++++++++++++++++++ 5 files changed, 197 insertions(+) diff --git a/apollo-router/src/plugins/telemetry/config_new/conditional.rs b/apollo-router/src/plugins/telemetry/config_new/conditional.rs index 94136ee63d..dd36bfee5b 100644 --- a/apollo-router/src/plugins/telemetry/config_new/conditional.rs +++ b/apollo-router/src/plugins/telemetry/config_new/conditional.rs @@ -334,6 +334,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. diff --git a/apollo-router/src/plugins/telemetry/config_new/conditions.rs b/apollo-router/src/plugins/telemetry/config_new/conditions.rs index f4782a3c13..db75771ffe 100644 --- a/apollo-router/src/plugins/telemetry/config_new/conditions.rs +++ b/apollo-router/src/plugins/telemetry/config_new/conditions.rs @@ -456,6 +456,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)] @@ -545,6 +552,10 @@ mod test { _ => None, } } + + fn is_active(&self, _stage: crate::plugins::telemetry::config_new::Stage) -> bool { + true + } } #[test] diff --git a/apollo-router/src/plugins/telemetry/config_new/graphql/selectors.rs b/apollo-router/src/plugins/telemetry/config_new/graphql/selectors.rs index 20a648d465..853681087f 100644 --- a/apollo-router/src/plugins/telemetry/config_new/graphql/selectors.rs +++ b/apollo-router/src/plugins/telemetry/config_new/graphql/selectors.rs @@ -13,6 +13,7 @@ use crate::plugins::telemetry::config_new::instruments::InstrumentValue; use crate::plugins::telemetry::config_new::instruments::StandardUnit; use crate::plugins::telemetry::config_new::selectors::OperationName; use crate::plugins::telemetry::config_new::Selector; +use crate::plugins::telemetry::config_new::Stage; use crate::Context; #[derive(Deserialize, JsonSchema, Clone, Debug)] @@ -173,6 +174,10 @@ impl Selector for GraphQLSelector { } } } + + fn is_active(&self, stage: Stage) -> bool { + matches!(stage, Stage::ResponseField) + } } fn name_to_otel_string(name: &apollo_compiler::Name) -> opentelemetry::StringValue { diff --git a/apollo-router/src/plugins/telemetry/config_new/mod.rs b/apollo-router/src/plugins/telemetry/config_new/mod.rs index 2a3f46edcf..41b04f942b 100644 --- a/apollo-router/src/plugins/telemetry/config_new/mod.rs +++ b/apollo-router/src/plugins/telemetry/config_new/mod.rs @@ -51,6 +51,15 @@ pub(crate) trait Selectors { } } +pub(crate) enum Stage { + Request, + Response, + ResponseEvent, + ResponseField, + Error, + Drop, +} + pub(crate) trait Selector { type Request; type Response; @@ -79,6 +88,8 @@ pub(crate) trait Selector { fn on_drop(&self) -> Option { None } + + fn is_active(&self, stage: Stage) -> bool; } pub(crate) trait DefaultForLevel { diff --git a/apollo-router/src/plugins/telemetry/config_new/selectors.rs b/apollo-router/src/plugins/telemetry/config_new/selectors.rs index 9b373f8ac9..43556bf26b 100644 --- a/apollo-router/src/plugins/telemetry/config_new/selectors.rs +++ b/apollo-router/src/plugins/telemetry/config_new/selectors.rs @@ -815,6 +815,55 @@ impl Selector for RouterSelector { _ => None, } } + + fn is_active(&self, stage: super::Stage) -> bool { + match stage { + super::Stage::Request => { + matches!( + self, + RouterSelector::RequestHeader { .. } + | RouterSelector::RequestMethod { .. } + | RouterSelector::TraceId { .. } + | RouterSelector::StudioOperationId { .. } + | RouterSelector::Baggage { .. } + | RouterSelector::Static(_) + | RouterSelector::Env { .. } + | RouterSelector::StaticField { .. } + ) + } + super::Stage::Response | super::Stage::ResponseEvent => matches!( + self, + RouterSelector::TraceId { .. } + | RouterSelector::StudioOperationId { .. } + | RouterSelector::OperationName { .. } + | RouterSelector::Baggage { .. } + | RouterSelector::Static(_) + | RouterSelector::Env { .. } + | RouterSelector::StaticField { .. } + | RouterSelector::ResponseHeader { .. } + | RouterSelector::ResponseContext { .. } + | RouterSelector::ResponseStatus { .. } + | RouterSelector::OnGraphQLError { .. } + ), + super::Stage::ResponseField => false, + super::Stage::Error => matches!( + self, + RouterSelector::TraceId { .. } + | RouterSelector::StudioOperationId { .. } + | RouterSelector::OperationName { .. } + | RouterSelector::Baggage { .. } + | RouterSelector::Static(_) + | RouterSelector::Env { .. } + | RouterSelector::StaticField { .. } + | RouterSelector::ResponseContext { .. } + | RouterSelector::Error { .. } + ), + super::Stage::Drop => matches!( + self, + RouterSelector::Static(_) | RouterSelector::StaticField { .. } + ), + } + } } impl Selector for SupergraphSelector { @@ -1160,6 +1209,66 @@ impl Selector for SupergraphSelector { _ => None, } } + + fn is_active(&self, stage: super::Stage) -> bool { + match stage { + super::Stage::Request => matches!( + self, + SupergraphSelector::OperationName { .. } + | SupergraphSelector::OperationKind { .. } + | SupergraphSelector::Query { .. } + | SupergraphSelector::RequestHeader { .. } + | SupergraphSelector::QueryVariable { .. } + | SupergraphSelector::RequestContext { .. } + | SupergraphSelector::Baggage { .. } + | SupergraphSelector::Env { .. } + | SupergraphSelector::Static(_) + | SupergraphSelector::StaticField { .. } + ), + super::Stage::Response => matches!( + self, + SupergraphSelector::Query { .. } + | SupergraphSelector::ResponseHeader { .. } + | SupergraphSelector::ResponseStatus { .. } + | SupergraphSelector::ResponseContext { .. } + | SupergraphSelector::OnGraphQLError { .. } + | SupergraphSelector::OperationName { .. } + | SupergraphSelector::OperationKind { .. } + | SupergraphSelector::IsPrimaryResponse { .. } + | SupergraphSelector::Static(_) + | SupergraphSelector::StaticField { .. } + ), + super::Stage::ResponseEvent => matches!( + self, + SupergraphSelector::ResponseData { .. } + | SupergraphSelector::ResponseErrors { .. } + | SupergraphSelector::Cost { .. } + | SupergraphSelector::OnGraphQLError { .. } + | SupergraphSelector::OperationName { .. } + | SupergraphSelector::OperationKind { .. } + | SupergraphSelector::IsPrimaryResponse { .. } + | SupergraphSelector::ResponseContext { .. } + | SupergraphSelector::Static(_) + | SupergraphSelector::StaticField { .. } + ), + super::Stage::ResponseField => false, + super::Stage::Error => matches!( + self, + SupergraphSelector::OperationName { .. } + | SupergraphSelector::OperationKind { .. } + | SupergraphSelector::Query { .. } + | SupergraphSelector::Error { .. } + | SupergraphSelector::Static(_) + | SupergraphSelector::StaticField { .. } + | SupergraphSelector::ResponseContext { .. } + | SupergraphSelector::IsPrimaryResponse { .. } + ), + super::Stage::Drop => matches!( + self, + SupergraphSelector::Static(_) | SupergraphSelector::StaticField { .. } + ), + } + } } impl Selector for SubgraphSelector { @@ -1535,6 +1644,63 @@ impl Selector for SubgraphSelector { _ => None, } } + + fn is_active(&self, stage: super::Stage) -> bool { + match stage { + super::Stage::Request => matches!( + self, + SubgraphSelector::SubgraphOperationName { .. } + | SubgraphSelector::SupergraphOperationName { .. } + | SubgraphSelector::SubgraphName { .. } + | SubgraphSelector::SubgraphOperationKind { .. } + | SubgraphSelector::SupergraphOperationKind { .. } + | SubgraphSelector::SupergraphQuery { .. } + | SubgraphSelector::SubgraphQuery { .. } + | SubgraphSelector::SubgraphQueryVariable { .. } + | SubgraphSelector::SupergraphQueryVariable { .. } + | SubgraphSelector::SubgraphRequestHeader { .. } + | SubgraphSelector::SupergraphRequestHeader { .. } + | SubgraphSelector::RequestContext { .. } + | SubgraphSelector::Baggage { .. } + | SubgraphSelector::Env { .. } + | SubgraphSelector::Static(_) + | SubgraphSelector::StaticField { .. } + ), + super::Stage::Response => matches!( + self, + SubgraphSelector::SubgraphResponseHeader { .. } + | SubgraphSelector::SubgraphResponseStatus { .. } + | SubgraphSelector::SubgraphOperationKind { .. } + | SubgraphSelector::SupergraphOperationKind { .. } + | SubgraphSelector::SupergraphOperationName { .. } + | SubgraphSelector::SubgraphName { .. } + | SubgraphSelector::SubgraphResponseBody { .. } + | SubgraphSelector::SubgraphResponseData { .. } + | SubgraphSelector::SubgraphResponseErrors { .. } + | SubgraphSelector::ResponseContext { .. } + | SubgraphSelector::OnGraphQLError { .. } + | SubgraphSelector::Static(_) + | SubgraphSelector::StaticField { .. } + | SubgraphSelector::Cache { .. } + ), + super::Stage::ResponseEvent => false, + super::Stage::ResponseField => false, + super::Stage::Error => matches!( + self, + SubgraphSelector::SubgraphOperationKind { .. } + | SubgraphSelector::SupergraphOperationKind { .. } + | SubgraphSelector::SupergraphOperationName { .. } + | SubgraphSelector::Error { .. } + | SubgraphSelector::Static(_) + | SubgraphSelector::StaticField { .. } + | SubgraphSelector::ResponseContext { .. } + ), + super::Stage::Drop => matches!( + self, + SubgraphSelector::Static(_) | SubgraphSelector::StaticField { .. } + ), + } + } } #[cfg(test)] From 8f44ff55008fca44e8a249038999775f457e167e Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Thu, 1 Aug 2024 14:15:08 +0200 Subject: [PATCH 02/13] improve support of conditions at the request level, especially for events Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- .../telemetry/config_new/conditions.rs | 58 +++++++++++++------ .../src/plugins/telemetry/config_new/mod.rs | 1 + 2 files changed, 40 insertions(+), 19 deletions(-) diff --git a/apollo-router/src/plugins/telemetry/config_new/conditions.rs b/apollo-router/src/plugins/telemetry/config_new/conditions.rs index db75771ffe..0050928911 100644 --- a/apollo-router/src/plugins/telemetry/config_new/conditions.rs +++ b/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; @@ -57,26 +58,36 @@ where { pub(crate) fn evaluate_request(&mut self, request: &T::Request) -> Option { 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 - } - (Some(left), None) => { - eq[0] = SelectorOrValue::Value(left.into()); - None + Condition::Eq(eq) => { + if !eq[0].is_active(Stage::Request) && !eq[1].is_active(Stage::Request) { + // Nothing to compute here + return None; } - (Some(left), Some(right)) => { - if left == right { - *self = Condition::True; - Some(true) - } else { - Some(false) + 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) { @@ -101,6 +112,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) { @@ -125,9 +140,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 } @@ -309,6 +328,7 @@ where Condition::False => false, } } + pub(crate) fn evaluate_drop(&self) -> Option { match self { Condition::Eq(eq) => match (eq[0].on_drop(), eq[1].on_drop()) { diff --git a/apollo-router/src/plugins/telemetry/config_new/mod.rs b/apollo-router/src/plugins/telemetry/config_new/mod.rs index 41b04f942b..d6b713ad4c 100644 --- a/apollo-router/src/plugins/telemetry/config_new/mod.rs +++ b/apollo-router/src/plugins/telemetry/config_new/mod.rs @@ -51,6 +51,7 @@ pub(crate) trait Selectors { } } +#[allow(dead_code)] pub(crate) enum Stage { Request, Response, From 10c8e32c59a3a246dcd01b969c3854bd8a46b589 Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Thu, 1 Aug 2024 17:21:26 +0200 Subject: [PATCH 03/13] fix tests Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- apollo-router/src/plugins/telemetry/config_new/conditions.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/apollo-router/src/plugins/telemetry/config_new/conditions.rs b/apollo-router/src/plugins/telemetry/config_new/conditions.rs index 0050928911..f64547c767 100644 --- a/apollo-router/src/plugins/telemetry/config_new/conditions.rs +++ b/apollo-router/src/plugins/telemetry/config_new/conditions.rs @@ -580,7 +580,7 @@ mod test { #[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))); @@ -751,7 +751,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_eq!(exists(Resp).resp(None), false); 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); From f556154694fc89827900dda8d358fce3b104f770 Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Thu, 1 Aug 2024 17:22:21 +0200 Subject: [PATCH 04/13] fix lint Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- apollo-router/src/plugins/telemetry/config_new/conditions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apollo-router/src/plugins/telemetry/config_new/conditions.rs b/apollo-router/src/plugins/telemetry/config_new/conditions.rs index f64547c767..e99c940e82 100644 --- a/apollo-router/src/plugins/telemetry/config_new/conditions.rs +++ b/apollo-router/src/plugins/telemetry/config_new/conditions.rs @@ -752,7 +752,7 @@ mod test { assert_eq!(exists(Req).req(Some(1i64)), Some(true)); assert_eq!(exists(Req).req(None), Some(false)); - assert_eq!(exists(Resp).resp(None), false); + assert_eq!(!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); From a0a2a96d042dd385a768b788fd3ffa27f0667b84 Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Thu, 1 Aug 2024 17:22:33 +0200 Subject: [PATCH 05/13] fix lint Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- apollo-router/src/plugins/telemetry/config_new/conditions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apollo-router/src/plugins/telemetry/config_new/conditions.rs b/apollo-router/src/plugins/telemetry/config_new/conditions.rs index e99c940e82..3b3c606092 100644 --- a/apollo-router/src/plugins/telemetry/config_new/conditions.rs +++ b/apollo-router/src/plugins/telemetry/config_new/conditions.rs @@ -752,7 +752,7 @@ mod test { assert_eq!(exists(Req).req(Some(1i64)), Some(true)); assert_eq!(exists(Req).req(None), Some(false)); - assert_eq!(!exists(Resp).resp(None)); + 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); From 746429446b9af2e8c99282b55f671ae2ac19bd59 Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Fri, 2 Aug 2024 11:46:13 +0200 Subject: [PATCH 06/13] add test Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- .../plugins/telemetry/config_new/events.rs | 49 +++++++++++++++++++ ...aph_events_with_exists_condition@logs.snap | 22 +++++++++ ...custom_events_exists_condition.router.yaml | 13 +++++ 3 files changed, 84 insertions(+) create mode 100644 apollo-router/src/plugins/telemetry/config_new/snapshots/apollo_router__plugins__telemetry__config_new__events__tests__supergraph_events_with_exists_condition@logs.snap create mode 100644 apollo-router/src/plugins/telemetry/testdata/custom_events_exists_condition.router.yaml diff --git a/apollo-router/src/plugins/telemetry/config_new/events.rs b/apollo-router/src/plugins/telemetry/config_new/events.rs index bf471dc019..ff9979f00e 100644 --- a/apollo-router/src/plugins/telemetry/config_new/events.rs +++ b/apollo-router/src/plugins/telemetry/config_new/events.rs @@ -736,6 +736,7 @@ mod tests { use super::*; use crate::assert_snapshot_subscriber; use crate::context::CONTAINS_GRAPHQL_ERROR; + use crate::context::OPERATION_NAME; use crate::graphql; use crate::plugins::telemetry::Telemetry; use crate::plugins::test::PluginTestHarness; @@ -877,6 +878,54 @@ mod tests { .await } + #[tokio::test(flavor = "multi_thread")] + async fn test_supergraph_events_with_exists_condition() { + let test_harness: PluginTestHarness = PluginTestHarness::builder() + .config(include_str!( + "../testdata/custom_events_exists_condition.router.yaml" + )) + .build() + .await; + + async { + let ctx = Context::new(); + ctx.insert(OPERATION_NAME, String::from("Test")).unwrap(); + test_harness + .call_supergraph( + supergraph::Request::fake_builder() + .query("query Test { foo }") + .context(ctx) + .build() + .unwrap(), + |_r| { + supergraph::Response::fake_builder() + .data(serde_json::json!({"data": "res"}).to_string()) + .build() + .expect("expecting valid response") + }, + ) + .await + .expect("expecting successful response"); + test_harness + .call_supergraph( + supergraph::Request::fake_builder() + .query("query { foo }") + .build() + .unwrap(), + |_r| { + supergraph::Response::fake_builder() + .data(serde_json::json!({"data": "res"}).to_string()) + .build() + .expect("expecting valid response") + }, + ) + .await + .expect("expecting successful response"); + } + .with_subscriber(assert_snapshot_subscriber!()) + .await + } + #[tokio::test(flavor = "multi_thread")] async fn test_supergraph_events_on_graphql_error() { let test_harness: PluginTestHarness = PluginTestHarness::builder() diff --git a/apollo-router/src/plugins/telemetry/config_new/snapshots/apollo_router__plugins__telemetry__config_new__events__tests__supergraph_events_with_exists_condition@logs.snap b/apollo-router/src/plugins/telemetry/config_new/snapshots/apollo_router__plugins__telemetry__config_new__events__tests__supergraph_events_with_exists_condition@logs.snap new file mode 100644 index 0000000000..0c9630144c --- /dev/null +++ b/apollo-router/src/plugins/telemetry/config_new/snapshots/apollo_router__plugins__telemetry__config_new__events__tests__supergraph_events_with_exists_condition@logs.snap @@ -0,0 +1,22 @@ +--- +source: apollo-router/src/plugins/telemetry/config_new/events.rs +expression: yaml +--- +- fields: + kind: my.event + level: INFO + message: Auditing Router Event + span: + apollo_private.field_level_instrumentation_ratio: 0.01 + apollo_private.graphql.variables: "{}" + graphql.document: "query Test { foo }" + graphql.operation.name: Test + name: supergraph + otel.kind: INTERNAL + spans: + - apollo_private.field_level_instrumentation_ratio: 0.01 + apollo_private.graphql.variables: "{}" + graphql.document: "query Test { foo }" + graphql.operation.name: Test + name: supergraph + otel.kind: INTERNAL diff --git a/apollo-router/src/plugins/telemetry/testdata/custom_events_exists_condition.router.yaml b/apollo-router/src/plugins/telemetry/testdata/custom_events_exists_condition.router.yaml new file mode 100644 index 0000000000..0ee5b021f7 --- /dev/null +++ b/apollo-router/src/plugins/telemetry/testdata/custom_events_exists_condition.router.yaml @@ -0,0 +1,13 @@ +telemetry: + instrumentation: + events: + supergraph: + my.event: + message: "Auditing Router Event" + level: info + on: request + attributes: + graphql.operation.name: true + condition: + exists: + operation_name: string \ No newline at end of file From 5f63ce4f6063b68ac91781c07c7f8fb72fb9af0a Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Fri, 2 Aug 2024 14:07:06 +0200 Subject: [PATCH 07/13] changelog Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- .changesets/fix_bnjjj_fix_5702.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 .changesets/fix_bnjjj_fix_5702.md diff --git a/.changesets/fix_bnjjj_fix_5702.md b/.changesets/fix_bnjjj_fix_5702.md new file mode 100644 index 0000000000..14e662e6bb --- /dev/null +++ b/.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 \ No newline at end of file From 50756f66bac99442c63f04a27c4ff8fe0f0e1c35 Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Fri, 2 Aug 2024 17:21:24 +0200 Subject: [PATCH 08/13] add support of warning for configuration Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- apollo-router/src/plugins/telemetry/config.rs | 8 +++ .../telemetry/config_new/conditional.rs | 12 ++++ .../telemetry/config_new/conditions.rs | 58 +++++++++++++++++ .../plugins/telemetry/config_new/events.rs | 64 +++++++++++++++++++ .../telemetry/config_new/extendable.rs | 18 ++++++ .../telemetry/config_new/instruments.rs | 30 +++++++++ .../src/plugins/telemetry/config_new/mod.rs | 51 +++++++++++++++ .../src/plugins/telemetry/config_new/spans.rs | 20 ++++++ apollo-router/src/plugins/telemetry/mod.rs | 3 + 9 files changed, 264 insertions(+) diff --git a/apollo-router/src/plugins/telemetry/config.rs b/apollo-router/src/plugins/telemetry/config.rs index 1f36eb4c34..707a1bcc09 100644 --- a/apollo-router/src/plugins/telemetry/config.rs +++ b/apollo-router/src/plugins/telemetry/config.rs @@ -93,6 +93,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)] diff --git a/apollo-router/src/plugins/telemetry/config_new/conditional.rs b/apollo-router/src/plugins/telemetry/config_new/conditional.rs index dd36bfee5b..a42f112a8c 100644 --- a/apollo-router/src/plugins/telemetry/config_new/conditional.rs +++ b/apollo-router/src/plugins/telemetry/config_new/conditional.rs @@ -149,6 +149,18 @@ where } } +impl Conditional +where + Att: Selector, +{ + pub(crate) fn validate(&self) -> Result<(), String> { + match &self.condition { + Some(cond) => cond.lock().validate(None), + None => Ok(()), + } + } +} + impl Selector for Conditional where Att: Selector, diff --git a/apollo-router/src/plugins/telemetry/config_new/conditions.rs b/apollo-router/src/plugins/telemetry/config_new/conditions.rs index 3b3c606092..4c0faca4c8 100644 --- a/apollo-router/src/plugins/telemetry/config_new/conditions.rs +++ b/apollo-router/src/plugins/telemetry/config_new/conditions.rs @@ -56,6 +56,64 @@ impl Condition where T: Selector, { + /// 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) -> Result<(), String> { + match self { + 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")) + } + (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(String::from("can't compare a selector computed in another stage than 'request' because it won't be computed at all")); + } + } + Ok(()) + }, + (SelectorOrValue::Selector(sel1), SelectorOrValue::Selector(sel2)) => { + // Special condition for events + if let Some(Stage::Request) = &restricted_stage { + if !sel1.is_active(Stage::Request) || !sel2.is_active(Stage::Request) { + return Err(String::from("can't compare a selector computed in another stage than 'request' because it won't be computed at all")); + } + } + 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 { match self { Condition::Eq(eq) => { diff --git a/apollo-router/src/plugins/telemetry/config_new/events.rs b/apollo-router/src/plugins/telemetry/config_new/events.rs index ff9979f00e..a91067a0e2 100644 --- a/apollo-router/src/plugins/telemetry/config_new/events.rs +++ b/apollo-router/src/plugins/telemetry/config_new/events.rs @@ -14,6 +14,7 @@ use tracing::Span; use super::instruments::Instrumented; use super::Selector; use super::Selectors; +use super::Stage; use crate::plugins::telemetry::config_new::attributes::RouterAttributes; use crate::plugins::telemetry::config_new::attributes::SubgraphAttributes; use crate::plugins::telemetry::config_new::attributes::SupergraphAttributes; @@ -127,6 +128,54 @@ impl Events { custom: custom_events, } } + + pub(crate) fn validate(&self) -> Result<(), String> { + if let StandardEventConfig::Conditional { condition, .. } = &self.router.attributes.request + { + condition.validate(Some(Stage::Request))?; + } + if let StandardEventConfig::Conditional { condition, .. } = &self.router.attributes.response + { + condition.validate(Some(Stage::Response))?; + } + if let StandardEventConfig::Conditional { condition, .. } = + &self.supergraph.attributes.request + { + condition.validate(Some(Stage::Request))?; + } + if let StandardEventConfig::Conditional { condition, .. } = + &self.supergraph.attributes.response + { + condition.validate(Some(Stage::Response))?; + } + if let StandardEventConfig::Conditional { condition, .. } = + &self.subgraph.attributes.request + { + condition.validate(Some(Stage::Request))?; + } + if let StandardEventConfig::Conditional { condition, .. } = + &self.subgraph.attributes.response + { + condition.validate(Some(Stage::Response))?; + } + for (name, custom_event) in &self.router.custom { + custom_event.validate().map_err(|err| { + format!("configuration error for router custom event {name:?}: {err}") + })?; + } + for (name, custom_event) in &self.supergraph.custom { + custom_event.validate().map_err(|err| { + format!("configuration error for supergraph custom event {name:?}: {err}") + })?; + } + for (name, custom_event) in &self.subgraph.custom { + custom_event.validate().map_err(|err| { + format!("configuration error for subgraph custom event {name:?}: {err}") + })?; + } + + Ok(()) + } } pub(crate) type RouterEvents = @@ -576,6 +625,21 @@ where condition: Condition, } +impl Event +where + A: Selectors + + Default + + Debug, + E: Selector + Debug, +{ + pub(crate) fn validate(&self) -> Result<(), String> { + let stage = Some(self.on.into()); + self.attributes.validate(stage)?; + self.condition.validate(stage)?; + Ok(()) + } +} + /// When to trigger the event. #[derive(Deserialize, JsonSchema, Clone, Debug, Copy, PartialEq)] #[serde(rename_all = "snake_case")] diff --git a/apollo-router/src/plugins/telemetry/config_new/extendable.rs b/apollo-router/src/plugins/telemetry/config_new/extendable.rs index f3c1a4d332..66541ce8f3 100644 --- a/apollo-router/src/plugins/telemetry/config_new/extendable.rs +++ b/apollo-router/src/plugins/telemetry/config_new/extendable.rs @@ -17,6 +17,7 @@ use serde_json::Map; use serde_json::Value; use tower::BoxError; +use super::Stage; use crate::plugins::telemetry::config_new::attributes::DefaultAttributeRequirementLevel; use crate::plugins::telemetry::config_new::DefaultForLevel; use crate::plugins::telemetry::config_new::Selector; @@ -255,6 +256,23 @@ where } } +impl Extendable +where + A: Default + Selectors, + E: Selector, +{ + pub(crate) fn validate(&self, restricted_stage: Option) -> Result<(), String> { + if let Some(Stage::Request) = &restricted_stage { + for (name, custom) in &self.custom { + if !custom.is_active(Stage::Request) { + return Err(format!("cannot set the attribute {name:?} because it's using a selector computed in another stage than 'request' because it won't be computed")); + } + } + } + + Ok(()) + } +} #[cfg(test)] mod test { use std::sync::Arc; diff --git a/apollo-router/src/plugins/telemetry/config_new/instruments.rs b/apollo-router/src/plugins/telemetry/config_new/instruments.rs index 3b112a6f2d..341f84ad35 100644 --- a/apollo-router/src/plugins/telemetry/config_new/instruments.rs +++ b/apollo-router/src/plugins/telemetry/config_new/instruments.rs @@ -103,6 +103,36 @@ const HTTP_CLIENT_REQUEST_BODY_SIZE_METRIC: &str = "http.client.request.body.siz const HTTP_CLIENT_RESPONSE_BODY_SIZE_METRIC: &str = "http.client.response.body.size"; impl InstrumentsConfig { + pub(crate) fn validate(&self) -> Result<(), String> { + for (name, custom) in &self.router.custom { + custom.condition.validate(None).map_err(|err| { + format!("error for custom router instrument {name:?} in condition: {err}") + })?; + } + for (name, custom) in &self.supergraph.custom { + custom.condition.validate(None).map_err(|err| { + format!("error for custom supergraph instrument {name:?} in condition: {err}") + })?; + } + for (name, custom) in &self.subgraph.custom { + custom.condition.validate(None).map_err(|err| { + format!("error for custom subgraph instrument {name:?} in condition: {err}") + })?; + } + for (name, custom) in &self.graphql.custom { + custom.condition.validate(None).map_err(|err| { + format!("error for custom graphql instrument {name:?} in condition: {err}") + })?; + } + for (name, custom) in &self.cache.custom { + custom.condition.validate(None).map_err(|err| { + format!("error for custom cache instrument {name:?} in condition: {err}") + })?; + } + + Ok(()) + } + /// Update the defaults for spans configuration regarding the `default_attribute_requirement_level` pub(crate) fn update_defaults(&mut self) { self.router diff --git a/apollo-router/src/plugins/telemetry/config_new/mod.rs b/apollo-router/src/plugins/telemetry/config_new/mod.rs index d6b713ad4c..d63d586d0a 100644 --- a/apollo-router/src/plugins/telemetry/config_new/mod.rs +++ b/apollo-router/src/plugins/telemetry/config_new/mod.rs @@ -1,3 +1,4 @@ +use events::EventOn; use opentelemetry::baggage::BaggageExt; use opentelemetry::trace::TraceContextExt; use opentelemetry::trace::TraceId; @@ -52,6 +53,7 @@ pub(crate) trait Selectors { } #[allow(dead_code)] +#[derive(Clone, Copy, Debug, PartialEq)] pub(crate) enum Stage { Request, Response, @@ -61,6 +63,55 @@ pub(crate) enum Stage { Drop, } +impl std::fmt::Display for Stage { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Stage::Request => write!(f, "request"), + Stage::Response => write!(f, "response"), + Stage::ResponseEvent => write!(f, "response_event"), + Stage::ResponseField => write!(f, "response_field"), + Stage::Error => write!(f, "error"), + Stage::Drop => write!(f, "drop"), + } + } +} + +impl From for Stage { + fn from(value: EventOn) -> Self { + match value { + EventOn::Request => Self::Request, + EventOn::Response => Self::Response, + EventOn::EventResponse => Self::ResponseEvent, + EventOn::Error => Self::Error, + } + } +} + +impl PartialOrd for Stage { + fn partial_cmp(&self, other: &Self) -> Option { + match self { + Stage::Request => { + if *other != Self::Request { + Some(std::cmp::Ordering::Less) + } else { + Some(std::cmp::Ordering::Equal) + } + } + Stage::Response + | Stage::ResponseEvent + | Stage::ResponseField + | Stage::Error + | Stage::Drop => { + if *other != Self::Request { + Some(std::cmp::Ordering::Greater) + } else { + Some(std::cmp::Ordering::Less) + } + } + } + } +} + pub(crate) trait Selector { type Request; type Response; diff --git a/apollo-router/src/plugins/telemetry/config_new/spans.rs b/apollo-router/src/plugins/telemetry/config_new/spans.rs index ff4a3b00a0..61dfb0f35c 100644 --- a/apollo-router/src/plugins/telemetry/config_new/spans.rs +++ b/apollo-router/src/plugins/telemetry/config_new/spans.rs @@ -53,6 +53,26 @@ impl Spans { TelemetryDataKind::Traces, ); } + + pub(crate) fn validate(&self) -> Result<(), String> { + for (name, custom) in &self.router.attributes.custom { + custom + .validate() + .map_err(|err| format!("error for router span attribute {name:?}: {err}"))?; + } + for (name, custom) in &self.supergraph.attributes.custom { + custom + .validate() + .map_err(|err| format!("error for supergraph span attribute {name:?}: {err}"))?; + } + for (name, custom) in &self.subgraph.attributes.custom { + custom + .validate() + .map_err(|err| format!("error for subgraph span attribute {name:?}: {err}"))?; + } + + Ok(()) + } } #[derive(Deserialize, JsonSchema, Clone, Debug, Default)] diff --git a/apollo-router/src/plugins/telemetry/mod.rs b/apollo-router/src/plugins/telemetry/mod.rs index 42b34cbfae..ed227119e7 100644 --- a/apollo-router/src/plugins/telemetry/mod.rs +++ b/apollo-router/src/plugins/telemetry/mod.rs @@ -289,6 +289,9 @@ impl Plugin for Telemetry { config.instrumentation.spans.update_defaults(); config.instrumentation.instruments.update_defaults(); config.exporters.logging.validate()?; + if let Err(err) = config.instrumentation.validate() { + ::tracing::warn!("Potential configuration error for 'instrumentation': {err}, please check the documentation on https://www.apollographql.com/docs/router/configuration/telemetry/instrumentation/events"); + } let field_level_instrumentation_ratio = config.calculate_field_level_instrumentation_ratio()?; From e4476a37a4fd4a8076444122a29eaa334741e302 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Mon, 12 Aug 2024 09:32:20 +0200 Subject: [PATCH 09/13] lint --- apollo-router/src/plugins/telemetry/config_new/conditions.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/apollo-router/src/plugins/telemetry/config_new/conditions.rs b/apollo-router/src/plugins/telemetry/config_new/conditions.rs index 4c0faca4c8..497e3e3879 100644 --- a/apollo-router/src/plugins/telemetry/config_new/conditions.rs +++ b/apollo-router/src/plugins/telemetry/config_new/conditions.rs @@ -81,7 +81,6 @@ where } Ok(()) }, - }, Condition::Exists(sel) => { match restricted_stage { From b4086a0ace1f9e02eafdd96f20aae6f80d6bd2ac Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Tue, 20 Aug 2024 12:10:22 +0200 Subject: [PATCH 10/13] add tests Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- .../telemetry/config_new/conditions.rs | 25 +++++++++++++++++-- .../telemetry/config_new/extendable.rs | 2 +- .../src/plugins/telemetry/config_new/mod.rs | 25 ------------------- 3 files changed, 24 insertions(+), 28 deletions(-) diff --git a/apollo-router/src/plugins/telemetry/config_new/conditions.rs b/apollo-router/src/plugins/telemetry/config_new/conditions.rs index 519bb5d56d..5e53959244 100644 --- a/apollo-router/src/plugins/telemetry/config_new/conditions.rs +++ b/apollo-router/src/plugins/telemetry/config_new/conditions.rs @@ -579,6 +579,7 @@ 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; enum TestSelector { @@ -653,8 +654,12 @@ mod test { } } - fn is_active(&self, _stage: crate::plugins::telemetry::config_new::Stage) -> bool { - true + 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, + } } } @@ -850,6 +855,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()); diff --git a/apollo-router/src/plugins/telemetry/config_new/extendable.rs b/apollo-router/src/plugins/telemetry/config_new/extendable.rs index 66541ce8f3..6af5d2bf1c 100644 --- a/apollo-router/src/plugins/telemetry/config_new/extendable.rs +++ b/apollo-router/src/plugins/telemetry/config_new/extendable.rs @@ -265,7 +265,7 @@ where if let Some(Stage::Request) = &restricted_stage { for (name, custom) in &self.custom { if !custom.is_active(Stage::Request) { - return Err(format!("cannot set the attribute {name:?} because it's using a selector computed in another stage than 'request' because it won't be computed")); + return Err(format!("cannot set the attribute {name:?} because it is using a selector computed in another stage than 'request' so it will not be computed")); } } } diff --git a/apollo-router/src/plugins/telemetry/config_new/mod.rs b/apollo-router/src/plugins/telemetry/config_new/mod.rs index d63d586d0a..10cc7fb26f 100644 --- a/apollo-router/src/plugins/telemetry/config_new/mod.rs +++ b/apollo-router/src/plugins/telemetry/config_new/mod.rs @@ -87,31 +87,6 @@ impl From for Stage { } } -impl PartialOrd for Stage { - fn partial_cmp(&self, other: &Self) -> Option { - match self { - Stage::Request => { - if *other != Self::Request { - Some(std::cmp::Ordering::Less) - } else { - Some(std::cmp::Ordering::Equal) - } - } - Stage::Response - | Stage::ResponseEvent - | Stage::ResponseField - | Stage::Error - | Stage::Drop => { - if *other != Self::Request { - Some(std::cmp::Ordering::Greater) - } else { - Some(std::cmp::Ordering::Less) - } - } - } - } -} - pub(crate) trait Selector { type Request; type Response; From aeb1917c0ef9ed87101d98143cdaa3e8dcbf39a1 Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Tue, 20 Aug 2024 14:30:29 +0200 Subject: [PATCH 11/13] lint Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- apollo-router/src/plugins/telemetry/config_new/conditions.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/apollo-router/src/plugins/telemetry/config_new/conditions.rs b/apollo-router/src/plugins/telemetry/config_new/conditions.rs index b5b99aab3f..fd6114852b 100644 --- a/apollo-router/src/plugins/telemetry/config_new/conditions.rs +++ b/apollo-router/src/plugins/telemetry/config_new/conditions.rs @@ -656,7 +656,10 @@ mod test { 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), + Resp => matches!( + stage, + Stage::Response | Stage::ResponseEvent | Stage::ResponseField + ), Static(_) => true, } } From 5ef90f4a479addf64f4ea7c7fd713c41d9784efb Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Tue, 20 Aug 2024 15:46:20 +0200 Subject: [PATCH 12/13] add changeset Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- .../feat_candle_exhale_deodorant_weeds.md | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 .changesets/feat_candle_exhale_deodorant_weeds.md diff --git a/.changesets/feat_candle_exhale_deodorant_weeds.md b/.changesets/feat_candle_exhale_deodorant_weeds.md new file mode 100644 index 0000000000..2e248b32bc --- /dev/null +++ b/.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 \ No newline at end of file From b96552485e29fa9a1644e494f49aff46e65406f4 Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Tue, 20 Aug 2024 17:52:17 +0200 Subject: [PATCH 13/13] lint changeset Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- .changesets/feat_candle_exhale_deodorant_weeds.md | 4 ++-- .../src/plugins/telemetry/config_new/conditions.rs | 12 ++++++++---- .../src/plugins/telemetry/config_new/mod.rs | 2 +- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/.changesets/feat_candle_exhale_deodorant_weeds.md b/.changesets/feat_candle_exhale_deodorant_weeds.md index 2e248b32bc..ed3f311569 100644 --- a/.changesets/feat_candle_exhale_deodorant_weeds.md +++ b/.changesets/feat_candle_exhale_deodorant_weeds.md @@ -13,11 +13,11 @@ telemetry: 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 + 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 + - 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. diff --git a/apollo-router/src/plugins/telemetry/config_new/conditions.rs b/apollo-router/src/plugins/telemetry/config_new/conditions.rs index fd6114852b..915fad6135 100644 --- a/apollo-router/src/plugins/telemetry/config_new/conditions.rs +++ b/apollo-router/src/plugins/telemetry/config_new/conditions.rs @@ -67,7 +67,7 @@ where // Special condition for events if let Some(Stage::Request) = &restricted_stage { if !sel.is_active(Stage::Request) { - return Err(String::from("can't compare a selector computed in another stage than 'request' because it won't be computed at all")); + return Err(format!("selector {sel:?} is only valid for request stage, this log event will never trigger")); } } Ok(()) @@ -75,8 +75,11 @@ where (SelectorOrValue::Selector(sel1), SelectorOrValue::Selector(sel2)) => { // Special condition for events if let Some(Stage::Request) = &restricted_stage { - if !sel1.is_active(Stage::Request) || !sel2.is_active(Stage::Request) { - return Err(String::from("can't compare a selector computed in another stage than 'request' because it won't be computed at all")); + 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(()) @@ -88,7 +91,7 @@ where 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)) + Err(format!("the 'exists' condition use a selector applied at the wrong stage, this condition will be executed at the {} stage", stage)) } }, None => Ok(()) @@ -581,6 +584,7 @@ mod test { use crate::plugins::telemetry::config_new::Stage; use crate::Context; + #[derive(Debug)] enum TestSelector { Req, Resp, diff --git a/apollo-router/src/plugins/telemetry/config_new/mod.rs b/apollo-router/src/plugins/telemetry/config_new/mod.rs index 10cc7fb26f..082d0a438e 100644 --- a/apollo-router/src/plugins/telemetry/config_new/mod.rs +++ b/apollo-router/src/plugins/telemetry/config_new/mod.rs @@ -87,7 +87,7 @@ impl From for Stage { } } -pub(crate) trait Selector { +pub(crate) trait Selector: std::fmt::Debug { type Request; type Response; type EventResponse;