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

improvement(perf): add validation rules for latency decorator #9295

Merged
merged 1 commit into from
Dec 23, 2024
Merged

improvement(perf): add validation rules for latency decorator #9295

merged 1 commit into from
Dec 23, 2024

Conversation

soyacz
Copy link
Contributor

@soyacz soyacz commented Nov 20, 2024 •
edited
Loading

Added validation rules for results sent by
latency_calculator_decorator to Argus.
Each workload and result name (nemesis, predefined step) may set own rules.

Current rules were created based on existing results - to pass typical good results.

closes: #9237

Testing

PR pre-checks (self review)

  • I added the relevant backport labels
  • I didn't leave commented-out/debugging code

Reminders

  • Add New configuration option and document them (in sdcm/sct_config.py)
  • Add unit tests to cover my changes (under unit-test/ folder)
  • Update the Readme/doc folder relevant to this change (if needed)

fruch
fruch reviewed Nov 20, 2024
View reviewed changes
sdcm/argus_results.py Outdated Show resolved Hide resolved
fruch
fruch reviewed Nov 20, 2024
View reviewed changes
sdcm/argus_results.py Outdated Show resolved Hide resolved
@soyacz soyacz force-pushed the add-limits-to-latency-decorator branch from 09a3d76 to bfbe9d1 Compare November 21, 2024 16:10
@soyacz soyacz marked this pull request as draft November 21, 2024 16:14
@soyacz
Copy link
Contributor Author

soyacz commented Nov 21, 2024

@fruch I adjusted the code to configure it by config file. Converted to draft as I didn't add error thresholds for OSS yet.

fruch
fruch reviewed Nov 21, 2024
View reviewed changes
sdcm/sct_config.py Outdated Show resolved Hide resolved
fruch
fruch reviewed Nov 21, 2024
View reviewed changes
defaults/test_default.yaml Outdated Show resolved Hide resolved
@soyacz soyacz force-pushed the add-limits-to-latency-decorator branch from bfbe9d1 to 2530138 Compare November 22, 2024 09:18
@fruch
Copy link
Contributor

fruch commented Nov 25, 2024

we are missing a configuration for the the upgrade cases

@soyacz soyacz force-pushed the add-limits-to-latency-decorator branch 2 times, most recently from 733c652 to c0a7676 Compare November 25, 2024 17:53
@soyacz
Copy link
Contributor Author

soyacz commented Nov 26, 2024

I verified predefined steps test (with null'ed validation rules for latencies in unthrottled) - all seem to work (except one small issue with Argus: https://argus.scylladb.com/tests/scylla-cluster-tests/9e2af03d-b1a5-4df0-b516-4ce5e624586d)
Besides, due #9294 I think of taking out default throughput verifications - until this one is closed (not to generate false errors).
I'll prepare this PR for final review (and construct rules for tablets scenarios).
Defaults should be good for upgrade, until we want to verify durations for these - @fruch @juliayakovlev let me know if I should add it).

