-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add expensive op extension #3181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 7 commits
3e3e35d
1282c20
255c7cd
76eb195
1223dc6
aa5581c
c50c477
b871e89
cd47895
af74930
482b098
301fa3c
c57d35a
c36556c
5caa22b
b14aca9
bd06491
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Add expensive op extension for full block queries on GraphQL |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| use async_graphql::{ | ||
| Response, | ||
| ServerError, | ||
| extensions::{ | ||
| Extension, | ||
| ExtensionContext, | ||
| ExtensionFactory, | ||
| NextExecute, | ||
| }, | ||
| }; | ||
| use std::{ | ||
| sync::Arc, | ||
| time::Duration, | ||
| }; | ||
| use tokio::sync::Semaphore; | ||
|
|
||
| pub struct ExpensiveOpGuardFactory { | ||
| expensive_op_names: Arc<[String]>, | ||
| semaphore: Arc<Semaphore>, | ||
| timeout: Duration, | ||
| } | ||
|
|
||
| impl ExpensiveOpGuardFactory { | ||
| pub fn new( | ||
| expensive_op_names: Arc<[String]>, | ||
| max_in_flight: usize, | ||
| timeout: Duration, | ||
| ) -> Self { | ||
| Self { | ||
| expensive_op_names, | ||
| semaphore: Arc::new(Semaphore::new(max_in_flight)), | ||
| timeout, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl ExtensionFactory for ExpensiveOpGuardFactory { | ||
| fn create(&self) -> Arc<dyn Extension> { | ||
| Arc::new(ExpensiveOpGuard { | ||
| expensive_op_names: self.expensive_op_names.clone(), | ||
| semaphore: self.semaphore.clone(), | ||
| timeout: self.timeout, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| pub struct ExpensiveOpGuard { | ||
| expensive_op_names: Arc<[String]>, | ||
| semaphore: Arc<Semaphore>, | ||
| timeout: Duration, | ||
| } | ||
|
|
||
| #[async_trait::async_trait] | ||
| impl Extension for ExpensiveOpGuard { | ||
| async fn execute( | ||
| &self, | ||
| ctx: &ExtensionContext<'_>, | ||
| operation_name: Option<&str>, | ||
| next: NextExecute<'_>, | ||
| ) -> Response { | ||
| let op = operation_name.clone().unwrap_or_default(); | ||
| let is_expensive = self.expensive_op_names.iter().any(|name| op == name); | ||
cursor[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| tracing::debug!( | ||
| "Executing operation: {:?}, and expected one of {:?}, expensive: {:?}, timeout: {:?}, semaphore_size: {:?}", | ||
| operation_name, | ||
| self.expensive_op_names, | ||
| is_expensive, | ||
| self.timeout, | ||
| self.semaphore.available_permits(), | ||
| ); | ||
|
|
||
| if !is_expensive { | ||
| return next.run(ctx, operation_name).await; | ||
| } | ||
|
|
||
| // Concurrency gate (bulkhead) | ||
| let permit = match self.semaphore.clone().try_acquire_owned() { | ||
| Ok(p) => p, | ||
| Err(_) => { | ||
| let mut resp = Response::new(async_graphql::Value::Null); | ||
| resp.errors.push(ServerError::new( | ||
| "Rate limit exceeded for this operation", | ||
| None, | ||
| )); | ||
| return resp; | ||
| } | ||
| }; | ||
|
|
||
| // Time bound (avoid request pile-ups) | ||
| let fut = next.run(ctx, operation_name); | ||
| let starting_time = tokio::time::Instant::now(); | ||
| let out = tokio::time::timeout(self.timeout, fut).await; | ||
| tracing::warn!( | ||
| "finished executing in {:?}ns, success: {:?}", | ||
| starting_time.elapsed().as_nanos(), | ||
|
||
| out.is_ok(), | ||
| ); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Warn-level logging on every expensive request executionMedium Severity
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Log message appends spurious "ns" after Duration debug formatLow Severity The format string |
||
|
|
||
| drop(permit); | ||
|
|
||
| match out { | ||
| Ok(resp) => resp, | ||
| Err(_) => { | ||
| let mut resp = Response::new(async_graphql::Value::Null); | ||
| resp.errors | ||
| .push(ServerError::new("Operation timed out", None)); | ||
| resp | ||
| } | ||
| } | ||
| } | ||
| } | ||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be nice if this was optional for local envs, but just setting a high limit should be fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into this, it makes the code a bit more complicated. Still worth considering, but not prioritizing now.