diff --git a/.changesets/feat_candle_exhale_deodorant_weeds.md b/.changesets/feat_candle_exhale_deodorant_weeds.md new file mode 100644 index 0000000000..ed3f311569 --- /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 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 diff --git a/apollo-router/src/plugins/telemetry/config.rs b/apollo-router/src/plugins/telemetry/config.rs index f360774b9b..dba6f207f4 100644 --- a/apollo-router/src/plugins/telemetry/config.rs +++ b/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)] diff --git a/apollo-router/src/plugins/telemetry/config_new/conditional.rs b/apollo-router/src/plugins/telemetry/config_new/conditional.rs index 94136ee63d..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, @@ -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. diff --git a/apollo-router/src/plugins/telemetry/config_new/conditions.rs b/apollo-router/src/plugins/telemetry/config_new/conditions.rs index d3c44610a0..915fad6135 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; @@ -55,28 +56,98 @@ impl Condition where T: Selector, { - pub(crate) fn evaluate_request(&mut self, request: &T::Request) -> Option { + /// 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(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 { + 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 { 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()); diff --git a/apollo-router/src/plugins/telemetry/config_new/events.rs b/apollo-router/src/plugins/telemetry/config_new/events.rs index bf471dc019..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")] @@ -736,6 +800,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 +942,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/extendable.rs b/apollo-router/src/plugins/telemetry/config_new/extendable.rs index f3c1a4d332..6af5d2bf1c 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 is using a selector computed in another stage than 'request' so it will not be computed")); + } + } + } + + Ok(()) + } +} #[cfg(test)] mod test { use std::sync::Arc; 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/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 2a3f46edcf..082d0a438e 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; @@ -51,7 +52,42 @@ pub(crate) trait Selectors { } } -pub(crate) trait Selector { +#[allow(dead_code)] +#[derive(Clone, Copy, Debug, PartialEq)] +pub(crate) enum Stage { + Request, + Response, + ResponseEvent, + ResponseField, + Error, + 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, + } + } +} + +pub(crate) trait Selector: std::fmt::Debug { type Request; type Response; type EventResponse; @@ -79,6 +115,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 2f8aa1faf6..9047764a80 100644 --- a/apollo-router/src/plugins/telemetry/config_new/selectors.rs +++ b/apollo-router/src/plugins/telemetry/config_new/selectors.rs @@ -805,6 +805,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 { @@ -1173,6 +1222,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 { @@ -1548,6 +1657,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)] 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/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 8b2aefe4a9..345d1936ee 100644 --- a/apollo-router/src/plugins/telemetry/mod.rs +++ b/apollo-router/src/plugins/telemetry/mod.rs @@ -290,6 +290,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()?; 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