refactor: prelculate advisory and vulnerability average scores and severities #1480
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
As I just added a bunch of tests to the scale test, could you re-run that again, with a recent checkout of the scale-testing repository? |
|
before I review this - I just want to state that adding triggers at this stage is risky ... event driven handling in databases introduces a next level of complexity - would rather avoid that. Is there any way to do this on ingestion instead of using triggers ? |
jcrossley3
reviewed
Mar 26, 2025
Comment on lines
-60
to
+56
| severity: score.severity(), | ||
| score: score.value(), | ||
| severity: advisory | ||
| .average_severity | ||
| .map(|sev| sev.into()) | ||
| .unwrap_or(Severity::None), | ||
| score: advisory.average_score.unwrap_or(0.0), |
There was a problem hiding this comment.
This is actually a bug: #1374
Can we just change these to Options in AdvisoryVulnerabilityHead and kill 2 birds with 1 stone? 😄
There was a problem hiding this comment.
Yes. It bothered me as well, but didn't want to change API on my own. I can include it as well.
There was a problem hiding this comment.
Not sure when this is getting merged, so went ahead and pushed #1591
|
@JimFuller-RedHat @jcrossley3 Yeah, trigger was the easiest way to get things going. I will rework it to do it from the code. |
@ctron I had some issues running the latest changes in scale-testing, both errors for the analysis tests and querying the database for large sboms. So, I reverted to the old one, just to get the first feeling of the performance. It'd be good to be able to run the action against PR as we discussed before. |
What errors did you encounter? |
|
@ctron: I got But I didn't look too much into it, so I might be doing something weird. |
Ah, you might want to try latest
Yea, that's a problem I'm not sure we can easily solve. Goose uses it's own argument system. That's why we rely on env-vars for rest. We forked Goose already, maybe migrating to clap is an idea. But that's expensive. |
dejanb
force-pushed
the
precalculate-averages
branch
2 times, most recently
from
39d1b82 to
bdf9c5e
Compare
dejanb
force-pushed
the
precalculate-averages
branch
from
bdf9c5e to
e1eda3f
Compare
|
/scale-test |
Goose Report# Goose Attack ReportPlan Overview
Request Metrics
Response Time Metrics
Status Code Metrics
Transaction Metrics
Scenario Metrics
|
|
/scale-test |
Goose Report# Goose Attack ReportPlan Overview
Request Metrics
Response Time Metrics
Status Code Metrics
Transaction Metrics
Scenario Metrics
|
|
/scale-test |
|
🛠️ Scale test has started! Follow the progress here: Workflow Run |
|
/scale-test |
|
🛠️ Scale test has started! Follow the progress here: Workflow Run |
|
/scale-test |
|
🛠️ Scale test has started! Follow the progress here: Workflow Run |
Goose ReportGoose Attack ReportPlan Overview
Request Metrics
Response Time Metrics
Status Code Metrics
Transaction Metrics
Scenario Metrics
📄 Full Report (Go to "Artifacts" and download report) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit introduces pre-calculated averages for scores and severities. The averages are pre-calculated during ingestion. The change simplifies querying significantly (along with performances) as there's no need to do recalculation on every query and often in the loop for a large number of entities.
Ingestion
Currently, there's a trigger that will update advisory and vulnerability each time the cvss3 score is inserted. Volume of data here is not that big ~50K even for the load dataset. Updating all entries takes locally around 10 sec
My main concern was if this will impact the ingestion. In the little testing that I have done, I didn't see any changes in ingestion time during this change.
In theory we can move this to the application logic and recalculate all after ingestion, but I wound't like to do it if it is not necessary.
Querying
I ran load test locally and things looks promising in general
I saw even better improvements on the most problematic advisory endpoint in the previous runs. And although I didn't run the latest version of the scale tests that test sbom/advisory endpoint, manually testing shows at least 50% performance improvements.
Let's see what the automated load testing will show.
There's still more things to change here as some of the queries are still can be improved.