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

Disable express response validation in production #4810

Merged
merged 12 commits into from
Oct 23, 2023

Conversation

mnaamani
Copy link
Contributor

@mnaamani mnaamani commented Jul 21, 2023

A potential source of delay in processing requests for assets from storage and distributor nodes is the OpenAPI response validation that is happening because the nodes are running in "development" mode.

Most operators are likely not setting their NODE_ENV=production at all. But even they did, Colossus has hard-coded true for response validation, and Argus only disables it if NODE_ENV=prod.

I tested disabling response validation on my distributor node. Clearly we see a significant jump in the number requests/s the node is able to handle once the validation is disabled.

With response validation:
without-node_env-prod

Without validation:
with-node_env-prod

For Colossus it might also make sense to disable request validation to avoid validating large file uploads?

Distributors can immediately test this out by adding NODE_ENV: prod under the environment: section of their docker-compose.yml.

Storage operators would have to pull down latest version once it is merged, and set env variable NODE_ENV=production

If this truly makes a difference we will not feel it until all distributors and storage nodes apply this.

@mnaamani
Copy link
Contributor Author

Once distributors update to new verison you should change your NODE_ENV from prod to production so any other packages that maybbe also behaving in development mode, would switch to production and we may see more improvements (possibly).

@mnaamani mnaamani added the origo Origo Release label Jul 26, 2023
Copy link
Contributor

@zeeshanakram3 zeeshanakram3 left a comment

Choose a reason for hiding this comment

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

Looks Good.

Please run yarn generate:docs:all in distributor-node directory to update the API documentation

Just curious how did you figure out that this was sort of bottleneck. Was this latency information present in Opentelemetry traces.
This is express-openapi-validator's validator function responsible for response validation. I am assuming it involves complex checks like regex matching, type checking, or nested validation for objects and arrays, which increases response times

I guess it's fine to disable validation in prod, and delegate the validation responsibility to client applications

@mnaamani
Copy link
Contributor Author

Looks Good.

Please run yarn generate:docs:all in distributor-node directory to update the API documentation

will do.

Just curious how did you figure out that this was sort of bottleneck. Was this latency information present in Opentelemetry traces.

Actually nothing in the traces pointed at response validation being a bottleneck. I was just parsing the code and looking for something small to change that could have an impact, and from past experience I now if the response is large it could be a problem.

But I tested it after disabling validation and saw a large spike in request rate. But I'm starting to wonder if this was pure chance timing..

This is express-openapi-validator's validator function responsible for response validation. I am assuming it involves complex checks like regex matching, type checking, or nested validation for objects and arrays, which increases response times

Looking at the validation code, it looks like even if validation is enabled it seems it only does it if the content-type is JSON and skips otherwise. So It might in-fact not be having much of an affect.

@mnaamani
Copy link
Contributor Author

@zeeshanakram3 I set distributor-node package version to 1.3.2 as there is another already published v1.3.1 as you know from #4886

@kdembler
Copy link
Member

oof and I just changed to 1.4.0 in my PR, we will need to merge this all somehow :D

@mnaamani
Copy link
Contributor Author

oof and I just changed to 1.4.0 in my PR, we will need to merge this all somehow :D

Lets merge yours first, then I'll update this one to also be part of 1.4.0 before publishing docker image. WDYT?

@mnaamani mnaamani merged commit 6c13068 into Joystream:master Oct 23, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
origo Origo Release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants