chore(performance): refactor generating tokens #1843
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new function, Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)tests/k6/tests/enduser/performance/enduser-search.js (2)🔇 Additional comments (4)
✨ Finishing Touches
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
🧹 Nitpick comments (5)
tests/k6/tests/enduser/performance/enduserSearchBreakpoint.js (1)
17-17: Consider documenting the 5000ms threshold.The max duration threshold of 5000ms seems reasonable, but consider adding a comment explaining why this specific value was chosen.
tests/k6/tests/enduser/performance/enduserSearchWithThresholds.js (1)
1-3: Consider parameterizing the number of end users.The hardcoded value of 100 end users might not be suitable for all test scenarios. Consider making this configurable through environment variables for flexibility in different test environments.
-const numberOfEndUsers = 100; +const numberOfEndUsers = parseInt(__ENV.numberOfEndUsers ?? '100');tests/k6/tests/enduser/performance/enduser-search.js (1)
9-18: Consider documenting threshold values.While the thresholds are initialized as empty arrays, it would be helpful to document why these specific metrics are being tracked and what values will be assigned to them during test execution.
tests/k6/tests/graphql/performance/graphql-search.js (1)
19-28: Consider documenting performance expectations.While the thresholds are initialized as empty arrays, it would be helpful to document the expected performance metrics for GraphQL operations.
tests/k6/tests/serviceowner/performance/create-dialog.js (1)
20-23: Consider adding error handling for token validation.Given the PR's focus on bulk token fetching, ensure robust error handling when validating and using tokens.
Consider wrapping the token usage in a try-catch block:
export default function(data) { - const { endUsers, index } = validateTestData(data, serviceOwners); - createDialog(serviceOwners[0], endUsers[index], traceCalls); + try { + const { endUsers, index } = validateTestData(data, serviceOwners); + if (!endUsers[index]?.token) { + throw new Error('Invalid or missing token'); + } + createDialog(serviceOwners[0], endUsers[index], traceCalls); + } catch (error) { + console.error('Failed to create dialog:', error); + throw error; // Re-throw to fail the test + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
tests/k6/common/token.js(2 hunks)tests/k6/tests/enduser/performance/enduser-search.js(1 hunks)tests/k6/tests/enduser/performance/enduserSearchBreakpoint.js(1 hunks)tests/k6/tests/enduser/performance/enduserSearchWithThresholds.js(1 hunks)tests/k6/tests/graphql/performance/graphql-search.js(2 hunks)tests/k6/tests/performancetest_common/getDefaultThresholds.js(0 hunks)tests/k6/tests/performancetest_common/simpleSearch.js(5 hunks)tests/k6/tests/scenarios/performance/create-dialog-and-search.js(1 hunks)tests/k6/tests/serviceowner/performance/create-dialog.js(1 hunks)tests/k6/tests/serviceowner/performance/create-remove-dialog.js(1 hunks)tests/k6/tests/serviceowner/performance/create-transmissions.js(1 hunks)tests/k6/tests/serviceowner/performance/purge-dialogs.js(1 hunks)tests/k6/tests/serviceowner/performance/serviceowner-search.js(2 hunks)
💤 Files with no reviewable changes (1)
- tests/k6/tests/performancetest_common/getDefaultThresholds.js
🔇 Additional comments (17)
tests/k6/tests/enduser/performance/enduserSearchBreakpoint.js (3)
1-1: LGTM! Clean import consolidation.The imports have been nicely consolidated using named imports with aliases, improving code organization.
19-20: LGTM! Clean function refactoring.The setup and run functions have been simplified by delegating to imported functions, reducing code duplication.
8-16: Verify the 10m setupTimeout is sufficient.The setupTimeout of '10m' has been added to accommodate bulk token fetching. Please ensure this timeout is sufficient for your test environment and load conditions.
✅ Verification successful
🌐 Web query:
What is the recommended setupTimeout for k6 tests with bulk operations?💡 Result:
The recommended
setupTimeoutfor k6 tests involving bulk operations depends on the specific use case and system performance characteristics. Here’s a synthesis of best practices from available resources:
Default Timeout:
By default, k6 uses a 60-second timeout for HTTP requests and other actions[3][9]. This may need adjustment for bulk operations due to factors like large data volumes or network latency.Bulk-Specific Adjustments:
- For HTTP requests in bulk scenarios:
Set per-request timeouts usingparams.timeout. Community examples use 240–360 seconds (4–6 minutes)[4]:const params = { timeout: '360s' }; // Applied per HTTP call const res = http.get('https://api.example.com/bulk', params);- For browser automation (k6 Browser module):
Usecontext.setDefaultTimeout()to override defaults (e.g.,1000msfor page interactions)[1]:const context = await browser.newContext(); context.setDefaultTimeout(30000); // 30s for browser actionsGlobal Configuration:
If using libraries like Entity Framework Extensions for database bulk operations (outside k6), configureBatchTimeoutseparately (e.g., 180 seconds)[2]:context.BulkSaveChanges(options => options.BatchTimeout = 180);Recommendations:
- Start with 180–300 seconds (3–5 minutes) for bulk HTTP/data operations[2][4].
- Monitor CPU/memory usage during tests to avoid resource bottlenecks[3].
- For distributed load tests across Kubernetes or multiple machines, synchronize timeouts using
--execution-segment[3].Always validate timeout settings against real-world system behavior under load[4][9].
Citations:
- 1: https://grafana.com/docs/k6/latest/javascript-api/k6-browser/browsercontext/setdefaulttimeout/
- 2: https://entityframework-extensions.net/batch-timeout
- 3: https://grafana.com/docs/k6/latest/testing-guides/running-large-tests/
- 4: https://community.grafana.com/t/k6-timeout-config-with-syntax-example/99613
- 5: https://www.lambdatest.com/automation-testing-advisor/golang/methods/k6_go.httpext.Timeout
- 6: https://ultimateqa.com/k6-for-load-testing/
- 7: https://www.lambdatest.com/automation-testing-advisor/golang/methods/k6_go.http.Batch
- 8: https://github.com/grafana/xk6-loki
- 9: Make default HTTP request timeout configurable grafana/k6#3932
- 10: https://www.lambdatest.com/automation-testing-advisor/golang/methods/k6_go.events.setTimeout
10m setupTimeout appears sufficient for bulk token fetching.
The web resources indicate that while k6’s default HTTP request timeout is 60 seconds, bulk operations typically adjust per-request timeouts to around 240–360 seconds (4–6 minutes) or recommend starting with 180–300 seconds. Since the 10-minute (600-second) setup timeout far exceeds these values, it should comfortably accommodate bulk token fetching under typical load conditions.• Verify in your specific environment that the token fetching process completes within this window.
• Monitor the system during tests to ensure that the extended timeout doesn’t mask any underlying performance issues.tests/k6/tests/enduser/performance/enduserSearchWithThresholds.js (2)
4-18: LGTM! Performance thresholds are well-defined.The thresholds configuration is comprehensive, covering all relevant API endpoints with consistent p95 and p99 latency requirements.
19-20: LGTM! Setup function is concise and clear.The setup function effectively delegates to the imported setup function with the configured number of end users.
tests/k6/tests/enduser/performance/enduser-search.js (1)
31-35: LGTM! Test execution logic is clear and efficient.The random selection of end users and token retrieval is well-implemented.
tests/k6/tests/serviceowner/performance/create-transmissions.js (1)
12-17: LGTM! Threshold configuration is clear and consistent.The explicit threshold configuration for both dialog and transmission creation metrics aligns with the PR's performance focus.
tests/k6/tests/graphql/performance/graphql-search.js (3)
5-10: Standardize k6-utils version across test files.Using version 1.4.0 of k6-utils while other files use 1.1.0 could lead to inconsistencies.
This was previously flagged in enduser-search.js. The same verification script applies here.
30-36: Verify token scope configuration.The scope configuration is identical to enduser-search.js. Consider extracting this to a shared configuration.
This was previously flagged in enduser-search.js. The same verification script applies here.
38-42: LGTM! Test execution logic is clear and efficient.The random selection of end users and token retrieval is well-implemented, consistent with the approach in enduser-search.js.
tests/k6/common/token.js (1)
70-79: LGTM! The bulk token fetching implementation looks solid.The implementation includes proper error handling, timeout configuration, and JSON response parsing.
Let's verify that the timeout value is sufficient for bulk operations:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check if any token requests are timing out in the logs rg -A 5 "timeout.*token" .Length of output: 28
Timeout Configuration Appears Appropriate, but Manual Verification Is Recommended
Based on a code inspection, the 10-minute timeout (600,000ms) is correctly configured and used for bulk token operations, and no obvious log entries indicate timeout issues. However, since the automated grep for timeout log entries produced no output, I recommend a manual verification in a staging environment to ensure that no unexpected timeouts occur during bulk token requests.
- Action Item: Manually verify token request logs under heavy load to ensure no timeout issues are occurring.
tests/k6/tests/performancetest_common/simpleSearch.js (3)
12-29: LGTM! Well-structured performance thresholds.The new constant provides a clear and maintainable structure for tracking various HTTP metrics.
63-80: Verify token parameter handling in enduserSearch.The function now accepts an optional token parameter, falling back to generating a new token if none is provided. This change supports the bulk token fetching feature.
Let's verify the token parameter is being used correctly across the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for any direct token generation calls that could be replaced with bulk fetching rg -A 5 "getPersonalToken.*ssn.*scopes"Length of output: 1120
Token Parameter Handling Verification Complete
- The function conditionally calls
getPersonalTokenonly whentokenis null.- The search results confirm that the expected pattern for token generation appears in the codebase and no extraneous direct invocations were found.
- This confirms that the updated logic supports the bulk token fetching feature.
199-199: Verify the resource path construction change.The resource path now uses
enduser.resourceinstead ofserviceowner.resource. This change might affect functionality.Let's verify this change is consistent with the API requirements:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for other occurrences of resource path construction rg -A 5 "urn:altinn:resource"Length of output: 100747
Resource Path Construction Verified
The change to useenduser.resourcein constructing the resource path is consistent with our API’s design for end-user scenarios. The code and related tests (e.g., in end‐user dialog search modules) document the use of URNs constructed using the end user’s resource, while service owner endpoints continue to use their corresponding property.No inconsistencies were found when examining the usage of the
urn:altinn:resource:prefix across the codebase.tests/k6/tests/scenarios/performance/create-dialog-and-search.js (1)
43-64: LGTM! Clear and explicit threshold definitions.The thresholds are now defined explicitly rather than using a helper function, which improves clarity and maintainability.
tests/k6/tests/serviceowner/performance/purge-dialogs.js (1)
66-69: LGTM! Consistent threshold definition pattern.The threshold definitions follow the same explicit pattern as other files, maintaining consistency across the codebase.
tests/k6/tests/serviceowner/performance/create-dialog.js (1)
12-15: Verify the implications of empty threshold arrays.The new threshold configuration uses empty arrays
[]which effectively removes any performance constraints. This might not be the intended behavior for a performance test.Consider defining explicit threshold values to ensure performance requirements are met. For example:
thresholds: { - "http_req_duration{name:create dialog}": [], - "http_reqs{name:create dialog}": [] + "http_req_duration{name:create dialog}": ['p(95)<500'], // 95% of requests should complete within 500ms + "http_reqs{name:create dialog}": ['rate>100'] // Should maintain at least 100 requests per second }
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
tests/k6/tests/serviceowner/performance/serviceOwnerSearchBreakpoint.js (1)
8-18: Verify the HTTP duration threshold value.The configuration looks good, but please verify if the 5000ms max duration threshold aligns with your service level objectives (SLOs).
Consider:
- Is 5 seconds an acceptable maximum response time for your service?
- Does this threshold align with your production performance requirements?
- Should this value be configurable via environment variables like other parameters?
tests/k6/tests/serviceowner/performance/createDialogWithThresholds.js (1)
4-12: Consider adding thresholds for additional performance metrics.While the current thresholds cover response time, consider monitoring these additional metrics for comprehensive performance testing:
http_req_failed: Error rate monitoringhttp_req_sending/http_req_receiving: Network performanceiteration_duration: Overall test iteration performanceExample addition:
export let options = { summaryTrendStats: ['avg', 'min', 'med', 'max', 'p(95)', 'p(99)', 'p(99.5)', 'p(99.9)', 'count'], vus: 1, duration: "30s", thresholds: { "http_req_duration{name:create dialog}": ["p(95)<300", "p(99)<500"], - "http_reqs{name:create dialog}": [] + "http_reqs{name:create dialog}": [], + "http_req_failed{name:create dialog}": ["rate<0.01"], + "http_req_sending{name:create dialog}": ["p(95)<100"], + "http_req_receiving{name:create dialog}": ["p(95)<100"], + "iteration_duration": ["p(95)<500"] } }tests/k6/tests/serviceowner/performance/serviceOwnerSearchWithThresholds.js (2)
2-2: Simplify the setup export.The current export statement includes a redundant rename of the setup function.
Apply this diff to simplify the export:
-export { setup as setup } from '../../performancetest_common/readTestdata.js'; +export { setup } from '../../performancetest_common/readTestdata.js';🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
4-19: LGTM! Consider documenting threshold values.The configuration is well-structured with consistent performance thresholds across endpoints. The spread operator effectively reuses base thresholds from the imported configuration.
Consider adding a comment explaining the rationale behind the chosen threshold values (p95 < 100ms, p99 < 300ms) to help future maintainers understand the performance requirements.
tests/k6/tests/enduser/performance/enduserSearchWithThresholds.js (1)
3-3: Consider makingnumberOfEndUsersconfigurable.The hardcoded value of 100 with a TODO comment suggests this is temporary. Consider:
- Making it configurable via environment variables
- Adding documentation about the performance implications
-const numberOfEndUsers = 100; // Remove when altinn-testtools bulk get of endusers/tokens is fast +const numberOfEndUsers = parseInt(__ENV.NUMBER_OF_END_USERS ?? '100'); // Default to 100 until altinn-testtools bulk get is optimized
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/k6/tests/enduser/performance/enduser-search.js(1 hunks)tests/k6/tests/enduser/performance/enduserSearchBreakpoint.js(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/serviceOwnerSearchBreakpoint.js(2 hunks)tests/k6/tests/serviceowner/performance/serviceOwnerSearchWithThresholds.js(1 hunks)tests/k6/tests/serviceowner/performance/serviceowner-search.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/k6/tests/serviceowner/performance/serviceowner-search.js
🧰 Additional context used
🪛 Biome (1.9.4)
tests/k6/tests/serviceowner/performance/serviceOwnerSearchWithThresholds.js
[error] 2-2: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
🔇 Additional comments (11)
tests/k6/tests/serviceowner/performance/serviceOwnerSearchBreakpoint.js (3)
1-6: LGTM! Clean import structure and configuration.The imports and environment variable configuration are well-structured, providing good flexibility for test parameterization.
20-27: LGTM! Well-structured setup function.The setup function efficiently partitions test data across virtual users, which is crucial for proper load distribution.
29-30: LGTM! Clean and focused implementation.The default export function is concise and follows the single responsibility principle, properly delegating to the imported
runfunction.tests/k6/tests/serviceowner/performance/createDialogWithThresholds.js (1)
9-10: Improved metric specificity with named thresholds.The change to use named metrics (
{name:create dialog}) improves observability by making it clear which specific operation the thresholds apply to. This is a good practice for maintainability and debugging.However, the empty threshold array
[]forhttp_reqsmetric needs attention.Please confirm if this is intentional or if specific thresholds should be defined for request count monitoring. Common thresholds might include:
- Rate limits (requests per second)
- Total request count expectations
- Error rate thresholds
Example thresholds if needed:
- "http_reqs{name:create dialog}": [] + "http_reqs{name:create dialog}": ["rate>10", "count>100", "error_rate<1"]tests/k6/tests/serviceowner/performance/serviceOwnerSearchWithThresholds.js (1)
21-21: LGTM!The default export effectively forwards the data to the imported run function.
tests/k6/tests/enduser/performance/enduserSearchBreakpoint.js (4)
1-5: LGTM! Clean import consolidation and well-structured environment variables.The refactored imports improve code organization while maintaining clear naming conventions. The environment variables provide good configurability with sensible defaults.
12-13: Review the 5000ms threshold for HTTP requests.The maximum duration threshold of 5000ms (5 seconds) for HTTP requests might be:
- Too lenient for a performance test
- Too strict for bulk operations
Please verify if this threshold aligns with your performance requirements and the expected behavior of the bulk-fetch operations.
21-22: LGTM! Clean function delegation.The simplified function exports improve code maintainability by delegating to the imported functions, which aligns well with the PR's goal of utilizing the new bulk-fetch method.
9-9: Verify if 10-minute setup timeout is necessary.The setupTimeout of '10m' seems high. While this might be needed for bulk token fetching, please verify if this duration is actually required or if it can be optimized.
✅ Verification successful
🌐 Web query:
What is the recommended setupTimeout for k6 tests with bulk operations?💡 Result:
The recommended
setupTimeoutfor k6 tests with bulk operations depends on your specific requirements, but here's a structured approach based on best practices and documentation:
Default Behavior
k6 has no explicitsetupTimeoutparameter by default - timeouts are generally controlled through:
- HTTP request timeouts: Default 60s ([9][10])
- Browser operations: Configurable via
browser.newContext().setDefaultTimeout()([1])Bulk Operation Considerations
For heavy initialization/bulk operations:export let options = { setupTimeout: '300s', // 5 minutes (adjust based on operation size) teardownTimeout: '120s', scenarios: { main: { // scenario configuration } } };
- Entity Framework Example: Uses 180s timeout for batch operations ([2])
- Large Test Guidance: k6 can handle 30k-40k VUs with proper resource monitoring ([3])
Key Recommendations
- Start with
setupTimeout: '300s'(5 minutes) for moderate bulk operations- Monitor execution and adjust using this formula:
Required Timeout = (Estimated Operation Duration) * 1.5- For massive datasets (>100k records), consider:
setupTimeout: '600s' // 10 minutesComplementary Configuration
Combine with other timeout controls:const params = { timeout: '240s' // HTTP request timeout ([4][8]) }; // For browser-based bulk ops const context = await browser.newContext(); context.setDefaultTimeout(60000); // 60s ([1])Always validate timeout settings under realistic load conditions and monitor system resources during test execution ([3][7]). For distributed executions using Kubernetes, allocate additional buffer time for coordination overhead ([3]).
Citations:
- 1: https://grafana.com/docs/k6/latest/javascript-api/k6-browser/browsercontext/setdefaulttimeout/
- 2: https://entityframework-extensions.net/batch-timeout
- 3: https://grafana.com/docs/k6/latest/testing-guides/running-large-tests/
- 4: https://community.grafana.com/t/k6-timeout-config-with-syntax-example/99613
- 5: https://www.lambdatest.com/automation-testing-advisor/golang/methods/k6_go.http.Batch
- 6: https://ultimateqa.com/k6-for-load-testing/
- 7: https://www.lambdatest.com/automation-testing-advisor/golang/methods/k6_go.events.setTimeout
- 8: https://www.lambdatest.com/automation-testing-advisor/golang/methods/k6_go.httpext.Timeout
- 9: Make default HTTP request timeout configurable grafana/k6#3932
- 10: Per-VU init lifecycle function grafana/k6#785
Review Verification: Timeout Duration Analysis
The 10-minute timeout in the test configuration aligns with guidance for massive bulk operations (e.g., when processing >100k records) as mentioned in the documentation. However, for moderate bulk operations, a 5-minute timeout is typically sufficient. Please verify that the bulk token fetching in your tests truly requires the extended timeout. If the operation normally completes faster, consider reducing the timeout to around 5 minutes.
tests/k6/tests/enduser/performance/enduserSearchWithThresholds.js (1)
8-19: Document the rationale for performance thresholds.The thresholds (p95 < 300ms, p99 < 500ms) are consistently applied across all endpoints. Consider:
- Adding documentation explaining how these thresholds were determined
- Whether different endpoints might need different thresholds based on their complexity
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for documentation about performance thresholds rg -l "performance|threshold" docs/Length of output: 37
Missing Documentation for Performance Thresholds
The verification shows no documentation exists (e.g., in the docs/ directory) explaining how the thresholds (p95 < 300ms and p99 < 500ms) were determined. It’s unclear if these values have been benchmarked or adjusted per endpoint complexity. Please document the rationale behind these performance thresholds, and consider if different endpoints may require different criteria.
tests/k6/tests/enduser/performance/enduser-search.js (1)
3-3: Update k6-utils version for consistency.The file uses k6-utils v1.1.0 while other files use v1.4.0. This inconsistency could lead to compatibility issues.
🤖 I have created a release *beep* *boop* --- ## [1.50.2](v1.50.1...v1.50.2) (2025-02-12) ### Miscellaneous Chores * **ci:** Set OTEL sample ratio to 0 in yt01 ([#1861](#1861)) ([1c3908c](1c3908c)) * **deps:** Add FusionCache package grouping in Renovate config ([#1860](#1860)) ([d8f00d0](d8f00d0)) * **deps:** update dependency verify.xunit to 28.10.1 ([#1838](#1838)) ([d44593a](d44593a)) * **performance:** refactor generating tokens ([#1843](#1843)) ([c9933e1](c9933e1)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Use the new bulk-fetch of endusers/tokens implemented in Altinn-testtools. Tokens are now fetched in the k6 setup phase.
Also done some needed refactoring
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)