From f86456b2f167ec9dfa12f8cd5d11ea45cba3d076 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Wed, 10 Jul 2024 22:00:53 +0300 Subject: [PATCH] Revert "Revert "use spawn_blocking for parsing" (#5643)" This reverts commit 31f90b26b7b4b98b7e25e50bf5e6a429dea3cc36. --- .changesets/fix_spawn_blocking_parser.md | 5 ++ .../query_planner/caching_query_planner.rs | 10 +++- .../src/services/layers/query_analysis.rs | 28 +++++++++-- ...ollo_otel_traces__batch_send_header-2.snap | 48 +++++++++++++++++++ ...apollo_otel_traces__batch_send_header.snap | 48 +++++++++++++++++++ .../apollo_otel_traces__batch_trace_id-2.snap | 48 +++++++++++++++++++ .../apollo_otel_traces__batch_trace_id.snap | 48 +++++++++++++++++++ 7 files changed, 229 insertions(+), 6 deletions(-) create mode 100644 .changesets/fix_spawn_blocking_parser.md diff --git a/.changesets/fix_spawn_blocking_parser.md b/.changesets/fix_spawn_blocking_parser.md new file mode 100644 index 0000000000..0fd442df94 --- /dev/null +++ b/.changesets/fix_spawn_blocking_parser.md @@ -0,0 +1,5 @@ +### use spawn_blocking for query parsing & validation ([PR #5235](https://github.com/apollographql/router/pull/5235)) + +Moves query parsing and validation in a tokio blocking task to prevent all executor threads from blocking on large queries. + +By [@xuorig](https://github.com/xuorig) in https://github.com/apollographql/router/pull/5235 \ No newline at end of file diff --git a/apollo-router/src/query_planner/caching_query_planner.rs b/apollo-router/src/query_planner/caching_query_planner.rs index ff28cbc4c5..a409df3756 100644 --- a/apollo-router/src/query_planner/caching_query_planner.rs +++ b/apollo-router/src/query_planner/caching_query_planner.rs @@ -269,7 +269,10 @@ where } in all_cache_keys { let context = Context::new(); - let doc = match query_analysis.parse_document(&query, operation.as_deref()) { + let doc = match query_analysis + .parse_document(&query, operation.as_deref()) + .await + { Ok(doc) => doc, Err(_) => continue, }; @@ -310,7 +313,10 @@ where }) .await; if entry.is_first() { - let doc = match query_analysis.parse_document(&query, operation.as_deref()) { + let doc = match query_analysis + .parse_document(&query, operation.as_deref()) + .await + { Ok(doc) => doc, Err(error) => { let e = Arc::new(QueryPlannerError::SpecError(error)); diff --git a/apollo-router/src/services/layers/query_analysis.rs b/apollo-router/src/services/layers/query_analysis.rs index 4beb5a1e21..71eae898e7 100644 --- a/apollo-router/src/services/layers/query_analysis.rs +++ b/apollo-router/src/services/layers/query_analysis.rs @@ -11,6 +11,7 @@ use http::StatusCode; use lru::LruCache; use router_bridge::planner::UsageReporting; use tokio::sync::Mutex; +use tokio::task; use crate::apollo_studio_interop::generate_extended_references; use crate::apollo_studio_interop::ExtendedReferenceStats; @@ -73,12 +74,32 @@ impl QueryAnalysisLayer { } } - pub(crate) fn parse_document( + pub(crate) async fn parse_document( &self, query: &str, operation_name: Option<&str>, ) -> Result { - Query::parse_document(query, operation_name, &self.schema, &self.configuration) + let query = query.to_string(); + let operation_name = operation_name.map(|o| o.to_string()); + let schema = self.schema.clone(); + let conf = self.configuration.clone(); + + // Must be created *outside* of the spawn_blocking or the span is not connected to the + // parent + let span = tracing::info_span!(QUERY_PARSING_SPAN_NAME, "otel.kind" = "INTERNAL"); + + task::spawn_blocking(move || { + span.in_scope(|| { + Query::parse_document( + &query, + operation_name.as_deref(), + schema.as_ref(), + conf.as_ref(), + ) + }) + }) + .await + .expect("parse_document task panicked") } pub(crate) async fn supergraph_request( @@ -127,8 +148,7 @@ impl QueryAnalysisLayer { let res = match entry { None => { - let span = tracing::info_span!(QUERY_PARSING_SPAN_NAME, "otel.kind" = "INTERNAL"); - match span.in_scope(|| self.parse_document(&query, op_name.as_deref())) { + match self.parse_document(&query, op_name.as_deref()).await { Err(errors) => { (*self.cache.lock().await).put( QueryAnalysisKey { diff --git a/apollo-router/tests/snapshots/apollo_otel_traces__batch_send_header-2.snap b/apollo-router/tests/snapshots/apollo_otel_traces__batch_send_header-2.snap index a573b5890f..c1fe319e7a 100644 --- a/apollo-router/tests/snapshots/apollo_otel_traces__batch_send_header-2.snap +++ b/apollo-router/tests/snapshots/apollo_otel_traces__batch_send_header-2.snap @@ -228,6 +228,54 @@ resourceSpans: code: 0 schemaUrl: "" schemaUrl: "" + - resource: + attributes: + - key: apollo.client.host + value: + stringValue: "[redacted]" + - key: apollo.client.uname + value: + stringValue: "[redacted]" + - key: apollo.graph.ref + value: + stringValue: test + - key: apollo.router.id + value: + stringValue: "[redacted]" + - key: apollo.schema.id + value: + stringValue: "[redacted]" + - key: apollo.user.agent + value: + stringValue: "[redacted]" + droppedAttributesCount: 0 + scopeSpans: + - scope: + name: apollo-router + version: "[version]" + attributes: [] + droppedAttributesCount: 0 + spans: + - traceId: "[trace_id]" + spanId: "[span_id]" + traceState: "" + parentSpanId: "[span_id]" + flags: 0 + name: parse_query + kind: 1 + startTimeUnixNano: "[start_time]" + endTimeUnixNano: "[end_time]" + attributes: [] + droppedAttributesCount: 0 + events: [] + droppedEventsCount: 0 + links: [] + droppedLinksCount: 0 + status: + message: "" + code: 0 + schemaUrl: "" + schemaUrl: "" - resource: attributes: - key: apollo.client.host diff --git a/apollo-router/tests/snapshots/apollo_otel_traces__batch_send_header.snap b/apollo-router/tests/snapshots/apollo_otel_traces__batch_send_header.snap index a573b5890f..c1fe319e7a 100644 --- a/apollo-router/tests/snapshots/apollo_otel_traces__batch_send_header.snap +++ b/apollo-router/tests/snapshots/apollo_otel_traces__batch_send_header.snap @@ -228,6 +228,54 @@ resourceSpans: code: 0 schemaUrl: "" schemaUrl: "" + - resource: + attributes: + - key: apollo.client.host + value: + stringValue: "[redacted]" + - key: apollo.client.uname + value: + stringValue: "[redacted]" + - key: apollo.graph.ref + value: + stringValue: test + - key: apollo.router.id + value: + stringValue: "[redacted]" + - key: apollo.schema.id + value: + stringValue: "[redacted]" + - key: apollo.user.agent + value: + stringValue: "[redacted]" + droppedAttributesCount: 0 + scopeSpans: + - scope: + name: apollo-router + version: "[version]" + attributes: [] + droppedAttributesCount: 0 + spans: + - traceId: "[trace_id]" + spanId: "[span_id]" + traceState: "" + parentSpanId: "[span_id]" + flags: 0 + name: parse_query + kind: 1 + startTimeUnixNano: "[start_time]" + endTimeUnixNano: "[end_time]" + attributes: [] + droppedAttributesCount: 0 + events: [] + droppedEventsCount: 0 + links: [] + droppedLinksCount: 0 + status: + message: "" + code: 0 + schemaUrl: "" + schemaUrl: "" - resource: attributes: - key: apollo.client.host diff --git a/apollo-router/tests/snapshots/apollo_otel_traces__batch_trace_id-2.snap b/apollo-router/tests/snapshots/apollo_otel_traces__batch_trace_id-2.snap index 9db10c85be..4e1e59aac6 100644 --- a/apollo-router/tests/snapshots/apollo_otel_traces__batch_trace_id-2.snap +++ b/apollo-router/tests/snapshots/apollo_otel_traces__batch_trace_id-2.snap @@ -228,6 +228,54 @@ resourceSpans: code: 0 schemaUrl: "" schemaUrl: "" + - resource: + attributes: + - key: apollo.client.host + value: + stringValue: "[redacted]" + - key: apollo.client.uname + value: + stringValue: "[redacted]" + - key: apollo.graph.ref + value: + stringValue: test + - key: apollo.router.id + value: + stringValue: "[redacted]" + - key: apollo.schema.id + value: + stringValue: "[redacted]" + - key: apollo.user.agent + value: + stringValue: "[redacted]" + droppedAttributesCount: 0 + scopeSpans: + - scope: + name: apollo-router + version: "[version]" + attributes: [] + droppedAttributesCount: 0 + spans: + - traceId: "[trace_id]" + spanId: "[span_id]" + traceState: "" + parentSpanId: "[span_id]" + flags: 0 + name: parse_query + kind: 1 + startTimeUnixNano: "[start_time]" + endTimeUnixNano: "[end_time]" + attributes: [] + droppedAttributesCount: 0 + events: [] + droppedEventsCount: 0 + links: [] + droppedLinksCount: 0 + status: + message: "" + code: 0 + schemaUrl: "" + schemaUrl: "" - resource: attributes: - key: apollo.client.host diff --git a/apollo-router/tests/snapshots/apollo_otel_traces__batch_trace_id.snap b/apollo-router/tests/snapshots/apollo_otel_traces__batch_trace_id.snap index 9db10c85be..4e1e59aac6 100644 --- a/apollo-router/tests/snapshots/apollo_otel_traces__batch_trace_id.snap +++ b/apollo-router/tests/snapshots/apollo_otel_traces__batch_trace_id.snap @@ -228,6 +228,54 @@ resourceSpans: code: 0 schemaUrl: "" schemaUrl: "" + - resource: + attributes: + - key: apollo.client.host + value: + stringValue: "[redacted]" + - key: apollo.client.uname + value: + stringValue: "[redacted]" + - key: apollo.graph.ref + value: + stringValue: test + - key: apollo.router.id + value: + stringValue: "[redacted]" + - key: apollo.schema.id + value: + stringValue: "[redacted]" + - key: apollo.user.agent + value: + stringValue: "[redacted]" + droppedAttributesCount: 0 + scopeSpans: + - scope: + name: apollo-router + version: "[version]" + attributes: [] + droppedAttributesCount: 0 + spans: + - traceId: "[trace_id]" + spanId: "[span_id]" + traceState: "" + parentSpanId: "[span_id]" + flags: 0 + name: parse_query + kind: 1 + startTimeUnixNano: "[start_time]" + endTimeUnixNano: "[end_time]" + attributes: [] + droppedAttributesCount: 0 + events: [] + droppedEventsCount: 0 + links: [] + droppedLinksCount: 0 + status: + message: "" + code: 0 + schemaUrl: "" + schemaUrl: "" - resource: attributes: - key: apollo.client.host