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.

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

Set maxQueryExecutionTime through an environment variable #538

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thesan
Copy link
Member

@thesan thesan commented Mar 4, 2024

Address part of Joystream/joystream#5088

IMO rate limiting (DDOS prevention...) should be taken cared of by specialized tools (e.g Fail2ban).

affects: @joystream/hydra-cli

Add optional timeout for gql request and db queries

@thesan thesan marked this pull request as draft March 5, 2024 13:03
@thesan thesan requested a review from kdembler March 5, 2024 15:00
@thesan thesan marked this pull request as ready for review March 5, 2024 15:00
@thesan
Copy link
Member Author

thesan commented Mar 5, 2024

@kdembler I had made a mistake on the server config. Now I made a bunch of tests and even though according to the telemetry queries keep on going after the request timed out especially when the timeout is less than 300 ms, it looks like it's working. But the best would be to identify one of the slow queries thank to the telemetry and see what happens when the timeout is set.

@thesan
Copy link
Member Author

thesan commented Mar 5, 2024

Also I set this.httpServer.setTimeout(requestTimeout); in @joystream/warthog directly and it had the exact same effect than using the middleware (i.e the flow is not interrupted immediately but the request does timeout).

@thesan
Copy link
Member Author

thesan commented Mar 8, 2024

Hey @kdembler. You were right: I tested the express middleware with a partially processed mainnet, which made it clear that the flow wasn't stopping at all after the request timed out.

So I tried a similar approach but using a type-graphql global middleware and this time the flow does appear to be aborted. It did take a few 100s of ms after the timeout but with a query that would take ~800ms to execute (without timeout): the last event reported by the telemetry was at ~400ms (for a timeout of 200ms) so it seems to be working now.

@thesan
Copy link
Member Author

thesan commented Mar 8, 2024

Actually it didn't work with the slow distributionBuckets query this one ran in 51s without timing out. So I'll look at it some more on Monday...

That was just due to a the last rename of GRAPHQL_SERVER_RESOLVER_TIMEOUT.

Still I'm seeing some strange results so I'll look some more on Monday.

@thesan thesan marked this pull request as draft March 11, 2024 11:13
…custom maxQueryExecutionTime

affects: @joystream/hydra-cli

Set \`maxQueryExecutionTime\` through an environment variable
@thesan thesan force-pushed the feature/qn-request-timeout branch from 00e6ca4 to d4a124a Compare March 14, 2024 08:57
@thesan thesan changed the title feat(Add optional timeout for gql request and db queries): add timeout Set maxQueryExecutionTime through an environment variable Mar 14, 2024
@thesan thesan marked this pull request as ready for review March 14, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants