feat(performance): Performance/create serviceowner search #1413
Conversation
… and an enduser(ssn)
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files related to K6 performance testing. Key changes include the addition of a new test suite path in the workflow configuration, enhancements to the 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: 4
🧹 Outside diff range and nitpick comments (6)
tests/k6/tests/serviceowner/performance/serviceowner-search.js (2)
5-5: Consider using kebab-case for the tag name.Using spaces in metric tags might cause issues with some monitoring systems. Consider using kebab-case instead.
-const tag_name = 'serviceowner search'; +const tag_name = 'serviceowner-search';
7-10: Fix array parameter formatting.There's a missing space after the comma in the array parameter.
- thresholds: getDefaultThresholds(['http_req_duration', 'http_reqs'],[tag_name]) + thresholds: getDefaultThresholds(['http_req_duration', 'http_reqs'], [tag_name])tests/k6/tests/scenarios/performance/create-dialog-and-search.js (1)
Line range hint
28-46: Consider improving naming consistency and adding service owner search thresholds.
The scenario name
simple_searchshould be updated toenduser_searchto maintain consistency with the threshold label 'enduser search'.Since this PR implements service owner search functionality, consider adding corresponding performance thresholds for it.
Here's the suggested update:
scenarios: { create_dialogs: { executor: 'constant-vus', tags: { name: 'create dialog'}, exec: 'createDialogs', vus: __ENV.svus, duration: __ENV.duration }, - simple_search: { + enduser_search: { executor: 'constant-vus', tags: { name: 'search dialogs'}, exec: 'enduserSearches', vus: __ENV.evus, duration: __ENV.duration, } }, summaryTrendStats: ['avg', 'min', 'med', 'max', 'p(95)', 'p(99)', 'p(99.5)', 'p(99.9)', 'count'], thresholds: getDefaultThresholds(['http_req_duration', 'http_reqs'],[ 'enduser search', + 'serviceowner search', 'create dialog', 'get dialog',tests/k6/tests/scripts/generate_tokens.sh (1)
Line range hint
85-86: Apply consistent URL parameter encoding for personal tokens.For consistency and security, consider applying the same URL encoding approach to the personal token generation.
- url="https://altinn-testtools-token-generator.azurewebsites.net/api/GetPersonalToken?env=$env&scopes=$scopes&pid=$ssn&ttl=3600" - token=$(curl -s -f $url -u "$tokengenuser:$tokengenpasswd" ) + url="https://altinn-testtools-token-generator.azurewebsites.net/api/GetPersonalToken" + token=$(curl -s -f --get \ + --data-urlencode "env=$env" \ + --data-urlencode "scopes=$scopes" \ + --data-urlencode "pid=$ssn" \ + --data-urlencode "ttl=3600" \ + $url -u "$tokengenuser:$tokengenpasswd" )tests/k6/tests/performancetest_common/simpleSearch.js (2)
34-37: Fix grammatical error in JSDoc comment.Change "Performs a enduser search" to "Performs an enduser search" to fix the article usage.
/** - * Performs a enduser search. + * Performs an enduser search. * @param {Object} enduser - The end user. * @returns {void} */
131-135: Complete the JSDoc documentation.The JSDoc is incomplete and contains unclear parameter types.
/** * Performs a serviceowner search. - * @param {P} serviceowner - * @param {*} enduser + * @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 tag for the request. + * @returns {void} */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tests/k6/tests/performancetest_data/serviceowners-yt01.csvis excluded by!**/*.csv
📒 Files selected for processing (5)
.github/workflows/dispatch-k6-performance.yml(1 hunks)tests/k6/tests/performancetest_common/simpleSearch.js(3 hunks)tests/k6/tests/scenarios/performance/create-dialog-and-search.js(1 hunks)tests/k6/tests/scripts/generate_tokens.sh(1 hunks)tests/k6/tests/serviceowner/performance/serviceowner-search.js(1 hunks)
🔇 Additional comments (7)
tests/k6/tests/serviceowner/performance/serviceowner-search.js (1)
1-26: Verify PII handling in performance metrics.
Since this test handles sensitive PII (SSNs), ensure that:
- No PII is included in the performance metrics or logs
- Test data is properly anonymized
- Metrics tags don't contain any sensitive information
Let's verify the PII handling in the common modules:
✅ Verification successful
Let me gather more information about how the test data and metrics are handled.
Based on the code analysis, I can now provide the final response:
PII handling in performance metrics appears secure
The implementation shows proper handling of sensitive data:
- Metrics tags only contain generic names like 'serviceowner search' without PII
- SSNs are only used in API requests, not exposed in logs or metrics
- Performance thresholds are configured with generic counters and labels
- Test data is loaded via SharedArray which provides isolation
The only recommendations would be:
- Consider obfuscating the SSN in the URN format (
urn:altinn:person:identifier-no:) when constructing API requests - Add input validation for the SSN format in the test data loading process
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential PII logging or metrics
echo "Checking for potential PII exposure in related files..."
# Check simpleSearch.js for console.log statements or metrics that might expose PII
rg -A 3 "console\.|check\(|add\(" "tests/k6/tests/performancetest_common/simpleSearch.js"
# Check test data handling
rg -A 3 "endUsersWithTokens|serviceOwners" "tests/k6/tests/performancetest_common/readTestdata.js"
Length of output: 676
Script:
#!/bin/bash
# Check the content structure of test data files
echo "Checking test data handling..."
fd -e csv . tests/k6/tests/performancetest_common/ -x head -n 2 {}
# Check metric tags and logging in simpleSearch implementation
echo "Checking simpleSearch implementation..."
cat tests/k6/tests/performancetest_common/simpleSearch.js
# Check metric configuration
echo "Checking metric configuration..."
cat tests/k6/tests/performancetest_common/getDefaultThresholds.js
Length of output: 6540
.github/workflows/dispatch-k6-performance.yml (2)
52-52: LGTM: New test suite path properly integrated
The addition of the service owner search test path follows the established pattern and integrates well with the existing workflow configuration.
52-52: Verify PII data handling in performance tests
Since this test suite involves searching by social security numbers (SSN), ensure that:
- Test data uses anonymized/synthetic SSNs
- Performance test logs don't expose sensitive data
- K6 cloud storage complies with data protection requirements
✅ Verification successful
Based on the gathered information, I can now provide the final response:
Test data and PII handling appear to be properly managed
The performance test implementation shows appropriate handling of sensitive data:
- SSNs are loaded from environment-specific CSV files that are gitignored (prefixed with
.) - Test data is loaded via a structured process using
readTestdata.js - No direct logging of sensitive information in the search implementations
- SSNs are only used as identifiers in API calls and properly formatted as URNs
- Test data appears to use synthetic test users (e.g., "UROMANTISK LITTERATUR")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential PII exposure in test files and logs
# Test 1: Look for SSN patterns in test files
echo "Checking for SSN patterns in test files..."
rg -i "ssn|social|security|number" "tests/k6"
# Test 2: Check for any sensitive data logging
echo "Checking for console logging in the test file..."
rg -i "console.log|debug|print" "tests/k6/tests/serviceowner/performance/serviceowner-search.js"
Length of output: 4401
Script:
#!/bin/bash
# Let's check the specific performance test file for SSN handling and test data
# Check the content of the performance test file
echo "Checking serviceowner-search.js content..."
cat "tests/k6/tests/serviceowner/performance/serviceowner-search.js"
# Check if there are any test data files referenced
echo -e "\nChecking for test data file references..."
rg -l "test.*data" "tests/k6/tests/serviceowner/performance/serviceowner-search.js"
# Check the config file for any additional SSN configurations
echo -e "\nChecking config file for SSN configurations..."
cat "tests/k6/common/config.js"
Length of output: 4751
Script:
#!/bin/bash
# Let's check the test data loading and search implementation
# Check the test data loading implementation
echo "Checking test data loading implementation..."
cat "tests/k6/tests/performancetest_common/readTestdata.js"
# Check the search implementation
echo -e "\nChecking search implementation..."
cat "tests/k6/tests/performancetest_common/simpleSearch.js"
# Check if there are any CSV test data files
echo -e "\nChecking for CSV test data files..."
fd -e csv -x cat {} \; tests/k6/tests/performancetest_data/
Length of output: 8498
tests/k6/tests/scenarios/performance/create-dialog-and-search.js (1)
45-45: LGTM: Threshold label update is appropriate.
The change from 'simple search' to 'enduser search' better reflects the specific functionality being tested and aligns with the enduserSearch function name.
tests/k6/tests/scripts/generate_tokens.sh (1)
60-61: LGTM! Proper URL encoding of parameters.
The change to use --get with --data-urlencode for the scopes parameter is a good security practice that ensures proper URL encoding of potentially special characters.
tests/k6/tests/performancetest_common/simpleSearch.js (2)
8-8: LGTM: Import changes align with new functionality.
The addition of getSO import is consistent with the new service owner search feature.
136-153: Verify the impact on performance metrics.
Since this is a performance test, we should ensure that the new service owner search functionality is properly integrated with the performance monitoring system.
✅ Verification successful
Let me gather more information about the performance test configuration and metrics by checking the content of the identified files.
Let me check the default thresholds configuration since it's imported from a common file.
Performance monitoring is properly configured for the service owner search functionality
The service owner search functionality is well-integrated with the performance monitoring system:
- Uses standard K6 performance metrics (http_req_duration, http_reqs)
- Implements default thresholds including error rate monitoring (rate<0.01)
- Properly tagged for metric collection with 'serviceowner search'
- Follows the same performance testing pattern as other endpoints (graphql-search, enduser-search)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for performance metric configurations and thresholds
# Test 1: Look for performance thresholds in config files
echo "Checking for performance thresholds..."
rg -l "threshold|duration|vus|target" tests/k6/config/
# Test 2: Check if the new endpoint is included in any existing performance scenarios
echo "Checking for service owner search usage in test scenarios..."
rg -l "serviceownerSearch" tests/k6/tests/
Length of output: 491
Script:
#!/bin/bash
# Check the content of files that use serviceownerSearch
echo "Content of serviceowner-search.js:"
cat tests/k6/tests/serviceowner/performance/serviceowner-search.js
echo -e "\nChecking for performance configurations in the test files:"
rg "options|threshold|duration|vus|target" tests/k6/tests/serviceowner/performance/serviceowner-search.js
Length of output: 1772
Script:
#!/bin/bash
# Check the default thresholds configuration
echo "Content of getDefaultThresholds.js:"
cat tests/k6/tests/performancetest_common/getDefaultThresholds.js
# Check if there are any other performance test configurations
echo -e "\nChecking for other performance test configurations:"
rg -g '!node_modules' -l "getDefaultThresholds" tests/k6/tests/
Length of output: 1450
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests/k6/tests/performancetest_common/simpleSearch.js (2)
131-135: Improve function documentation.The JSDoc comments should include:
- Proper type for
enduserparameter- Descriptions for all parameters
- Return type and description
/** * Performs a serviceowner search. - * @param {P} serviceowner - * @param {*} enduser + * @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 + * @returns {void} */
136-147: Add basic input validation.While URL encoding is properly implemented, basic validation of required parameters would improve robustness.
export function serviceownerSearch(serviceowner, enduser, tag_name) { + if (!serviceowner?.token || !serviceowner?.resource) { + throw new Error('Missing required serviceowner parameters: token or resource'); + } + if (!enduser?.ssn) { + throw new Error('Missing required enduser parameter: ssn'); + } + let paramsWithToken = {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
tests/k6/tests/performancetest_common/simpleSearch.js(3 hunks)tests/k6/tests/serviceowner/performance/serviceowner-search.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/k6/tests/serviceowner/performance/serviceowner-search.js
🧰 Additional context used
📓 Learnings (1)
tests/k6/tests/performancetest_common/simpleSearch.js (1)
Learnt from: dagfinno
PR: digdir/dialogporten#1413
File: tests/k6/tests/performancetest_common/simpleSearch.js:148-152
Timestamp: 2024-11-07T08:34:20.609Z
Learning: In the `serviceownerSearch` function in `tests/k6/tests/performancetest_common/simpleSearch.js`, additional error handling for the response is not required.
🔇 Additional comments (3)
tests/k6/tests/performancetest_common/simpleSearch.js (3)
8-8: LGTM!
The addition of getSO import is appropriate for the new service owner search functionality.
34-34: LGTM!
The documentation update improves clarity by explicitly stating the function's purpose.
148-152: LGTM!
The test implementation follows the established pattern and includes appropriate response validation.
|
🤖 I have created a release *beep* *boop* --- ## [1.30.0](v1.29.0...v1.30.0) (2024-11-08) ### Features * **performance:** Performance/create serviceowner search ([#1413](#1413)) ([f1096a4](f1096a4)) * **webapi:** Combine actorDtos ([#1374](#1374)) ([ca18a99](ca18a99)) * **webapi:** Limit Content-Length / request body size ([#1416](#1416)) ([44be20a](44be20a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Add a simple serviceowner search on enduser(ssn) and service resource
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit