DRAFT: add first draft of load simulator#172
Conversation
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive k6-based load testing simulator for the Stickerlandia application. The simulator tests public endpoints, OAuth 2.1 authentication flows with PKCE, and user registration under concurrent load, with full Docker Compose orchestration and Datadog APM integration.
Changes:
- Added k6-based load test script with support for multiple scenarios (public browsing, authenticated flows, registration)
- Created Docker Compose overlay configuration for load testing that disables rate limiting and includes all service dependencies
- Added 7 mise tasks for running different load test scenarios (smoke tests and sustained load tests)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| mise.toml | Added 7 load testing tasks for running smoke and sustained load tests with different scenario combinations |
| load-tests/load-test.js | Core k6 test script implementing OAuth 2.1 flows, registration, and public browsing scenarios with custom URL rewriting for Docker networking |
| load-tests/README.md | User documentation for load testing features, usage, configuration, and troubleshooting |
| docs/feat-load-simulator.md | Technical design document detailing implementation, architecture, and technical challenges solved |
| docker-compose.yml | Added load-test profile to datadog-agent service |
| docker-compose.load-test.yml | Docker Compose overlay defining load-simulator service configuration with disabled rate limiting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const WORKLOADS = { | ||
| smoke: { vus: 2, duration: '30s' }, | ||
| load: { | ||
| stages: [ | ||
| { duration: '1m', target: 10 }, | ||
| { duration: '3m', target: 30 }, | ||
| { duration: '5m', target: 30 }, | ||
| { duration: '1m', target: 0 }, | ||
| ], | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The constant WORKLOADS contains a stage configuration that ramps up load over 10 minutes, but the configuration uses inconsistent duration notation. The load workload uses '1m', '3m', '5m' format while the comment on line 295 and documentation describe it as "10 minutes". Consider verifying this totals 10 minutes as expected: 1m + 3m + 5m + 1m = 10m. This appears correct but could benefit from a comment explaining the total duration.
| while (res.status >= 300 && res.status < 400 && res.headers['Location']) { | ||
| let nextUrl = resolveUrl(res.url, res.headers['Location']); | ||
| nextUrl = rewriteUrl(nextUrl); | ||
| res = http.get(nextUrl, { redirects: 0, jar }); | ||
| } |
There was a problem hiding this comment.
The redirect following loops on lines 135-139, 188-192, 287-291, 333-337, and 379-383 don't have a maximum iteration limit. If there's a redirect loop in the application, this could cause the test to hang indefinitely. Consider adding a counter to limit the maximum number of redirects to follow (e.g., 10-20 redirects).
| while (submitRes.status >= 300 && submitRes.status < 400 && submitRes.headers['Location']) { | ||
| let nextUrl = resolveUrl(submitRes.url, submitRes.headers['Location']); | ||
| nextUrl = rewriteUrl(nextUrl); | ||
| submitRes = http.get(nextUrl, { redirects: 0, jar }); | ||
| } |
There was a problem hiding this comment.
This redirect following loop lacks a maximum iteration limit (same issue as in performOAuthLogin). If there's a redirect loop, this could hang indefinitely. Consider adding a counter to limit redirects.
| while (submitRes.status >= 300 && submitRes.status < 400 && submitRes.headers['Location']) { | ||
| let nextUrl = resolveUrl(submitRes.url, submitRes.headers['Location']); | ||
| nextUrl = rewriteUrl(nextUrl); | ||
| submitRes = http.get(nextUrl, { redirects: 0, jar }); |
There was a problem hiding this comment.
This redirect following loop in performRegistration lacks a maximum iteration limit. If there's a redirect loop, this could hang indefinitely. Consider adding a counter to limit redirects.
| while (submitRes.status >= 300 && submitRes.status < 400 && submitRes.headers['Location']) { | |
| let nextUrl = resolveUrl(submitRes.url, submitRes.headers['Location']); | |
| nextUrl = rewriteUrl(nextUrl); | |
| submitRes = http.get(nextUrl, { redirects: 0, jar }); | |
| const MAX_REDIRECTS = 10; | |
| let redirectCount = 0; | |
| while ( | |
| submitRes.status >= 300 && | |
| submitRes.status < 400 && | |
| submitRes.headers['Location'] && | |
| redirectCount < MAX_REDIRECTS | |
| ) { | |
| let nextUrl = resolveUrl(submitRes.url, submitRes.headers['Location']); | |
| nextUrl = rewriteUrl(nextUrl); | |
| submitRes = http.get(nextUrl, { redirects: 0, jar }); | |
| redirectCount++; |
| http.get(`${BASE_URL}/api/stickers/v1/${stickerId}/image`); | ||
| } | ||
| } catch (e) { | ||
| // JSON parse error - skip sticker detail |
There was a problem hiding this comment.
The JSON parsing error on line 456 is caught with an empty catch block. This silently swallows parsing errors which could hide issues with the API response format. Consider logging the error with console.warn or console.error to aid in debugging.
| // JSON parse error - skip sticker detail | |
| // JSON parse error - skip sticker detail | |
| console.warn('publicBrowsingFlow: failed to parse catalogueRes JSON; skipping sticker detail.', e); |
| const queryPart = url.includes('?') ? url.split('?')[1] : ''; | ||
| const fragmentPart = url.includes('#') ? url.split('#')[1] : ''; |
There was a problem hiding this comment.
The URL parsing in extractTokenFromUrl splits on '?' and '#' to extract query and fragment parts, but doesn't handle the case where both might be present (e.g., 'url?query=1#fragment=2'). When splitting by '?', the fragment part will include the query portion. Consider using a more robust approach that properly separates query and fragment.
| const queryPart = url.includes('?') ? url.split('?')[1] : ''; | |
| const fragmentPart = url.includes('#') ? url.split('#')[1] : ''; | |
| let queryPart = ''; | |
| let fragmentPart = ''; | |
| // First, split off the fragment so query parsing is not polluted by it | |
| const hashSplit = url.split('#', 2); | |
| const beforeHash = hashSplit[0]; | |
| const afterHash = hashSplit.length > 1 ? hashSplit[1] : ''; | |
| if (beforeHash && beforeHash.includes('?')) { | |
| queryPart = beforeHash.split('?', 2)[1]; | |
| } | |
| if (afterHash) { | |
| fragmentPart = afterHash; | |
| } |
| } | ||
| } | ||
| } catch (e) { | ||
| // Fall through to error |
There was a problem hiding this comment.
The extractTokenFromUrl function catches all exceptions on line 239 with an empty catch block and falls through to return an error. Consider logging the exception to help with debugging, as this could hide important errors during development and troubleshooting.
| // Fall through to error | |
| console.error('Failed to extract token from URL', url, e); |
| while (res.status >= 300 && res.status < 400 && res.headers['Location']) { | ||
| let nextUrl = resolveUrl(res.url, res.headers['Location']); | ||
| nextUrl = rewriteUrl(nextUrl); | ||
| res = http.get(nextUrl, { redirects: 0, jar }); | ||
| } |
There was a problem hiding this comment.
This redirect following loop in performRegistration lacks a maximum iteration limit. If there's a redirect loop, this could hang indefinitely. Consider adding a counter to limit redirects.
| depends_on: | ||
| traefik: | ||
| condition: service_healthy | ||
| user-management: | ||
| condition: service_healthy | ||
| sticker-catalogue: | ||
| condition: service_healthy | ||
| sticker-award: | ||
| condition: service_healthy | ||
| web-backend: | ||
| condition: service_started | ||
| datadog-agent: | ||
| condition: service_started |
There was a problem hiding this comment.
The Docker Compose override for web-backend in this file is missing the user-management-worker service from the depends_on list in the load-simulator service, but the documentation and inline comments indicate it's a required service. The main docker-compose.yml likely has this service, and it should be included here to ensure proper startup ordering.
| userId = userInfo.user.sub || userInfo.user.id; | ||
| } | ||
| } catch (e) { | ||
| // JSON parse error |
There was a problem hiding this comment.
The JSON parsing error on line 510 is caught with an empty catch block. This silently swallows parsing errors which could hide issues with the user authentication response. Consider adding console.warn or console.error to log these errors for debugging.
| // JSON parse error | |
| // Log JSON parse errors for debugging while keeping behavior unchanged | |
| console.warn('Failed to parse auth user JSON response', { | |
| status: userRes.status, | |
| bodyPreview: String(userRes.body || '').slice(0, 200), | |
| error: String(e), | |
| }); |
Add support for GameDay demo scenarios with: - Multi-user pool via k6 SharedArray (50 users, round-robin assignment) - Three hardcoded GameDay workload profiles: - gameday:auth (50 RPS on login/logout) - gameday:catalogue (100 RPS on sticker API) - gameday:sustained (30+50 RPS across services) - One new mise task: `WORKLOAD=gameday:auth mise run load:gameday` User pool is only used for gameday:* workloads; smoke/load tests continue to use the default test user. Load test traffic appears in APM as normal requests (by design for demos). Co-Authored-By: Claude <noreply@anthropic.com>
- Add provision-users.js script to register test users via IdP - Export registrationFlow for k6 scenarios - Add registration scenario to gameday:sustained (3 users/min) - Reduce auth RPS from 50 to 10 (IdP capacity limit) - Add mise tasks: load:start, load:stop, load:provision-users, load:gameday:run - Update README with GameDay documentation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update README with comprehensive GameDay documentation - Add performLogout helper with proper URL rewriting for Docker - Fix logout failing due to localhost:8080 redirects inside Docker Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| /** | ||
| * Extract access_token from URL query string or fragment | ||
| */ | ||
| function extractTokenFromUrl(url) { |
There was a problem hiding this comment.
This is gross; can't we just use new URL ?
There was a problem hiding this comment.
@scottgerring No, we actually can't. The load testing thing (k6) uses a custom JS runtime.
k6 doesn't have the native URL constructor. k6 runs in its own JavaScript runtime
(goja), not Node.js or a browser, so standard Web APIs like new URL() and
URLSearchParams aren't available.
This is a known k6 limitation - you can verify this in the
https://grafana.com/docs/k6/latest/javascript-api/
There was a problem hiding this comment.
It seems like the k6 recommended way to do it is to use this URL module grafana publishes for k6:
grafana/k6#991
https://github.com/grafana/k6-jslib-url
- Remove separate docker-compose.load-test.yml and merge into main compose with service profiles; disable rate limiting by default in development - Fix rate limiting retry-after logic to only set header when metadata is available (WindowSeconds is for bucketing, not retry timing) - Extract shared URL helpers to load-tests/helpers.js to reduce duplication - Rename gameday terminology to sustained (gameday:auth -> sustained:auth) - Align URL variable naming with web-backend (EXTERNAL_URL -> DEPLOYMENT_HOST_URL) - Add MAX_REDIRECTS constant to prevent infinite redirect loops - Simplify README from 165 to 60 lines - Simplify mise.toml commands to use single compose file with profiles Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| container_name: datadog-agent | ||
| profiles: | ||
| - monitoring | ||
| - load-test |
There was a problem hiding this comment.
Does it make sense to turn agent on if load-testing is on but monitoring isnt?
There was a problem hiding this comment.
@scottgerring Hmm, good question. I guess for what we are using load testing for is to generate data for the agent. Not load testing in the traditional sense. So yeah, I think we should turn load testing on even if monitoring isn't. Which is what this will do right? Or have I misunderstood docker profiles?
There was a problem hiding this comment.
This is an agent - so this'll turn on the datadog-agent, if we ask for load-testing
| # load: Load testing tasks | ||
| # ============================================================================= | ||
|
|
||
| [tasks."load:smoke"] |
There was a problem hiding this comment.
1/ Are these docker-only? If yes, they should probably be prefixed appropriately (e.g. compose:load-test:smoke)
2/ Do we really need all these different variants?
There was a problem hiding this comment.
@scottgerring yep, agreed. I've updated the name to be compose:load-test:smoke and updated the different variants
scottgerring
left a comment
There was a problem hiding this comment.
approved pending build passing from my conflict resolution
Added a first draft of a simple load simulator to get some consistent load running through the service. You can test it out by running
mise run load:smoke. It should startup all services, and then run a simple load test that sends data do Datadog.