feat(performance): Refactoring and tracing #1489
Conversation
…d function-headers
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to several K6 performance test scripts and workflow configurations. Key changes include the addition of tracing capabilities through new parameters and constants, such as 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
|
dagfinno
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (17)
tests/k6/tests/enduser/performance/enduser-search.js (1)
5-6: Consider adding input validation for environment variablesThe environment variable parsing is functional but could be more robust. Consider adding explicit validation and documentation.
+// Environment configuration +// __ENV.isSingleUserMode: 'true' | 'false' - Controls whether to test with single or multiple users +// __ENV.traceCalls: 'true' | 'false' - Controls whether to enable tracing for performance analysis + -const isSingleUserMode = (__ENV.isSingleUserMode ?? 'false') === 'true'; -const traceCalls = (__ENV.traceCalls ?? 'false') === 'true'; +const isSingleUserMode = validateBooleanEnv('isSingleUserMode'); +const traceCalls = validateBooleanEnv('traceCalls'); + +function validateBooleanEnv(name) { + const value = __ENV[name] ?? 'false'; + if (value !== 'true' && value !== 'false') { + throw new Error(`Invalid value for ${name}. Expected 'true' or 'false', got '${value}'`); + } + return value === 'true'; +}tests/k6/tests/serviceowner/performance/create-dialog.js (2)
15-16: Add JSDoc comments to document the new configuration flags.While the implementation is correct, these configuration flags would benefit from documentation explaining their purpose and impact on the test execution.
Consider adding JSDoc comments and slightly refactoring:
+/** + * Determines if the test should run in single user mode. + * When true, only tests with the first service owner and end user. + * @type {boolean} + */ const isSingleUserMode = (__ENV.isSingleUserMode ?? 'false') === 'true'; + +/** + * Enables tracing of API calls during test execution. + * When true, includes additional tracing parameters in requests. + * @type {boolean} + */ const traceCalls = (__ENV.traceCalls ?? 'false') === 'true';
30-33: Consider parallel execution and discuss test characteristics change.The current changes have two implications:
- Using a single
serviceOwnerfor all iterations is more efficient but differs from the previous random selection behavior. This changes the test characteristics and might not reflect real-world usage patterns as accurately.- Sequential execution might not stress test the system sufficiently for performance testing.
Consider these improvements:
- Add a comment explaining the reasoning behind removing randomization
- Consider using K6's parallel execution features for better performance testing:
// Example using batch requests const BATCH_SIZE = 10; let serviceOwner = randomItem(serviceOwners); for (let i = 0; i < endUsers.length; i += BATCH_SIZE) { let batch = endUsers.slice(i, i + BATCH_SIZE); batch.forEach(endUser => { createDialog(serviceOwner, endUser, traceCalls); }); }tests/k6/tests/serviceowner/performance/create-remove-dialog.js (2)
10-11: Consider improving environment variable handling.The current implementation could be made more robust:
- Environment variable names should be prefixed (e.g.,
K6_IS_SINGLE_USER_MODE) to avoid conflicts- Boolean conversion could be more explicit
Consider this alternative implementation:
-const isSingleUserMode = (__ENV.isSingleUserMode ?? 'false') === 'true'; -const traceCalls = (__ENV.traceCalls ?? 'false') === 'true'; +const isSingleUserMode = Boolean(__ENV.K6_IS_SINGLE_USER_MODE ?? false); +const traceCalls = Boolean(__ENV.K6_TRACE_CALLS ?? false);
Line range hint
1-7: Update file documentation to include tracing details.The file header comments should be updated to document:
- The new environment variables and their purpose
- How tracing affects the test execution
- Where to find the trace output
Consider adding this to the header comment:
/** * Performance test for creating and removing a dialog * Run: k6 run tests/k6/tests/serviceowner/performance/create-remove-dialog.js --vus 1 --iterations 1 + * + * Environment variables: + * - K6_IS_SINGLE_USER_MODE: When 'true', runs test with single user/service owner pair + * - K6_TRACE_CALLS: When 'true', enables detailed tracing of HTTP calls */tests/k6/tests/graphql/performance/graphql-search.js (1)
10-11: Consider using a more robust string comparison for boolean environment variablesThe current implementation only checks for 'true'. Consider using a more comprehensive check that handles various truthy values and is case-insensitive.
-const isSingleUserMode = (__ENV.isSingleUserMode ?? 'false') === 'true'; -const traceCalls = (__ENV.traceCalls ?? 'false') === 'true'; +const isSingleUserMode = (/^true$/i).test(__ENV.isSingleUserMode ?? 'false'); +const traceCalls = (/^true$/i).test(__ENV.traceCalls ?? 'false');tests/k6/tests/serviceowner/performance/serviceowner-search.js (1)
39-42: Consider performance optimization for multi-user mode.The current implementation processes all end users for a random service owner, which could lead to excessive test duration with large datasets. Consider:
- Adding a configurable batch size to limit the number of users processed
- Implementing random sampling of end users instead of processing all
else { let serviceOwner = randomItem(serviceOwners); - for (let i = 0; i < endUsers.length; i++) { - serviceownerSearch(serviceOwner, endUsers[i], tag_name, traceCalls); + // Get batch size from env var or default to 10 + const batchSize = parseInt(__ENV.testBatchSize ?? '10'); + // Take random sample of users up to batchSize + const selectedUsers = endUsers + .sort(() => Math.random() - 0.5) + .slice(0, batchSize); + for (const user of selectedUsers) { + serviceownerSearch(serviceOwner, user, tag_name, traceCalls); } }.github/workflows/workflow-run-k6-performance.yml (2)
49-50: LGTM! Consider enhancing the output handling.The addition of CSV output alongside cloud output is a good approach for local analysis. However, consider saving the results file as an artifact for future reference.
Add these steps after the K6 run:
- name: Upload test results uses: actions/upload-artifact@v3 with: name: k6-results path: results.csv retention-days: 14
51-51: Fix trailing whitespace.There is a trailing whitespace at the end of the line.
Apply this diff:
- grep http_req_duration ./results.csv | sort --field-separator=',' --key=3 -nr | head -10 + grep http_req_duration ./results.csv | sort --field-separator=',' --key=3 -nr | head -10🧰 Tools
🪛 yamllint
[error] 51-51: trailing spaces
(trailing-spaces)
tests/k6/tests/performancetest_common/createDialog.js (3)
16-18: Update JSDoc to include the newtraceCallsparameterThe function signature has been updated, but the JSDoc comments don't reflect the new
traceCallsparameter. This should be documented for better maintainability./** * Creates a dialog. * * @param {Object} serviceOwner - The service owner object. * @param {Object} endUser - The end user object. + * @param {boolean} traceCalls - Whether to include tracing information in the request. */
44-55: Extract common parameter initialization logicThe parameter initialization logic is duplicated between
createDialogandcreateAndRemoveDialog. Consider extracting this into a shared helper function.+function initializeRequestParams(serviceOwner, endUser, traceCalls, traceparent) { + const params = { + headers: { + Authorization: "Bearer " + serviceOwner.token, + traceparent: traceparent + }, + tags: { name: 'create dialog' } + }; + if (traceCalls) { + params.tags.traceparent = traceparent; + params.tags.enduser = endUser.ssn; + } + return params; +}Also, update the JSDoc comments here to include the new parameter.
60-60: Remove redundant tag name assignmentThe tag name 'create dialog' is already set during parameter initialization. This line appears to be redundant.
- paramsWithToken.tags.name = 'create dialog';tests/k6/tests/serviceowner/performance/purge-dialogs.js (3)
18-18: Consider a more robust environment variable parsingWhile the current implementation works, consider making it more explicit and robust.
-const traceCalls = (__ENV.traceCalls ?? 'false') === 'true'; +const traceCalls = ['true', '1', 'yes'].includes((__ENV.traceCalls ?? 'false').toLowerCase());
Line range hint
45-60: Consider implementing batch processing for large result setsThe current implementation collects all dialog IDs in memory before processing. For large result sets, this could lead to memory issues.
Consider processing dialogs in batches:
const BATCH_SIZE = 1000; let dialogIdsBatch = []; // Process in batches if (dialogIdsBatch.length >= BATCH_SIZE) { await processBatch(dialogIdsBatch); dialogIdsBatch = []; }
Line range hint
134-141: Enhance error handling and loggingThe error handling could be more informative and structured.
let r = purgeSO('dialogs/' + dialogIdsToPurge[i], paramsWithToken); -if (r.status != 204) { - console.error("Failed to purge dialog with id: " + dialogIdsToPurge[i]); - console.log(r); -} -else { - console.log("Purged dialog with id: " + dialogIdsToPurge[i]); - dialogIdsToPurge.splice(i, 1); -} +if (r.status !== 204) { + console.error(`Failed to purge dialog ${dialogIdsToPurge[i]}:`, { + status: r.status, + traceparent: paramsWithToken.headers.traceparent, + response: r.body + }); + continue; +} +console.log(`Successfully purged dialog ${dialogIdsToPurge[i]}`, { + traceparent: paramsWithToken.headers.traceparent +}); +dialogIdsToPurge.splice(i, 1);tests/k6/tests/performancetest_common/simpleSearch.js (2)
Line range hint
156-184: Improve function documentationThe function implementation is good, but the documentation needs improvement:
- Parameter types and descriptions are missing
- Return value is not documented
Add JSDoc documentation:
/** * Performs a serviceowner search. - * @param {P} serviceowner - * @param {*} enduser - * @param {*} tag_name + * @param {Object} serviceowner - The service owner object containing token and resource + * @param {Object} enduser - The end user object containing SSN + * @param {string} tag_name - The name to use for request tagging + * @param {boolean} traceCalls - Whether to enable tracing + * @param {boolean} [doSubqueries=true] - Whether to perform sub-queries + * @returns {Object} The response object from the search request */
Line range hint
44-184: Consider centralizing tracing logicThe tracing implementation is duplicated across search functions. Consider extracting the common tracing logic into a helper function to improve maintainability and reduce duplication.
Example implementation:
function addTracing(params, traceCalls, identifier) { const traceparent = uuidv4(); const paramsWithTracing = { ...params, headers: { ...params.headers, traceparent } }; if (traceCalls) { paramsWithTracing.tags = { ...params.tags, traceparent, ...(identifier && { identifier }) }; } return paramsWithTracing; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tests/k6/tests/performancetest_data/endusers-yt01.csvis excluded by!**/*.csv
📒 Files selected for processing (9)
.github/workflows/workflow-run-k6-performance.yml(1 hunks)tests/k6/tests/enduser/performance/enduser-search.js(2 hunks)tests/k6/tests/graphql/performance/graphql-search.js(2 hunks)tests/k6/tests/performancetest_common/createDialog.js(2 hunks)tests/k6/tests/performancetest_common/simpleSearch.js(7 hunks)tests/k6/tests/serviceowner/performance/create-dialog.js(1 hunks)tests/k6/tests/serviceowner/performance/create-remove-dialog.js(2 hunks)tests/k6/tests/serviceowner/performance/purge-dialogs.js(4 hunks)tests/k6/tests/serviceowner/performance/serviceowner-search.js(2 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/workflow-run-k6-performance.yml
[error] 51-51: trailing spaces
(trailing-spaces)
🔇 Additional comments (19)
tests/k6/tests/enduser/performance/enduser-search.js (2)
31-33: LGTM! Comprehensive testing of all users
The implementation now systematically tests all users when not in single-user mode, which provides better coverage. The addition of the traceCalls parameter aligns with the PR objective of enhancing output capabilities.
28-28: Verify the impact of using a fixed user instead of random selection
The change from random to fixed user selection in single-user mode might affect test coverage. Consider whether this aligns with your testing strategy.
✅ Verification successful
Fixed user selection in single-user mode is acceptable
The change from random to fixed user selection in single-user mode is acceptable because:
- The main test file (enduser-search.js) now includes a loop mode that tests all users, providing better coverage than random selection
- Random selection is still preserved in scenario tests (create-dialog-and-search.js) where randomization is more relevant for simulating real-world usage patterns
- Single-user mode appears to be primarily used for debugging/tracing purposes, where consistent reproduction with the same user is beneficial
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if random selection was important in other test files
rg -l "randomItem.*endUsersWithTokens" tests/k6/
Length of output: 115
Script:
#!/bin/bash
# Check the usage context of randomItem in the found file
rg -B 2 -A 2 "randomItem.*endUsersWithTokens" tests/k6/tests/scenarios/performance/create-dialog-and-search.js
# Check if there are other random selection patterns in enduser-search.js
rg -B 2 -A 2 "random|Math\.floor.*endUsersWithTokens" tests/k6/tests/enduser/performance/enduser-search.js
# Look for the enduserSearch function implementation
ast-grep --pattern 'function enduserSearch($_) { $$$ }'
Length of output: 538
tests/k6/tests/serviceowner/performance/create-dialog.js (1)
25-25: LGTM! Good code organization.
The empty line improves readability by clearly separating error handling from business logic.
tests/k6/tests/serviceowner/performance/create-remove-dialog.js (2)
33-36: Consider test coverage implications of the changes.
The multi-user mode changes raise some concerns:
- Using a single random service owner for all iterations instead of randomizing per iteration reduces test coverage
- Sequential iteration through end users instead of random selection might not represent real-world patterns as effectively
Would you like me to propose an alternative implementation that maintains better test coverage while still supporting tracing?
30-30: Verify createAndRemoveDialog function signature.
The function is being called with a new traceCalls parameter. Let's verify this change is implemented in the imported function.
✅ Verification successful
Function signature change is correctly implemented
The createAndRemoveDialog function in tests/k6/tests/performancetest_common/createDialog.js correctly accepts and handles the traceCalls parameter. The implementation properly uses this parameter to conditionally add tracing information to the request tags.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if createAndRemoveDialog function accepts traceCalls parameter
ast-grep --pattern 'function createAndRemoveDialog($_serviceOwner, $_endUser, $_traceCalls)' tests/k6/performancetest_common/createDialog.js
Length of output: 237
Script:
#!/bin/bash
# First, let's find the correct file containing createAndRemoveDialog
fd createDialog.js
# Then search for createAndRemoveDialog function definition in all js files
ast-grep --pattern 'function createAndRemoveDialog($_serviceOwner, $_endUser, $_)'
Length of output: 159
Script:
#!/bin/bash
# Let's check the content of the file to find the function definition
cat tests/k6/tests/performancetest_common/createDialog.js
# Also search for any direct references to createAndRemoveDialog
rg "createAndRemoveDialog" -A 2
Length of output: 3653
tests/k6/tests/graphql/performance/graphql-search.js (2)
33-33: Document the implications of single user mode testing
Using a single user for performance testing might not accurately represent real-world scenarios and could affect the validity of performance metrics.
Consider adding a comment explaining:
- When to use single user mode vs multi-user mode
- The impact on performance metrics
- Recommended scenarios for each mode
36-38: 🛠️ Refactor suggestion
Consider randomization in multi-user mode
Sequential iteration through users might create an artificial pattern that doesn't represent real-world usage. The previous implementation using randomItem() might have provided more realistic load patterns.
- for (let i = 0; i < endUsers.length; i++) {
- graphqlSearch(endUsers[i], traceCalls);
- }
+ // Shuffle users array to prevent sequential access patterns
+ const shuffledUsers = [...endUsers].sort(() => Math.random() - 0.5);
+ for (const user of shuffledUsers) {
+ graphqlSearch(user, traceCalls);
+ }tests/k6/tests/serviceowner/performance/serviceowner-search.js (3)
7-8: LGTM! Well-implemented environment variables.
Good implementation of environment variables with:
- Proper use of nullish coalescing for defaults
- Clear boolean conversion
- Consistent naming convention
28-33: LGTM! Robust input validation.
Good implementation of null checks with clear error messages for both endUsers and serviceOwners.
4-4: Verify the impact of renamed import.
The import has changed from endUsersWithTokens to endUsers. This could be a breaking change if other parts of the codebase still expect the old structure.
tests/k6/tests/performancetest_common/createDialog.js (2)
Line range hint 16-73: Implementation aligns well with PR objectives
The changes successfully implement conditional tracing for performance monitoring while maintaining backward compatibility. The implementation is clean and should have minimal performance impact since tracing is optional.
✅ Verification successful
Let me gather more information about the performance thresholds and test configurations.
Let me check how the tracing functionality is used in other performance tests to ensure consistency.
Based on the gathered information, I can now provide the final response:
Optional tracing implementation verified with minimal performance impact
The implementation has been verified to be sound because:
- The tracing is properly implemented as an optional feature through the
traceCallsparameter - Performance test infrastructure is in place with appropriate thresholds and monitoring via k6
- The changes maintain backward compatibility as tracing is only added when explicitly enabled
- The implementation follows the existing performance testing patterns in the codebase
The lack of explicit traceCalls usage in other tests suggests this is a new feature being gradually introduced, which is a good approach for monitoring the impact.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the impact on performance test execution times
rg -l "createDialog.*traceCalls" | xargs rg "performance|duration|timing" -A 3
Length of output: 1865
Script:
#!/bin/bash
# Check the default thresholds and performance configurations
ast-grep --pattern 'export function getDefaultThresholds($_) {
$$$
}'
# Also check for any existing performance metrics or timing configurations
rg "http_req_duration|threshold" tests/k6/tests/performancetest_common/ -A 3
Length of output: 1386
Script:
#!/bin/bash
# Check usage of traceCalls parameter across performance tests
rg "createDialog.*traceCalls.*true|createDialog.*traceCalls.*false" tests/k6/tests/ -B 2 -A 2
# Check if there are any existing performance metrics collection
rg "tags.*name.*duration|metrics.*duration" tests/k6/tests/ -A 3
Length of output: 162
24-29: Review PII exposure in tracing tags
The end user's SSN is being included in the tracing tags. Consider whether exposing this PII in tracing/monitoring data aligns with privacy requirements. If possible, use a non-identifying unique identifier instead.
✅ Verification successful
Let me gather more information about the usage of SSN in the codebase, particularly in performance testing context.
Let me check the actual test data files and other usage contexts to understand if this is test data or real PII.
SSN usage in tracing tags is acceptable for test environment
The SSN values being used are synthetic test data from CSV files (e.g., 14886498226, 10865299538) specifically created for performance testing environments (yt01, staging). These are not real Norwegian SSNs, and the code is only used in test scenarios. The tracing implementation is consistent across test files and serves a valid testing purpose.
- Test data files (
endusers-staging.csv,endusers-yt01.csv) contain synthetic SSNs - Usage is limited to performance test environments
- Tracing with SSN helps correlate test user actions across the system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other instances of SSN exposure in tracing
rg -l "enduser.*ssn.*tags"
Length of output: 1865
Script:
#!/bin/bash
# Check for SSN usage in performance test files
rg -l "ssn" tests/k6/
# Check the content of readTestdata.js to understand how endUsers are structured
cat tests/k6/tests/performancetest_common/readTestdata.js
Length of output: 2583
Script:
#!/bin/bash
# Check the content of test data files for SSN format/content
head -n 5 tests/k6/tests/performancetest_data/endusers-*.csv
# Check how SSN is used in other performance test files
rg "ssn" tests/k6/tests/performancetest_common/ -A 2 -B 2
Length of output: 4950
tests/k6/tests/serviceowner/performance/purge-dialogs.js (2)
32-33: LGTM: Well-structured tracing implementation
The tracing implementation with unique traceparent and proper tagging will help in request tracking and debugging.
Also applies to: 40-44
113-119: LGTM: Consistent tracing implementation
The tracing implementation maintains consistency with the getDialogs function and properly tracks each purge operation.
Also applies to: 129-133
tests/k6/tests/performancetest_common/simpleSearch.js (5)
Line range hint 44-64: LGTM: Well-structured tracing implementation
The tracing implementation follows good practices:
- Unique traceparent using UUID
- Conditional tag addition
- Clean integration with existing functionality
73-74: LGTM: Clear parameter documentation
The added documentation for path and getFunction parameters is clear and descriptive.
91-92: LGTM: Clear parameter documentation
The added documentation for endpoint and getFunction parameters is clear and descriptive.
115-115: LGTM: Clear parameter documentation
The added documentation for getFunction parameter is clear and descriptive.
131-149: LGTM: Well-structured tracing implementation with good request identification
The changes follow good practices:
- Consistent implementation with enduserSearch
- Added User-Agent header for request identification
- Clean tracing integration
Co-authored-by: Ole Jørgen Skogstad <[email protected]>
|
🤖 I have created a release *beep* *boop* --- ## [1.37.0](v1.36.0...v1.37.0) (2024-11-20) ### Features * **performance:** Refactoring and tracing ([#1489](#1489)) ([760c345](760c345)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Added more users to testdata, from 70 to 2799.
Output 50 slowest requests
Improve tracing
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
New Features
Bug Fixes
Documentation