Skip to content

Conversation

@bxf12315
Copy link
Contributor

@bxf12315 bxf12315 commented Nov 3, 2025

No description provided.

@bxf12315 bxf12315 requested a review from ctron November 3, 2025 14:47
Copy link
Contributor

@ctron ctron left a comment

Choose a reason for hiding this comment

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

Running that change over and over again, it would always set the same value on the same entity.

I think we should have this a bit more random. At least when it comes to the values. One idea could be to use a timestamp as a value.

Another idea could be to pick a random advisory, instead of using only a single (configured) one.

@bxf12315
Copy link
Contributor Author

Running that change over and over again, it would always set the same value on the same entity.

I think we should have this a bit more random. At least when it comes to the values. One idea could be to use a timestamp as a value.

Another idea could be to pick a random advisory, instead of using only a single (configured) one.

I chose the second approach. Please review it again.

@ctron
Copy link
Contributor

ctron commented Nov 12, 2025

Another idea could be to pick a random advisory, instead of using only a single (configured) one.

I chose the second approach. Please review it again.

Great. Sorry for not being clear enough. I think in this case the values should not be configured. There seems to be no benefit of doing that. But simply always fetch it from the database.

@bxf12315
Copy link
Contributor Author

The corresponding pull request. Thanks
guacsec/trustify-scale-test-runs#30

@ctron
Copy link
Contributor

ctron commented Nov 26, 2025

The corresponding pull request. Thanks guacsec/trustify-scale-test-runs#30

I am not sure how this relates to the request of not having the value configured, but evaluated from the database?

@bxf12315
Copy link
Contributor Author

bxf12315 commented Nov 27, 2025

The corresponding pull request. Thanks guacsec/trustify-scale-test-runs#30

I am not sure how this relates to the request of not having the value configured, but evaluated from the database?

If put_advisory_labels and patch_advisory_labels are empty in the scenario, these two methods will be skipped. Does trustify-scale-test-run regenerate the scenario file on every run? Do you mean removing put_advisory_labels and patch_advisory_labels from the scenario and reading them from the database each time we run the scale test? However, in this case, no database is specified when the test is executed.
At the moment, the logic in this PR is to query 20 IDs from the database for put_advisory_labels and patch_advisory_labels. Then, here (https://github.com/guacsec/trustify-scale-testing/pull/78/files#diff-6991c4c6d61c5d119cd2983b5f8f30c3b5681dddb103d2978c7ab4175b2567f2R153), these IDs are read and used. As the scale test currently reads its data from the scenario, it seems logical to write those IDs into the scenario file. Would this approach be considered incorrect? If not, could you please provide some more detailed clarification? Thanks.

@ctron
Copy link
Contributor

ctron commented Nov 27, 2025

Another idea could be to pick a random advisory, instead of using only a single (configured) one.

I chose the second approach. Please review it again.

Great. Sorry for not being clear enough. I think in this case the values should not be configured. There seems to be no benefit of doing that. But simply always fetch it from the database.

Right, this is what I tried to say here. Those values should not be configured. But just picked randomly from the database, each time it runs. I think this could be done by using the API. Or it could be done by directly accessing the database. I think using the API might make it easier to run, as no DB connection would be required.

It may be beneficial to split this up into multiple steps.

@bxf12315
Copy link
Contributor Author

bxf12315 commented Nov 27, 2025

Another idea could be to pick a random advisory, instead of using only a single (configured) one.

I chose the second approach. Please review it again.

Great. Sorry for not being clear enough. I think in this case the values should not be configured. There seems to be no benefit of doing that. But simply always fetch it from the database.

Right, this is what I tried to say here. Those values should not be configured. But just picked randomly from the database, each time it runs. I think this could be done by using the API. Or it could be done by directly accessing the database. I think using the API might make it easier to run, as no DB connection would be required.

It may be beneficial to split this up into multiple steps.

Does this mean that before performing each put or patch action, an ID needs to be queried first, just like it is done when uploading and deleting advisories?
Do we need to retain the put_advisory_labels and patch_advisory_labels attributes in the scenario? If so, should the corresponding advisory IDs be randomly fetched via REST API at the start of the scale test and assigned to these attributes? Is that correct?

@bxf12315
Copy link
Contributor Author

The latest commit randomly selects an advisory ID and then performs put or patch operations on it. However, it results in two requests being made.
Screenshot 2025-11-27 at 22 44 20
That seems reasonable as a benchmark, don’t you think?

"download_advisory": "24ae57c3-4b57-4f4e-82c1-83ae26059a89",
"get_advisory": "24ae57c3-4b57-4f4e-82c1-83ae26059a89"
"get_advisory": "24ae57c3-4b57-4f4e-82c1-83ae26059a89",
"put_advisory_lables": [
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove those arrays.

src/restapi.rs Outdated
user: &mut GooseUser,
) -> Result<String, Box<TransactionError>> {
// Generate random 3-digit offset
let offset = rand::thread_rng().gen_range(0..=999);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of assuming a maximum number, you can (on startup) query the total number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please check again.

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