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

[server] Rate limit based on report count #3843

Merged
merged 3 commits into from
Oct 19, 2023
Merged

[server] Rate limit based on report count #3843

merged 3 commits into from
Oct 19, 2023

Conversation

vodorok
Copy link
Contributor

@vodorok vodorok commented Feb 15, 2023

This patch implements a basic implementation of rate limiting based on the raw report count in the incoming storage request.

@vodorok vodorok added WIP 💣 Work In Progress server 🖥️ labels Feb 15, 2023
@vodorok vodorok self-assigned this Feb 15, 2023
@vodorok vodorok requested a review from bruntib as a code owner February 15, 2023 16:13
@vodorok

This comment was marked as outdated.

Sign in to view
@vodorok vodorok marked this pull request as draft February 16, 2023 19:22
@vodorok vodorok requested a review from Szelethus March 13, 2023 11:35
@vodorok vodorok marked this pull request as ready for review March 13, 2023 11:40
@vodorok vodorok added API change 📄 Content of patch changes API! GUI 🎨 CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands and removed WIP 💣 Work In Progress labels Mar 13, 2023
@vodorok vodorok added this to the release 6.22.0 milestone Mar 17, 2023
@dkrupp dkrupp modified the milestones: release 6.22.0, release 6.22.1 Apr 13, 2023
@bruntib bruntib modified the milestones: release 6.22.2, release 6.23.0 Jul 4, 2023
@vodorok vodorok marked this pull request as draft August 31, 2023 13:05
@vodorok
Copy link
Contributor Author

vodorok commented Aug 31, 2023

The pull request was demoted to draft, as work still needs to be done.
I intend to return a better warning to the clients: what to do in case of an overflow in the following manner:

  • Show the worst-performing checkers by name and count, at least as much as needed to be under the limit.
  • Show what to insert into the invocation to disable these checkers.
You were trying to store a report folder that surpasses the report limit (500000) set on the web GUI by 231346.
Checker-a     5458473
Checker-b     323524
Checker-c     128543

Extend your CodeChecker analyze invocation with the following flags to disable these checkers and get under the limit:
-d Checker-a -d Checker-b -d Checker-c

@vodorok vodorok marked this pull request as ready for review October 10, 2023 13:29
Szelethus
Szelethus requested changes Oct 11, 2023
View reviewed changes
Copy link
Contributor

@Szelethus Szelethus left a comment

Choose a reason for hiding this comment

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

Seems like linters didn't run on this patch yet.

web/server/codechecker_server/database/config_db_model.py Show resolved Hide resolved
web/client/codechecker_client/cmd/store.py Outdated Show resolved Hide resolved
web/client/codechecker_client/cmd/store.py Outdated Show resolved Hide resolved
web/tests/functional/store/test_store.py Outdated Show resolved Hide resolved
web/server/vue-cli/src/components/Product/ProductConfigForm.vue Outdated Show resolved Hide resolved
web/server/vue-cli/package.json Outdated Show resolved Hide resolved
web/server/codechecker_server/api/product_server.py Outdated Show resolved Hide resolved
web/server/codechecker_server/api/mass_store_run.py Outdated Show resolved Hide resolved
web/server/codechecker_server/api/mass_store_run.py Outdated Show resolved Hide resolved
@Szelethus Szelethus mentioned this pull request Oct 11, 2023
Closed
@vodorok vodorok force-pushed the store_limit branch 3 times, most recently from 1885dcf to 936a47f Compare October 11, 2023 13:29
@vodorok vodorok force-pushed the store_limit branch 3 times, most recently from a8c6774 to bb5f368 Compare October 11, 2023 14:05
dkrupp
dkrupp requested changes Oct 11, 2023
View reviewed changes
Copy link
Member

@dkrupp dkrupp left a comment

Choose a reason for hiding this comment

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

Good job!
I tested this and works well with old and new clients.

I have only minor help text related comments.

Please mention this limit feature in this doc too
https://github.com/Ericsson/codechecker/blob/master/docs/web/products.md#adding-new-product

After these small fixes it is good to be merged in.

web/client/codechecker_client/cmd/store.py Outdated Show resolved Hide resolved
web/server/vue-cli/src/components/Product/ProductConfigForm.vue Outdated Show resolved Hide resolved
web/client/codechecker_client/cmd/cmd.py Outdated Show resolved Hide resolved
@vodorok vodorok force-pushed the store_limit branch 3 times, most recently from 5ca6fed to 6147717 Compare October 12, 2023 13:14
@vodorok
Copy link
Contributor Author

vodorok commented Oct 12, 2023

Good job! I tested this and works well with old and new clients.

I have only minor help text related comments.

Please mention this limit feature in this doc too https://github.com/Ericsson/codechecker/blob/master/docs/web/products.md#adding-new-product

After these small fixes it is good to be merged in.

Thanks, fixed the remarks, and extended the documentation.

@vodorok vodorok requested review from Szelethus and dkrupp October 12, 2023 13:45
@vodorok
Copy link
Contributor Author

vodorok commented Oct 12, 2023

One question. Should the report limit set to zero mean that there is no limit?

Szelethus
Szelethus approved these changes Oct 13, 2023
View reviewed changes
Copy link
Contributor

@Szelethus Szelethus left a comment

Choose a reason for hiding this comment

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

One question. Should the report limit set to zero mean that there is no limit?

No, 0 is zero, negative values with the exception of -1 should be invalid. -1 should mean no limit. Or at least, thats my spin on it :)

LGTM!

@vodorok vodorok force-pushed the store_limit branch 2 times, most recently from 5df6b98 to a1f1cc3 Compare October 18, 2023 16:09
@vodorok
[server] Rate limit based on report count
8def7f9 
This patch implements a basic implementation of rate limiting based on
the raw report count in the incoming storage request.

[feat][server][gui] Report limit in prof config

Added database entries in config db.
Modified api for passing report limit information.
Added widgets on the gui.
Added config option for commandline project creation.
A test is included.

Resolve conflict

Fix product remove parameter in web tests
@vodorok
[cmd] Store command should print deduplicated statistics
ef5d486 
Previously the store command printed the raw statistics of the report
folder, but none of the other commands or result of actions (parse
command or the result of the store) reflected these numbers.
This patch fixes that, and now the store commands prints the same
summary as the parse command.
@vodorok
[test] Test the store statistics output
76adc4a
dkrupp
dkrupp approved these changes Oct 19, 2023
View reviewed changes
Copy link
Member

@dkrupp dkrupp left a comment

Choose a reason for hiding this comment

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

LGTM.

I tested this with old and new clients. All works fine.

@dkrupp dkrupp merged commit fa41e4e into Ericsson:master Oct 19, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Szelethus Szelethus Szelethus approved these changes

@dkrupp dkrupp dkrupp approved these changes

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

Assignees

@vodorok vodorok

Labels
API change 📄 Content of patch changes API! CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands GUI 🎨 server 🖥️
Projects
None yet
Milestone
release 6.23.0
Development

Successfully merging this pull request may close these issues.
4 participants
@vodorok @dkrupp @Szelethus @bruntib