fruch
fruch reviewed Nov 26, 2024
View reviewed changes
test-cases/performance/perf-regression-latency-650gb-with-nemesis.yaml Outdated
@@ -1,12 +1,12 @@
test_duration: 3000
prepare_write_cmd: ["cassandra-stress write no-warmup cl=ALL n=162500000 -schema 'replication(strategy=NetworkTopologyStrategy,replication_factor=3)' -mode cql3 native -rate threads=200 -col 'size=FIXED(128) n=FIXED(8)' -pop seq=1..162500000",
Copy link
Contributor

Choose a reason for hiding this comment

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

reminder, taking this out

@soyacz soyacz force-pushed the add-limits-to-latency-decorator branch 3 times, most recently from 588f762 to 5e07743 Compare November 26, 2024 10:25
@soyacz soyacz marked this pull request as ready for review November 26, 2024 10:28
@soyacz
Copy link
Contributor Author

soyacz commented Nov 26, 2024

@fruch @juliayakovlev I think it's ready for review. All duration/latency error thresholds I based on graphs - mostly to make them passing. I think fine tuning them may be done later on perf weekly meetings, when graphs show them.

@soyacz soyacz force-pushed the add-limits-to-latency-decorator branch from 5e07743 to 993c44c Compare November 26, 2024 15:49
fruch
fruch previously approved these changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@fruch fruch left a comment

Choose a reason for hiding this comment

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

LGTM

but let's wait for @roydahan and @juliayakovlev to cross check the figures

@soyacz
Copy link
Contributor Author

soyacz commented Dec 1, 2024

@roydahan @juliayakovlev ^^

@soyacz soyacz force-pushed the add-limits-to-latency-decorator branch from 8a72dfa to 4adf310 Compare December 2, 2024 12:41
@soyacz
Copy link
Contributor Author

soyacz commented Dec 2, 2024

small comments

generally, if something is not provided then defaults are used. But I added it for clarity.
What I'm most concerned if my values are correct - if I wasn't too delicate for scylla and bars should be marked lower at some places (especially for durations).

@soyacz soyacz requested a review from juliayakovlev December 3, 2024 14:09
@fruch
Copy link
Contributor

fruch commented Dec 3, 2024

@soyacz

you have a small conflict here

@soyacz soyacz force-pushed the add-limits-to-latency-decorator branch from 4adf310 to ca452ea Compare December 3, 2024 15:54
@soyacz
Copy link
Contributor Author

soyacz commented Dec 3, 2024

@soyacz

you have a small conflict here

fixed

roydahan
roydahan reviewed Dec 18, 2024
View reviewed changes
configurations/performance/latency-decorator-error-thresholds-nemesis-oss-vnodes.yaml Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove all OSS ones and replace with nemesis (which this one is missing for enterprise)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed, missing file added

@soyacz soyacz force-pushed the add-limits-to-latency-decorator branch from ca452ea to 8e87053 Compare December 18, 2024 17:32
@soyacz
Copy link
Contributor Author

soyacz commented Dec 18, 2024

@roydahan I adjusted according to our discussion, please review

@soyacz
improvement(perf): add validation rules for latency decorator
bad6f66 
Added validation rules for results sent by
`latency_calculator_decorator` to Argus.
Each workload and result name (nemesis, predefined step) may set own
rules.

Current rules were created based on existing results - to pass typical
good results.

closes: scylladb#9237
@soyacz soyacz force-pushed the add-limits-to-latency-decorator branch from 8e87053 to bad6f66 Compare December 19, 2024 08:44
fruch
fruch approved these changes Dec 23, 2024
View reviewed changes
Copy link
Contributor

@fruch fruch left a comment

Choose a reason for hiding this comment

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

LGTM

@fruch fruch merged commit bc3552a into scylladb:master Dec 23, 2024
7 checks passed
This was referenced Dec 23, 2024
Merged
Merged
@juliayakovlev juliayakovlev mentioned this pull request Dec 30, 2024
Merged
3 tasks
juliayakovlev added a commit to juliayakovlev/scylla-cluster-tests that referenced this pull request Jan 6, 2025
@juliayakovlev
fix(pipeline): config params for predefined write test
9b65325 
Add missed double quote. It was presented by
scylladb#9295 and cause to job
failure
@juliayakovlev juliayakovlev mentioned this pull request Jan 6, 2025
Merged
2 tasks
fruch pushed a commit that referenced this pull request Jan 6, 2025
@juliayakovlev @fruch
fix(pipeline): config params for predefined write test
113a0bf 
Add missed double quote. It was presented by
#9295 and cause to job
failure
mergify bot pushed a commit that referenced this pull request Jan 6, 2025
@juliayakovlev @mergify
fix(pipeline): config params for predefined write test
11d6250 
Add missed double quote. It was presented by
#9295 and cause to job
failure

(cherry picked from commit 113a0bf)
@mergify mergify bot mentioned this pull request Jan 6, 2025
Merged
2 tasks
fruch pushed a commit that referenced this pull request Jan 6, 2025
@juliayakovlev @fruch
fix(pipeline): config params for predefined write test
595d984 
Add missed double quote. It was presented by
#9295 and cause to job
failure

(cherry picked from commit 113a0bf)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roydahan roydahan roydahan left review comments

@fruch fruch fruch approved these changes

@juliayakovlev juliayakovlev Awaiting requested review from juliayakovlev

Assignees

@soyacz soyacz

Labels
backport/perf-v15-done backport/perf-v16-done promoted-to-master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Latency decorator error thresholds
5 participants
@soyacz @fruch @roydahan @juliayakovlev @scylladbbot