chore(performance): Adjust thresholds #1870
Conversation
📝 WalkthroughWalkthroughThis pull request updates the performance testing configuration and associated test scripts. The CI/CD workflows now reference a new performance testing workflow file and use Azure credentials instead of token-based secrets. Additional parameters have been added to better control test execution. In the test scripts, performance thresholds have been modified by removing certain conditions, and the dialog creation script has been refactored with updated imports and a new function signature. A new performance test script for creating transmissions has also been introduced. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/k6/tests/serviceowner/performance/createTransmissionsWithThresholds.js (1)
17-20: Consider documenting environment variables.The environment variables provide good flexibility for test configuration. Consider adding documentation about these variables and their impact.
Add a comment block explaining the environment variables:
+// Environment variables: +// - traceCalls: Enable/disable tracing (default: false) +// - numberOfTransmissions: Number of transmissions to create (default: 10) +// - maxTransmissionsInThread: Maximum transmissions per thread (default: 100) +// - TESTID: Identifier for the test run (default: createTransmissions) const traceCalls = (__ENV.traceCalls ?? 'false') === 'true'; const numberOfTransmissions = (__ENV.numberOfTransmissions ?? '10'); const maxTransmissionsInThread = (__ENV.maxTransmissionsInThread ?? '100'); const testid = (__ENV.TESTID ?? 'createTransmissions');.github/workflows/ci-cd-yt01.yml (1)
117-117: Remove commented out code.The commented out
needsline should be removed as it's no longer needed.- #needs: [deploy-apps, check-for-changes]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/ci-cd-yt01.yml(2 hunks).github/workflows/workflow-run-k6-performance.yml(1 hunks)tests/k6/tests/enduser/performance/enduserSearchWithThresholds.js(1 hunks)tests/k6/tests/serviceowner/performance/createDialogWithThresholds.js(1 hunks)tests/k6/tests/serviceowner/performance/createTransmissionsWithThresholds.js(1 hunks)tests/k6/tests/serviceowner/performance/serviceOwnerSearchWithThresholds.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dry run deploy infrastructure / Deploy to test
- GitHub Check: build / build-and-test
🔇 Additional comments (13)
tests/k6/tests/serviceowner/performance/createDialogWithThresholds.js (3)
1-3: LGTM! Good modularization.The imports have been refactored to use common modules, improving code reusability and maintainability.
14-14: LGTM! Clean implementation with environment variable support.The implementation has been simplified and now supports tracing through environment variables, making it more configurable and maintainable.
Also applies to: 16-18
9-9:❓ Verification inconclusive
Verify the impact of removing p99 threshold.
The p99 threshold has been removed and p95 has been made stricter (from 300ms to 200ms). While this aligns with the PR objectives, please ensure this doesn't impact the reliability of performance testing.
🌐 Web query:
What are the best practices for setting performance test thresholds in k6?💡 Result:
When setting performance test thresholds in k6, these best practices ensure effective evaluation of system performance against service-level objectives (SLOs):
Error Rate Thresholds
- Set maximum acceptable error rates using
http_req_failedmetric
rate<=0.05(5% max errors) [1][2][7]- Combine with HTTP status checks for additional validation:
check(res, {'status is 200': (r) => r.status === 200})[2][5]Response Time Thresholds
- Use percentile-based targets for realistic performance assessment:
http_req_duration: ['p(95)<500', 'p(99)<1000'][1][7]- Layer multiple percentiles for comprehensive analysis:
['p(90) < 400', 'p(95) < 800', 'p(99.9) < 2000'][7]Threshold Configuration Strategies
- Abort tests on critical failures using
abortOnFail:thresholds: { http_req_failed: [{ threshold: 'rate<=0.01', abortOnFail: true }] }[1]- Validate check success rates:
checks: ['rate>0.9'][5]- Monitor system capacity metrics:
vus_maxfor maximum allocated users[2][10]iterationsfor completed test cycles[4]CI/CD Integration
- Return non-zero exit codes for automated failure detection[1][7]
- Combine with trend metrics for historical comparison:
'iteration_duration{scenario:default}': ['max>=0'][10]Maintenance Best Practices
- Modularize configurations for reusability across test suites[9]
- Set explicit duration targets in load profiles:
stages: [ {duration: '5m', target: 100}, {duration: '30m', target: 100} ][1][10]These practices enable teams to establish clear performance benchmarks while maintaining flexibility to adapt thresholds as systems evolve[3][7]. Always validate threshold values against actual business requirements and historical performance data[9].
Citations:
- 1: https://github.com/grafana/k6-learn/blob/main/Modules/II-k6-Foundations/07-Setting-test-criteria-with-thresholds.md
- 2: https://pflb.us/blog/k6-user-manual/
- 3: https://blog.nashtechglobal.com/best-practices-for-writing-efficient-k6-scripts-for-performance-testing/
- 4: https://jignect.tech/mastering-performance-testing-with-k6-a-guide-for-qa-testers/
- 5: https://www.youtube.com/watch?v=genc0YrFi9k
- 6: https://grafana.com/docs/k6/latest/testing-guides/
- 7: https://grafana.com/docs/k6/latest/using-k6/thresholds/
- 8: https://dev.to/nadirbasalamah/api-performance-testing-with-k6-a-quick-start-guide-2ngc
- 9: https://grafana.com/blog/2024/04/30/organizing-your-grafana-k6-performance-testing-suite-best-practices-to-get-started/
- 10: https://circleci.com/blog/api-performance-testing-with-k6/
Action Required: Review Impact of Removing p99 Threshold
The updated code now uses a stricter p95 threshold (from 300ms to 200ms) and removes the p99 threshold. While this change aligns with the PR objectives, best practices for k6 performance testing recommend layering multiple percentiles (e.g., combining p95 and p99) to capture both overall and tail performance metrics. Please verify that the removal of the p99 threshold does not overlook potential latency issues, especially under peak conditions or edge cases. Consider reviewing historical performance data and current SLOs to ensure that relying solely on a stricter p95 value meets reliability requirements.
tests/k6/tests/serviceowner/performance/serviceOwnerSearchWithThresholds.js (1)
10-17:❓ Verification inconclusive
Consistent threshold configuration across endpoints.
The thresholds have been standardized to p95<300ms for all endpoints, which aligns with the PR objectives. This consistency makes it easier to monitor and maintain performance standards.
Let's verify if these thresholds are reasonable for all endpoints:
🏁 Script executed:
#!/bin/bash # Description: Check historical performance data for these endpoints # Get the last 5 performance test runs gh run list --workflow "Performance Tests" --limit 5 --json conclusion,url | jq -r '.[] | "\(.conclusion) \(.url)"'Length of output: 237
Update: Threshold Consistency Verified – Manual Performance Verification Needed
The standardized thresholds (p(95) < 300ms) across the endpoints appear consistent with the PR’s objectives. However, the automated check for historical performance data failed (due to the repository not being recognized as a git repository), so we cannot conclusively verify the thresholds’ adequacy. Please manually review recent performance test outcomes to confirm that these thresholds are appropriate.
- File:
tests/k6/tests/serviceowner/performance/serviceOwnerSearchWithThresholds.js(Lines 10-17)tests/k6/tests/enduser/performance/enduserSearchWithThresholds.js (1)
10-18: LGTM! Consistent with service owner thresholds.The threshold configuration mirrors the changes in service owner endpoints, maintaining consistency across the codebase.
tests/k6/tests/serviceowner/performance/createTransmissionsWithThresholds.js (2)
1-15: LGTM! Well-structured performance test configuration.The configuration follows the established pattern from other performance tests, with appropriate thresholds and reusable modules.
22-24: LGTM! Clean implementation.The test function is concise and follows the same pattern as other performance tests.
.github/workflows/workflow-run-k6-performance.yml (3)
30-34: LGTM! Well-documented optional parameter.The new
refparameter is properly documented with a clear description and sensible default value.
50-55: LGTM! Secure authentication implementation.The workflow uses OIDC-based Azure authentication which is more secure than using long-lived credentials.
60-73: LGTM! Robust error handling in k8s setup.The script properly checks for failures at each step of the k8s setup process and provides clear error messages.
.github/workflows/ci-cd-yt01.yml (3)
118-121: LGTM! Secure authentication transition.Good transition from token-based authentication to Azure OIDC, which is more secure and follows best practices.
136-138: LGTM! Enhanced test control parameters.The addition of
parallelism,breakpoint, andabortOnFailparameters provides better control over test execution, aligning with the PR's objective to adjust performance testing.
122-129: LGTM! Well-structured test matrix.The matrix strategy is well-organized with proper fail-fast configuration and max-parallel limit to prevent resource contention.
tests/k6/tests/serviceowner/performance/createDialogWithThresholds.js
Outdated
Show resolved
Hide resolved
tests/k6/tests/serviceowner/performance/createTransmissionsWithThresholds.js
Outdated
Show resolved
Hide resolved
🤖 I have created a release *beep* *boop* --- ## [1.50.5](v1.50.4...v1.50.5) (2025-02-13) ### Miscellaneous Chores * **k6:** Consolidate k6-utils import URLs ([#1869](#1869)) ([09b3722](09b3722)) * Misc. formatting, typos ([#1868](#1868)) ([5de894f](5de894f)) * **performance:** Adjust thresholds ([#1870](#1870)) ([3e6e3b9](3e6e3b9)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Adjust smoketests thresholds for p95 and removed thresholds for p99.
Use k8s k6 operator cluster instead of github runner
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)