chore: add performance tests to ci-cd-yt01 workflow #1595
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new job named Changes
Possibly related PRs
Suggested reviewers
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
🧹 Outside diff range and nitpick comments (8)
tests/k6/tests/serviceowner/createDialogWithThresholds.js (2)
5-6: Consider increasing virtual users for stress testingA single virtual user (VU) might not effectively stress test the dialog creation endpoint. Consider implementing a ramping pattern or higher VU count to better simulate real-world load.
- vus: 1, + stages: [ + { duration: '10s', target: 1 }, // Warm-up + { duration: '10s', target: 5 }, // Ramp-up load + { duration: '10s', target: 5 }, // Stay at peak + ],
1-1: Consider using absolute imports for better maintainabilityUsing relative imports can be fragile if file structure changes. Consider using absolute imports from a base path.
-import { default as run } from "./performance/create-dialog.js"; +import { default as run } from "@tests/k6/tests/serviceowner/performance/create-dialog.js";tests/k6/tests/enduser/enduserSearchWithThresholds.js (1)
3-25: Consider extracting common configuration to a shared moduleThe options configuration is largely similar across all three test files. Consider creating a shared configuration module to:
- Standardize common settings (summaryTrendStats, duration, VUs)
- Provide factory functions for threshold generation
- Ensure consistent performance criteria across similar endpoints
Example shared configuration:
// tests/k6/config/common.js export const baseOptions = { summaryTrendStats: ['avg', 'min', 'med', 'max', 'p(95)', 'p(99)', 'p(99.5)', 'p(99.9)', 'count'], duration: "30s" }; export const createThresholds = (endpoints, { p95 = 300, p99 = 500 } = {}) => { return endpoints.reduce((acc, endpoint) => ({ ...acc, [`http_req_duration{name:${endpoint}}`]: [`p(95)<${p95}`, `p(99)<${p99}`], [`http_reqs{name:${endpoint}}`]: [] }), {}); };.github/workflows/workflow-run-k6-performance.yml (1)
56-59: Consider adding error handling for token generationThe script execution should be checked for errors before proceeding with the K6 test.
- ./tests/k6/tests/scripts/generate_tokens.sh ./tests/k6/tests/performancetest_data ${{ inputs.tokens }} ${{ inputs.numberOfTokens }} ${{ inputs.ttl }} + if ! ./tests/k6/tests/scripts/generate_tokens.sh ./tests/k6/tests/performancetest_data ${{ inputs.tokens }} ${{ inputs.numberOfTokens }} ${{ inputs.ttl }}; then + echo "Token generation failed" + exit 1 + fitests/k6/tests/scripts/generate_tokens.sh (2)
63-68: Verify token limit implementation consistencyThe token limit implementation is duplicated in both enterprise and personal token generation loops. Consider extracting this logic into a function to ensure consistency and maintainability.
+check_token_limit() { + local generated=$1 + local limit=$2 + if [ $limit -gt 0 ] && [ $generated -ge $limit ]; then + return 1 + fi + return 0 +} - if [ $limit -gt 0 ] && [ $generated -ge $limit ]; then - break - fi + if ! check_token_limit "$generated" "$limit"; then + break + fiAlso applies to: 91-96
69-69: Consider adding retry mechanism for token generationThe token generation URLs are called without retry logic. Network issues could cause intermittent failures.
+retry_curl() { + local url=$1 + local auth=$2 + local max_attempts=3 + local attempt=1 + + while [ $attempt -le $max_attempts ]; do + response=$(curl -s -f "$url" -u "$auth") + if [ $? -eq 0 ]; then + echo "$response" + return 0 + fi + echo "Attempt $attempt failed. Retrying..." >&2 + attempt=$((attempt + 1)) + sleep 1 + done + return 1 +} - token=$(curl -s -f $url -u "$tokengenuser:$tokengenpasswd" ) + token=$(retry_curl "$url" "$tokengenuser:$tokengenpasswd")Also applies to: 97-97
.github/workflows/ci-cd-yt01.yml (2)
156-162: Consider parameterizing the test matrixThe test files are hardcoded in the matrix. Consider moving them to a configuration file for better maintainability.
+ # Create a new file: .github/config/performance-tests.json + { + "tests": [ + "tests/k6/tests/serviceowner/serviceOwnerSearchWithThresholds.js", + "tests/k6/tests/serviceowner/createDialogWithThresholds.js", + "tests/k6/tests/enduser/enduserSearchWithThresholds.js" + ] + } - files: - - tests/k6/tests/serviceowner/serviceOwnerSearchWithThresholds.js - - tests/k6/tests/serviceowner/createDialogWithThresholds.js - - tests/k6/tests/enduser/enduserSearchWithThresholds.js + files: ${{ fromJson(readFile('.github/config/performance-tests.json')).tests }}🧰 Tools
🪛 yamllint (1.35.1)
[error] 159-159: trailing spaces
(trailing-spaces)
148-148: Remove commented-out codeRemove the commented-out needs line 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(1 hunks).github/workflows/workflow-run-k6-performance.yml(2 hunks)tests/k6/tests/enduser/enduserSearchWithThresholds.js(1 hunks)tests/k6/tests/scripts/generate_tokens.sh(6 hunks)tests/k6/tests/serviceowner/createDialogWithThresholds.js(1 hunks)tests/k6/tests/serviceowner/serviceOwnerSearchWithThresholds.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
tests/k6/tests/enduser/enduserSearchWithThresholds.js (1)
Learnt from: dagfinno
PR: digdir/dialogporten#1373
File: tests/k6/tests/enduser/performance/simple-search.js:11-20
Timestamp: 2024-11-12T05:32:45.311Z
Learning: In `tests/k6/tests/performancetest_common/common.js`, the function `getDefaultThresholds` may intentionally create empty threshold arrays to make labels visible in k6 reports.
🪛 yamllint (1.35.1)
.github/workflows/workflow-run-k6-performance.yml
[error] 27-27: trailing spaces
(trailing-spaces)
[error] 31-31: trailing spaces
(trailing-spaces)
.github/workflows/ci-cd-yt01.yml
[error] 155-155: trailing spaces
(trailing-spaces)
[error] 159-159: trailing spaces
(trailing-spaces)
🔇 Additional comments (7)
tests/k6/tests/serviceowner/serviceOwnerSearchWithThresholds.js (2)
5-6: Consider increasing virtual users for stress testing
Similar to createDialogWithThresholds.js, a single virtual user might not effectively stress test these endpoints.
8-13: Consider aligning performance thresholds across similar endpoints
The thresholds (p95<100ms, p99<300ms) are more stringent than those in createDialogWithThresholds.js (p95<300ms, p99<500ms). Consider standardizing these thresholds unless there's a specific reason for the difference.
tests/k6/tests/enduser/enduserSearchWithThresholds.js (1)
5-6: Consider increasing virtual users for stress testing
Similar to other test files, a single virtual user might not effectively stress test these endpoints.
.github/workflows/workflow-run-k6-performance.yml (1)
24-31: LGTM: New input parameters are well-defined
The new input parameters numberOfTokens and ttl are properly configured with appropriate defaults and types.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 27-27: trailing spaces
(trailing-spaces)
[error] 31-31: trailing spaces
(trailing-spaces)
tests/k6/tests/scripts/generate_tokens.sh (1)
14-16: LGTM: Usage information updated correctly
The usage information clearly describes the new parameters and includes a helpful example.
.github/workflows/ci-cd-yt01.yml (2)
146-147: LGTM: Proper job dependencies and conditions
The job correctly depends on both infrastructure and application deployments, with appropriate conditions for execution.
166-168: Verify performance test parameters
The initial values for VUs (1) and duration (30s) seem conservative. Ensure these values are sufficient for meaningful performance testing.
✅ Verification successful
Performance test parameters align with existing test configurations
The current VU (1) and duration (30s) values in the CI/CD workflow match the configurations used across multiple performance test files in the codebase. These tests have well-defined thresholds:
- HTTP request duration p95 < 100-300ms
- HTTP request duration p99 < 300-500ms
- Failed requests rate < 1%
The parameters are consistent with the established performance testing strategy in the codebase and appear sufficient for meaningful testing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the test files contain any specific thresholds or requirements
echo "Checking performance test thresholds..."
for test_file in $(fd -e js . tests/k6/tests/ | grep "Thresholds.js$"); do
echo "=== $test_file ==="
rg "threshold|target|vus|duration" "$test_file" -A 2
done
Length of output: 3301
…gporten into performance/threshold-tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
tests/k6/tests/enduser/performance/enduser-search.js (2)
11-18: LGTM! Consider documenting threshold naming conventions.The addition of the 'enduser' prefix to threshold names improves metric categorization and namespace isolation. The naming convention is consistent across all operations.
Consider adding a comment explaining the threshold naming convention and its relationship to the K6 reporting structure.
thresholds: getDefaultThresholds(['http_req_duration', 'http_reqs'],[ + // Threshold names are prefixed with 'enduser' to categorize metrics in K6 reports 'enduser get dialog', 'enduser get dialog activities',
Line range hint
4-6: Consider enhancing environment variable validation.While the current boolean parsing is functional, consider adding validation for unexpected values and providing meaningful error messages.
Here's a suggested enhancement:
-const isSingleUserMode = (__ENV.isSingleUserMode ?? 'false') === 'true'; -const traceCalls = (__ENV.traceCalls ?? 'false') === 'true'; +function parseBoolean(value, defaultValue, name) { + if (value === undefined) return defaultValue; + if (value !== 'true' && value !== 'false') { + throw new Error(`Invalid value for ${name}. Expected 'true' or 'false', got '${value}'`); + } + return value === 'true'; +} + +const isSingleUserMode = parseBoolean(__ENV.isSingleUserMode, false, 'isSingleUserMode'); +const traceCalls = parseBoolean(__ENV.traceCalls, false, 'traceCalls');.github/workflows/ci-cd-yt01.yml (3)
147-148: Remove commented code.The commented line
#needs: [deploy-apps, check-for-changes]should be removed as it appears to be a remnant of previous configuration.
156-163: Clean up trailing spaces.Remove trailing spaces from the following lines:
- Line 159:
files:- Line 163:
fail-fast: false🧰 Tools
🪛 yamllint (1.35.1)
[error] 159-159: trailing spaces
(trailing-spaces)
[error] 163-163: trailing spaces
(trailing-spaces)
156-162: Consider test execution strategy.The current configuration runs tests sequentially (
max-parallel: 1) which is good for accurate performance measurements. However, consider:
- Adding comments to explain why sequential execution is necessary
- Documenting the expected duration for the complete test suite
- Considering test prioritization if the suite grows larger
🧰 Tools
🪛 yamllint (1.35.1)
[error] 159-159: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci-cd-yt01.yml(1 hunks)tests/k6/tests/enduser/enduserSearchWithThresholds.js(1 hunks)tests/k6/tests/enduser/performance/enduser-search.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/k6/tests/enduser/enduserSearchWithThresholds.js
🧰 Additional context used
📓 Learnings (1)
tests/k6/tests/enduser/performance/enduser-search.js (1)
Learnt from: dagfinno
PR: digdir/dialogporten#1373
File: tests/k6/tests/enduser/performance/simple-search.js:11-20
Timestamp: 2024-11-12T05:32:45.311Z
Learning: In `tests/k6/tests/performancetest_common/common.js`, the function `getDefaultThresholds` may intentionally create empty threshold arrays to make labels visible in k6 reports.
🪛 yamllint (1.35.1)
.github/workflows/ci-cd-yt01.yml
[error] 155-155: trailing spaces
(trailing-spaces)
[error] 159-159: trailing spaces
(trailing-spaces)
[error] 163-163: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
tests/k6/tests/enduser/performance/enduser-search.js (1)
Line range hint 4-6: Verify CI/CD workflow integration.
Ensure that the environment variables and test data setup are properly configured in the ci-cd-yt01 workflow.
Also applies to: 22-33
✅ Verification successful
Environment variables properly configured in CI/CD workflow
The environment variables isSingleUserMode and traceCalls are correctly integrated in the test script and will be passed through the k6 test execution environment. The workflow configuration in ci-cd-yt01.yml properly sets up the k6 performance test execution with necessary secrets and configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check workflow configuration for environment variables and test data setup
# Check for workflow file and environment variable configuration
echo "Checking workflow configuration..."
rg -A 5 "isSingleUserMode|traceCalls" .github/workflows/
# Check for test data setup in workflow
echo "Checking test data setup..."
rg -A 10 "endUsersWithTokens" .github/workflows/
Length of output: 251
Script:
#!/bin/bash
# Let's check for workflow files that might be using k6 or performance tests
echo "Checking workflow files for k6 or performance test configurations..."
fd -e yml -e yaml . .github/workflows/ --exec cat {}
# Also check for any k6 related configuration files
echo -e "\nChecking for k6 configuration files..."
fd k6 . --exec cat {}
Length of output: 69490
Script:
#!/bin/bash
# Let's check the workflow that runs k6 performance tests
echo "Checking k6 performance test workflow configuration..."
rg -A 10 "run-performance" .github/workflows/ci-cd-yt01.yml
# Also check the workflow that actually runs the k6 tests
echo -e "\nChecking k6 test execution workflow..."
rg -A 10 "K6_CLOUD_TOKEN|isSingleUserMode|traceCalls" .github/workflows/workflow-run-k6-performance.yml
Length of output: 1531
Script:
#!/bin/bash
# Let's check the actual k6 test execution script
echo "Checking k6 test execution script configuration..."
cat tests/k6/tests/enduser/performance/enduser-search.js
# Also check if there are any environment variable configurations in the test setup
echo -e "\nChecking k6 test setup files..."
fd -e js config tests/k6 --exec cat {}
Length of output: 4732
.github/workflows/ci-cd-yt01.yml (2)
153-154: Verify K6 Cloud configuration.
Ensure that the K6 Cloud token and project ID are properly configured in the repository secrets for the YT01 environment.
143-175: Verify workflow integration.
The performance test job looks well-configured, but let's verify:
- Integration with the failure notification job
- Proper triggering conditions
- Workflow file syntax
✅ Verification successful
Let me gather more information about the workflow integration, particularly focusing on the failure notification setup and job dependencies.
Let me check one final aspect regarding the workflow integration and failure handling.
Performance test job is properly integrated and configured
The verification confirms that:
- The job has correct failure handling with
if: ${{ always() && !failure() && !cancelled() }}condition - It's properly integrated into the workflow's dependency chain with
needs: [deploy-apps, deploy-infra, check-for-changes] - The job's results are included in the Slack failure notification system
- The reusable workflow configuration is complete with all required inputs and secrets
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify workflow integration
# Check if the job is properly referenced in the failure notification job
echo "Checking failure notification job dependencies..."
rg "needs.*run-performance" .github/workflows/ci-cd-yt01.yml
# Verify workflow syntax
echo "Validating workflow syntax..."
gh workflow view "ci-cd-yt01.yml"
# Check for any hardcoded environment references
echo "Checking for hardcoded environment references..."
rg -i "yt01" .github/workflows/ci-cd-yt01.yml
Length of output: 1506
Script:
#!/bin/bash
# Check for failure notification job and its dependencies
echo "Checking failure notification configuration..."
rg -A 10 "failure-notification:" .github/workflows/ci-cd-yt01.yml
# Check the reusable workflow for k6 performance tests
echo "Checking k6 performance workflow..."
cat .github/workflows/workflow-run-k6-performance.yml
# Verify if there are any other jobs depending on run-performance
echo "Checking dependencies on run-performance..."
rg "needs:.*run-performance" .github/workflows/
Length of output: 2554
Script:
#!/bin/bash
# Check for any if conditions or job dependencies that might affect the workflow
echo "Checking workflow conditions and dependencies..."
rg -B 2 -A 5 "if: .*failure\(\)" .github/workflows/ci-cd-yt01.yml
# Check for any post-job actions or notifications
echo "Checking post-job configurations..."
rg "post:" .github/workflows/ci-cd-yt01.yml -A 10
# Check the complete job definition to understand the failure handling
echo "Checking complete run-performance job..."
rg -B 5 -A 30 "run-performance:" .github/workflows/ci-cd-yt01.yml
Length of output: 4793
🧰 Tools
🪛 yamllint (1.35.1)
[error] 155-155: trailing spaces
(trailing-spaces)
[error] 159-159: trailing spaces
(trailing-spaces)
[error] 163-163: trailing spaces
(trailing-spaces)
|
Add performance tests and run these tests in ci-cd-yt01 workflow
Description
Related Issue(s)
#1540
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)