Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: CLI arguments for configuring GraphQL query costs. #2335

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

feat: CLI arguments for configuring GraphQL query costs. #2335

wants to merge 5 commits into from
+325 −93

Conversation

netrome
Copy link
Contributor

@netrome netrome commented Oct 11, 2024 •
edited
Loading

Linked Issues/PRs

Description

This PR adds CLI options to configure graphql query costs dynamically.

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests

Before requesting review

  • I have reviewed the code myself

After merging, notify other teams

[Add or remove entries as needed]

@netrome netrome linked an issue Oct 11, 2024 that may be closed by this pull request
Make all limits of the of the complexity to be configurable via the CLI #2318
Open
@netrome netrome changed the base branch from release/v0.38.0 to master October 11, 2024 22:20
@netrome netrome force-pushed the 2318-make-all-limits-of-the-of-the-complexity-to-be-configurable-via-the-cli branch 2 times, most recently from 7bf18ce to ec34236 Compare October 11, 2024 22:25
@netrome netrome requested a review from a team October 11, 2024 22:26
@netrome netrome marked this pull request as ready for review October 11, 2024 22:26
rymnc
rymnc reviewed Oct 14, 2024
View reviewed changes
tests/tests/dos.rs Outdated Show resolved Hide resolved
rymnc
rymnc reviewed Oct 14, 2024
View reviewed changes
tests/tests/dos.rs Outdated Show resolved Hide resolved
rymnc
rymnc reviewed Oct 14, 2024
View reviewed changes
Copy link
Member

@rymnc rymnc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments and queries

@rymnc
Copy link
Member

rymnc commented Oct 14, 2024 •
edited
Loading

wonder why the ci is failing for the test included in this pr. lgtm otherwise, the test passes locally.

@netrome
Copy link
Contributor Author

netrome commented Oct 14, 2024

wonder why the ci is failing for the test included in this pr. lgtm otherwise, the test passes locally.

Yeah, I'll investigate it. It was the same with the previous parameters so something else is going on here but I have no idea what.

@netrome
Copy link
Contributor Author

netrome commented Oct 14, 2024

Yeah, I'll investigate it. It was the same with the previous parameters so something else is going on here but I have no idea what.

Reproduced locally now. The test only fails for me with libtest and not nextest when running cargo test complex_queries - so it seems to be some sort of interference between the tests. Running it in isolation succeeds with both libtest and nextest. I suspect that we're querying a node initialized in another test.

@netrome
Copy link
Contributor Author

netrome commented Oct 14, 2024

Reproduced locally now. The test only fails for me with libtest and not nextest when running cargo test complex_queries - so it seems to be some sort of interference between the tests. Running it in isolation succeeds with both libtest and nextest. I suspect that we're querying a node initialized in another test.

Alright, so the problem is that libtest will initialize QUERY_COSTS once for the entire test suite.

acerone85 and others added 5 commits October 14, 2024 13:54
@acerone85 @netrome
feat: CLI arguments for configuring GraphQL query costs.
c4329f8 
Co-authored-by: Mårten Blankfors <marten@blankfors.se>
@netrome
refactor: Address review comments
ceb43f6 
* Move query costs initialization function
* Feature gate default implementation on "test-helpers" feature
* Only set block header query cost in test
@netrome
fix: Use RwLock instead of OnceLock to allow writing new values (e.g.…
bc09dc4 
… for tests)
@netrome
Revert "fix: Use RwLock instead of OnceLock to allow writing new valu…
93dd831 
…es (e.g. for tests)"

This reverts commit 35c7214.
@netrome
feat: Return error in tests if attempting multiple initializations
645db49
@netrome netrome force-pushed the 2318-make-all-limits-of-the-of-the-complexity-to-be-configurable-via-the-cli branch from 8f93f4f to 645db49 Compare October 14, 2024 11:55
@netrome netrome requested a review from rymnc October 14, 2024 11:58
rymnc
rymnc reviewed Oct 14, 2024
View reviewed changes
crates/fuel-core/src/graphql_api/api_service.rs
@@ -220,6 +221,8 @@ where
OnChain::LatestView: OnChainDatabase,
OffChain::LatestView: OffChainDatabase,
{
graphql_api::initialize_query_costs(config.config.costs.clone())?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a comment here about not moving this line will be nice. if it is moved later, calls to query_costs() will yield the default value instead of the user's configuration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

additionally, I have a question about the graphql query complexity annotations, are they evaluated at compile time or runtime?

answered my own question - they are evaluated at runtime within a closure (expanded macro) -

image

I assume here that for every query it will run the closure, which might be a red flag for graphql performance, because of the atomic operations done by OnceLock::get() in the query_costs() function.

will defer to @xgreenx here, but we should load test this PR to ensure there isn't a regression.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, the async graphql book mentions that all complexity calculations are done during the verification phase of the query (https://async-graphql.github.io/async-graphql/en/depth_and_complexity.html#custom-complexity-calculation).

I'm not sure if a load acquire operation is going to create too much overhead, as it is one of the lowest overhead synchronization primitives. But i agree that this should be load-tested, perhaps in a different PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the load testing can happen with the image generated by this pr on devnet and testnet, let's do that first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rymnc rymnc rymnc left review comments

@xgreenx xgreenx Awaiting requested review from xgreenx xgreenx is a code owner

@Dentosal Dentosal Awaiting requested review from Dentosal Dentosal is a code owner

@MitchTurner MitchTurner Awaiting requested review from MitchTurner MitchTurner is a code owner

@acerone85 acerone85 Awaiting requested review from acerone85

At least 2 approving reviews are required to merge this pull request.

Assignees

@acerone85 acerone85

@netrome netrome

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make all limits of the of the complexity to be configurable via the CLI
3 participants
@netrome @rymnc @acerone85