Loadtest E2Es and monitor memory for leak detection #611
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request refines GitHub Actions workflows by updating the benchmark job and introducing a new memtest workflow. It adds new testing files and utilities for load and memory testing, including regression analysis to monitor memory trends. Several internal modules have been updated to improve server and container management, and TypeScript configurations and package scripts have been extended to support the new memtest functionality. Changes
Possibly related PRs
Suggested reviewers
Poem
✨ 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
CodeRabbit Configuration File (
|
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Node Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared |
🚀 Snapshot Release (Bun Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared |
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
| @@ -11,7 +11,6 @@ concurrency: | |||
|
|
|||
| env: | |||
| NODE_NO_WARNINGS: 1 | |||
| CI: true | |||
There was a problem hiding this comment.
| @@ -22,7 +21,7 @@ jobs: | |||
| - 10 | |||
| - 100 | |||
| - 1000 | |||
| name: Benchmark / ${{matrix.e2e_runner}} / ${{matrix.products_size}} items | |||
There was a problem hiding this comment.
Looks nicer in the checks overview.
- Bench / Benchmark / node / 10 items
+ Bench / node / 10 items
enisdenjo
changed the title
enisdenjo
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: 10
🔭 Outside diff range comments (1)
internal/perf/src/loadtest.ts (1)
26-103: Potential concurrency considerations.This loadtest functionality is useful, but if you plan to run multiple load tests in parallel with a shared
cwdor server instance, carefully handle concurrency to avoid port conflicts or shared resource issues.
♻️ Duplicate comments (1)
.github/workflows/memtest.yml (1)
33-36: Action Version Pinning AdvisoryThe workflow uses
the-guild-org/shared-config/setup@v1for environment setup. For enhanced reproducibility and security, consider pinning this action to a specific commit hash rather than using a mutable version alias.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
.github/workflows/bench.yml(1 hunks).github/workflows/memtest.yml(1 hunks)e2e/federation-example/federation-example.memtest.ts(1 hunks)internal/e2e/src/tenv.ts(3 hunks)internal/examples/src/convert.ts(1 hunks)internal/perf/package.json(1 hunks)internal/perf/src/index.ts(1 hunks)internal/perf/src/loadtest-script.ts(1 hunks)internal/perf/src/loadtest.ts(1 hunks)internal/perf/src/memtest.ts(1 hunks)internal/proc/src/index.ts(3 hunks)internal/testing/src/env.ts(1 hunks)package.json(1 hunks)tsconfig.json(1 hunks)vitest.projects.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
internal/perf/src/index.tsinternal/testing/src/env.tstsconfig.jsoninternal/perf/package.jsonvitest.projects.tse2e/federation-example/federation-example.memtest.tspackage.jsoninternal/perf/src/loadtest-script.tsinternal/examples/src/convert.tsinternal/perf/src/memtest.tsinternal/e2e/src/tenv.tsinternal/proc/src/index.tsinternal/perf/src/loadtest.ts
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/federation-example/federation-example.memtest.ts
🪛 Biome (1.9.4)
internal/perf/src/loadtest.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
⏰ Context from checks skipped due to timeout of 90000ms (31)
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: Unit / Bun
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: Unit / Node v23
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: Unit / Node v22
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: Unit / Node v20
- GitHub Check: Snapshot / snapshot
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Build
- GitHub Check: Leaks / Node v20
- GitHub Check: bun / 1000 items
- GitHub Check: Unit / Node v18
- GitHub Check: bun / 100 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: bun / 10 items
- GitHub Check: node / 1000 items
- GitHub Check: Leaks / Node v18
- GitHub Check: bun
- GitHub Check: Bundle
- GitHub Check: node / 100 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: node
- GitHub Check: node / 10 items
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
🔇 Additional comments (21)
internal/proc/src/index.ts (2)
11-12: LGTM! Good addition for process lifecycle management.The
waitForExitproperty provides a clean way to track process termination.
22-25: LGTM! Well-structured server interface.The
Serverinterface provides a clean abstraction for server-specific processes, which is essential for load testing.internal/e2e/src/tenv.ts (2)
709-714: LGTM! Clean implementation of the Server interface.The service object correctly implements the
Serverinterface with all required properties.
873-873: LGTM! Good use of Docker's wait() method.The
waitForExitproperty is correctly implemented using the container's nativewait()method.internal/perf/src/memtest.ts (2)
6-21: Interface design looks solid!The
MemtestOptionsinterface is clearly defined, with sensible default values and documentation.
56-72: Verify zero slope handling.Throwing an error when the slope is zero might be too strict if a truly stable (flat) memory usage is a valid outcome. Confirm the desired behavior for a perfectly flat slope scenario.
internal/perf/src/loadtest.ts (1)
6-24: Clean interface definition.The
LoadtestOptionsinterface is well-structured, providing clear defaults and documentation.e2e/federation-example/federation-example.memtest.ts (1)
1-8: Federation setup looks good!Establishing the
gatewayandsupergraphwithin this test is logical and easy to follow.internal/perf/src/loadtest-script.ts (1)
1-3: LGTM! Imports are appropriate for k6 load testing.The imports from k6 are correctly specified for load testing requirements.
internal/testing/src/env.ts (1)
3-10: LGTM! Documentation is clear and includes examples.The JSDoc comments effectively explain the function's purpose and provide a helpful example.
internal/examples/src/convert.ts (2)
183-184: LGTM! Appropriate exclusion of memtest files.The addition of
.memtest.to the exclusion list is correct and consistent with the handling of other test files.
1-1: Documentation update may be required.The PR implements memory leak detection functionality, but no corresponding documentation PR was found in the
graphql-hive/consolerepository. Consider creating a follow-up issue to add documentation for this feature.✅ Verification successful
🌐 Web query:
Search for documentation PRs in graphql-hive/console repository related to memory leak detection or load testing💡 Result:
Based on the search results provided, there are no documentation PRs (Pull Requests) mentioned in the graphql-hive/console repository related to memory leak detection or load testing. Here's what we found:
graphql-hive/console issues ([7]):
- Open issues include security vulnerability alerts, schema validation improvements, and GitHub integration requests.
- No issues explicitly mention memory leaks or load testing documentation.
Other relevant results:
- Memory leak detection guidance exists for ProxySQL ([1]) and Java applications ([4]), but these are unrelated to GraphQL Hive.
- Load testing advice appears for Apollo Federation ([8]), but this isn’t part of Hive’s documentation.
For direct insights into Hive’s memory management or load-testing features:
- Review Hive’s official development docs ([10]) for performance-related configurations.
- Check Hive’s GitHub repository for newer PRs beyond the indexed search results.
Citations:
- 1: https://proxysql.com/documentation/memory-leak-detection/
- 2: https://the-guild.dev/graphql/hive/docs/api-reference/cli
- 3: https://huggingface.co/datasets/h1alexbel/github-samples/commit/3b130bfbfb7e0a88925fd8d05559e51547be910e.diff?file=input.csv
- 4: https://www.dynatrace.com/news/blog/how-to-identify-a-java-memory-leak/
- 5: https://graphql.org/community/tools-and-libraries/
- 6: https://toppodcast.com/podcast_feeds/pauls-security-weekly-video/
- 7: https://github.com/graphql-hive/console/issues
- 8: https://www.apollographql.com/docs/graphos/platform/production-readiness/load-testing
- 9: https://community.apollographql.com/t/client-core-developers-possible-memory-leak-in-apolloclient-querymanager-despite-cache-policy-set-to-no-cache/6390
- 10: https://github.com/graphql-hive/console/blob/main/docs/DEVELOPMENT.md
Documentation update required
The web search confirms that there are no documentation PRs in the
graphql-hive/consolerepository related to memory leak detection or load testing. Since the PR implements memory leak detection functionality without corresponding documentation, please consider creating a follow-up issue to add the necessary documentation.
- Memory leak detection functionality introduced in the PR is undocumented.
- No related documentation PR was found in
graphql-hive/console.internal/perf/src/index.ts (1)
1-1: Re-export Loadtest ModuleThe new export statement cleanly re-exports all public members from the
loadtestmodule. This facilitates easier imports for consumers of the internal/perf module—an appropriate design choice for consolidating related testing utilities..github/workflows/bench.yml (3)
12-14: Global Environment Variable CleanupThe removal of the
CIenvironment variable from the global settings is appropriate since GitHub Actions setsCI=trueautomatically. This streamlines the configuration.
24-24: Enhanced Job NamingThe updated job name now dynamically reflects the matrix parameters (
${{matrix.e2e_runner}} / ${{matrix.products_size}} items), providing clear context in the workflow logs.
33-37: Conditional Command Execution in Benchmark StepUsing an inline conditional (
${{matrix.e2e_runner == 'bun' && 'yarn bun' || 'yarn'}}) to choose the appropriate command based on the runner is a neat tactic. Ensure that both commands are tested in your CI environment..github/workflows/memtest.yml (2)
1-8: New Memtest Workflow IntroductionThe new workflow named Memtest is set up correctly to trigger on pushes and pull requests to
main. Its concurrency settings will help avoid overlapping runs. This workflow lays the foundation for systematic memory leak detection.
24-31: Robust k6 Installation StepThe k6 installation step downloads and extracts the specified version (
v0.56.0) to a local bin directory and correctly appends its path to$GITHUB_PATH. This ensures that the k6 tool is available for the memory testing step.tsconfig.json (2)
17-18: Updated Path Mappings for Internal PerfThe new path mappings for
@internal/perfand@internal/perf/memtestcorrectly point to their respective source files. This update streamlines module resolution for the new performance and memory testing functionality. Please verify these mappings work as expected across all development and CI environments.
1-87: Overall TSConfig ConsistencyBeyond the new mappings, the TSConfig maintains strict type-checking and modern module settings. Confirm that these additions integrate well with your existing path aliases and that all newly added modules are appropriately referenced in your codebase.
package.json (1)
26-26: Review of the Existing Leak Detection Script ("test:leaks")The existing
"test:leaks"script is configured properly using Jest with the--detectOpenHandlesand--detectLeaksflags, ensuring that any resource leakage during tests is caught. This aligns well with the leak detection objectives of the PR.
internal/perf/src/memtest.ts
Outdated
| export function memtest(opts: MemtestOptions, setup: () => Promise<Server>) { | ||
| const { memoryThresholdInMB = 5, duration = 60_000, ...loadtestOpts } = opts; | ||
| it( | ||
| 'should not have a memory increase trend', | ||
| { | ||
| timeout: duration + 10_000, // allow 10s for the test teardown | ||
| }, | ||
| async ({ expect }) => { | ||
| const server = await setup(); | ||
|
|
||
| const { memoryInMBSnapshots } = await loadtest({ | ||
| ...loadtestOpts, | ||
| duration, | ||
| server, | ||
| }); | ||
|
|
||
| const slope = calculateRegressionSlope(memoryInMBSnapshots); | ||
|
|
||
| expect( | ||
| slope, | ||
| `Memory increase trend detected with slope of ${slope}MB (exceding threshold of ${memoryThresholdInMB}MB)`, | ||
| ).toBeLessThan(memoryThresholdInMB); | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider ensuring the server is properly shut down after the test completes.
Currently, there's no explicit teardown step to stop the server once the memory test finishes. Leaving an ephemeral server running can consume resources unnecessarily, especially for local development or CI pipelines.
it(
'should not have a memory increase trend',
{
timeout: duration + 10_000, // allow 10s for the test teardown
},
async ({ expect }) => {
const server = await setup();
try {
const { memoryInMBSnapshots } = await loadtest({
...loadtestOpts,
duration,
server,
});
const slope = calculateRegressionSlope(memoryInMBSnapshots);
expect(
slope,
`Memory increase trend detected with slope of ${slope}MB (exceding threshold of ${memoryThresholdInMB}MB)`,
).toBeLessThan(memoryThresholdInMB);
} finally {
+ await server.stop?.();
}
},
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function memtest(opts: MemtestOptions, setup: () => Promise<Server>) { | |
| const { memoryThresholdInMB = 5, duration = 60_000, ...loadtestOpts } = opts; | |
| it( | |
| 'should not have a memory increase trend', | |
| { | |
| timeout: duration + 10_000, // allow 10s for the test teardown | |
| }, | |
| async ({ expect }) => { | |
| const server = await setup(); | |
| const { memoryInMBSnapshots } = await loadtest({ | |
| ...loadtestOpts, | |
| duration, | |
| server, | |
| }); | |
| const slope = calculateRegressionSlope(memoryInMBSnapshots); | |
| expect( | |
| slope, | |
| `Memory increase trend detected with slope of ${slope}MB (exceding threshold of ${memoryThresholdInMB}MB)`, | |
| ).toBeLessThan(memoryThresholdInMB); | |
| }, | |
| ); | |
| } | |
| export function memtest(opts: MemtestOptions, setup: () => Promise<Server>) { | |
| const { memoryThresholdInMB = 5, duration = 60_000, ...loadtestOpts } = opts; | |
| it( | |
| 'should not have a memory increase trend', | |
| { | |
| timeout: duration + 10_000, // allow 10s for the test teardown | |
| }, | |
| async ({ expect }) => { | |
| const server = await setup(); | |
| try { | |
| const { memoryInMBSnapshots } = await loadtest({ | |
| ...loadtestOpts, | |
| duration, | |
| server, | |
| }); | |
| const slope = calculateRegressionSlope(memoryInMBSnapshots); | |
| expect( | |
| slope, | |
| `Memory increase trend detected with slope of ${slope}MB (exceding threshold of ${memoryThresholdInMB}MB)`, | |
| ).toBeLessThan(memoryThresholdInMB); | |
| } finally { | |
| await server.stop?.(); | |
| } | |
| }, | |
| ); | |
| } |
| import path from 'path'; | ||
| import { setTimeout } from 'timers/promises'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adopt Node.js import protocol for built-in modules.
According to static analysis hints, use node: prefix for built-in modules to be more explicit.
-import path from 'path';
-import { setTimeout } from 'timers/promises';
+import path from 'node:path';
+import { setTimeout } from 'node:timers/promises';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import path from 'path'; | |
| import { setTimeout } from 'timers/promises'; | |
| import path from 'node:path'; | |
| import { setTimeout } from 'node:timers/promises'; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
| memtest( | ||
| { | ||
| cwd, | ||
| query, | ||
| }, | ||
| async () => | ||
| gateway({ | ||
| supergraph: await supergraph(), | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add idle memory verification or documentation.
Your PR objectives mention an idle memory check after load testing and possible docs in console. Consider incorporating a post-loadtest idle check or opening a follow-up issue to add relevant documentation in graphql-hive/console.
| const url = __ENV['URL']; | ||
| if (!url) { | ||
| return test.abort('Environment variable "URL" not provided'); | ||
| } | ||
|
|
||
| const query = __ENV['QUERY']; | ||
| if (!query) { | ||
| return test.abort('Environment variable "QUERY" not provided'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding type validation for environment variables.
While the existence check is good, consider validating the URL format and query structure.
const url = __ENV['URL'];
if (!url) {
return test.abort('Environment variable "URL" not provided');
}
+try {
+ new URL(url); // Validate URL format
+} catch {
+ return test.abort('Environment variable "URL" is not a valid URL');
+}
const query = __ENV['QUERY'];
if (!query) {
return test.abort('Environment variable "QUERY" not provided');
}
+if (!query.trim().startsWith('{') || !query.trim().endsWith('}')) {
+ return test.abort('Environment variable "QUERY" is not a valid GraphQL query');
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const url = __ENV['URL']; | |
| if (!url) { | |
| return test.abort('Environment variable "URL" not provided'); | |
| } | |
| const query = __ENV['QUERY']; | |
| if (!query) { | |
| return test.abort('Environment variable "QUERY" not provided'); | |
| } | |
| const url = __ENV['URL']; | |
| if (!url) { | |
| return test.abort('Environment variable "URL" not provided'); | |
| } | |
| try { | |
| new URL(url); // Validate URL format | |
| } catch { | |
| return test.abort('Environment variable "URL" is not a valid URL'); | |
| } | |
| const query = __ENV['QUERY']; | |
| if (!query) { | |
| return test.abort('Environment variable "QUERY" not provided'); | |
| } | |
| if (!query.trim().startsWith('{') || !query.trim().endsWith('}')) { | |
| return test.abort('Environment variable "QUERY" is not a valid GraphQL query'); | |
| } |
| const res = http.post(url, { query }); | ||
|
|
||
| check(res, { | ||
| 'status is 200': (res) => res.status === 200, | ||
| 'body contains data': (res) => !!res.body?.toString().includes('"data":{'), | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider enhancing response validation.
The current response validation is basic. Consider adding more comprehensive checks:
const res = http.post(url, { query });
check(res, {
'status is 200': (res) => res.status === 200,
- 'body contains data': (res) => !!res.body?.toString().includes('"data":{'),
+ 'body contains data': (res) => {
+ const body = res.body?.toString();
+ return body && JSON.parse(body).data !== undefined;
+ },
+ 'response time under 2s': (res) => res.timings.duration < 2000,
+ 'no errors in response': (res) => {
+ const body = res.body?.toString();
+ return body && !JSON.parse(body).errors;
+ },
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const res = http.post(url, { query }); | |
| check(res, { | |
| 'status is 200': (res) => res.status === 200, | |
| 'body contains data': (res) => !!res.body?.toString().includes('"data":{'), | |
| }); | |
| const res = http.post(url, { query }); | |
| check(res, { | |
| 'status is 200': (res) => res.status === 200, | |
| 'body contains data': (res) => { | |
| const body = res.body?.toString(); | |
| return body && JSON.parse(body).data !== undefined; | |
| }, | |
| 'response time under 2s': (res) => res.timings.duration < 2000, | |
| 'no errors in response': (res) => { | |
| const body = res.body?.toString(); | |
| return body && !JSON.parse(body).errors; | |
| }, | |
| }); |
| { | ||
| extends: './vitest.config.ts', | ||
| test: { | ||
| name: 'memtest', | ||
| include: ['**/*.memtest.ts'], | ||
| hookTimeout: testTimeout, | ||
| testTimeout, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! The memtest configuration is properly structured.
The new memtest configuration correctly extends the base configuration and maintains consistency with other test suites by using the same timeout settings.
However, consider documenting the purpose of this test suite:
{
extends: './vitest.config.ts',
test: {
+ // Suite for memory leak detection tests
name: 'memtest',
include: ['**/*.memtest.ts'],
hookTimeout: testTimeout,
testTimeout,
},
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| extends: './vitest.config.ts', | |
| test: { | |
| name: 'memtest', | |
| include: ['**/*.memtest.ts'], | |
| hookTimeout: testTimeout, | |
| testTimeout, | |
| }, | |
| }, | |
| { | |
| extends: './vitest.config.ts', | |
| test: { | |
| // Suite for memory leak detection tests | |
| name: 'memtest', | |
| include: ['**/*.memtest.ts'], | |
| hookTimeout: testTimeout, | |
| testTimeout, | |
| }, | |
| }, |
internal/testing/src/env.ts
Outdated
| export function isDebug(module?: string) { | ||
| if (module) { | ||
| return boolEnv(`DEBUG_${module}`); | ||
| } | ||
| return boolEnv('DEBUG'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding input validation for the module parameter.
The function could benefit from validating the module name format.
export function isDebug(module?: string) {
+ if (module && !/^[a-z0-9-_]+$/i.test(module)) {
+ throw new Error('Module name must contain only alphanumeric characters, hyphens, and underscores');
+ }
if (module) {
return boolEnv(`DEBUG_${module}`);
}
return boolEnv('DEBUG');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function isDebug(module?: string) { | |
| if (module) { | |
| return boolEnv(`DEBUG_${module}`); | |
| } | |
| return boolEnv('DEBUG'); | |
| } | |
| export function isDebug(module?: string) { | |
| if (module && !/^[a-z0-9-_]+$/i.test(module)) { | |
| throw new Error('Module name must contain only alphanumeric characters, hyphens, and underscores'); | |
| } | |
| if (module) { | |
| return boolEnv(`DEBUG_${module}`); | |
| } | |
| return boolEnv('DEBUG'); | |
| } |
internal/perf/package.json
Outdated
| { | ||
| "name": "@internal/perf", | ||
| "type": "module", | ||
| "private": true, | ||
| "main": "./src/index.ts", | ||
| "dependencies": { | ||
| "parse-duration": "^2.0.0", | ||
| "regression": "^2.0.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/k6": "^0.54.2", | ||
| "@types/regression": "^2" | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Module Configuration & Dependency Integration
The configuration for the new @internal/perf module is well-structured:
- The module is correctly marked as an ES module and set to private.
- The
"main"entry point points to./src/index.ts, ensuring proper module resolution. - Dependencies (
parse-duration,regression) and development dependencies (type definitions) are versioned appropriately.
Additional Note: The enriched summary indicated a new script "test:mem": "vitest --project memtest". Please confirm this script is part of your final package configuration. Also, remember to update documentation (e.g., in the console project) describing the memory testing functionality introduced here.
.github/workflows/memtest.yml
Outdated
| - name: Test | ||
| run: yarn test:mem | ||
| env: | ||
| E2E_GATEWAY_RUNNER: ${{matrix.e2e_runner}} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Memory Test Execution
The final step executing yarn test:mem correctly triggers the memory test suite. Ensure that the corresponding script in package.json and the configuration in vitest.projects.ts align with this command.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
internal/perf/src/memtest.ts (1)
77-77: Document the new memtest feature in the console project.This PR references "GW-154," indicating a linked Linear issue. However, there's no mention of corresponding documentation in the
graphql-hive/consolerepository. Consider creating a follow-up issue in the console project to document the new memtest functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
e2e/opentelemetry/opentelemetry.memtest.ts(1 hunks)internal/perf/src/memtest.ts(1 hunks)internal/testing/src/env.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
internal/testing/src/env.tse2e/opentelemetry/opentelemetry.memtest.tsinternal/perf/src/memtest.ts
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/opentelemetry/opentelemetry.memtest.ts
⏰ Context from checks skipped due to timeout of 90000ms (31)
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Unit / Bun
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Unit / Node v23
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Unit / Node v22
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: bun / 1000 items
- GitHub Check: Snapshot / snapshot
- GitHub Check: Unit / Node v20
- GitHub Check: bun / 100 items
- GitHub Check: Missing peer deps
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: bun / 10 items
- GitHub Check: Leaks / Node v20
- GitHub Check: node / 1000 items
- GitHub Check: Bundle
- GitHub Check: node / 100 items
- GitHub Check: Unit / Node v18
- GitHub Check: node / 10 items
- GitHub Check: bun
- GitHub Check: Leaks / Node v18
- GitHub Check: Build
- GitHub Check: node
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
internal/perf/src/memtest.ts (2)
32-49: Ensure the server is properly shut down after the test completes.This code starts the server but never calls
server.stop(). Leaving the server running can needlessly consume resources in CI or local development.it( 'should not have a memory increase trend', { timeout: duration + 10_000, // allow 10s for the test teardown }, async ({ expect }) => { const server = await setup(); const { memoryInMBSnapshots } = await loadtest({ ...loadtestOpts, duration, server, }); + // Teardown + await server.stop?.(); const slope = calculateRegressionSlope(memoryInMBSnapshots); expect( slope, `Memory increase trend detected with slope of ${slope}MB (exceding threshold of ${memoryThresholdInMB}MB)`, ).toBeLessThan(memoryThresholdInMB); }, );
60-73: Confirm whether a zero slope should truly trigger an error.Returning an error for a zero slope implies no memory increase is unacceptable. Verify whether negative or zero slope scenarios should instead pass successfully.
internal/perf/src/memtest.ts
Outdated
|
|
||
| expect( | ||
| slope, | ||
| `Memory increase trend detected with slope of ${slope}MB (exceding threshold of ${memoryThresholdInMB}MB)`, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix the typographical error in the error message.
Use "exceeding" instead of "exceding".
- `Memory increase trend detected with slope of ${slope}MB (exceding threshold of ${memoryThresholdInMB}MB)`,
+ `Memory increase trend detected with slope of ${slope}MB (exceeding threshold of ${memoryThresholdInMB}MB)`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `Memory increase trend detected with slope of ${slope}MB (exceding threshold of ${memoryThresholdInMB}MB)`, | |
| `Memory increase trend detected with slope of ${slope}MB (exceeding threshold of ${memoryThresholdInMB}MB)`, |
| (['grpc', 'http'] as const).forEach((OTLP_EXPORTER_TYPE) => { | ||
| describe(`OpenTelemetry ${OTLP_EXPORTER_TYPE} exporter`, () => { | ||
| let jaeger: Container; | ||
| beforeAll(async () => { | ||
| jaeger = await container({ | ||
| name: `jaeger-${OTLP_EXPORTER_TYPE}`, | ||
| image: 'jaegertracing/all-in-one:1.56', | ||
| env: { | ||
| COLLECTOR_OTLP_ENABLED: 'true', | ||
| }, | ||
| containerPort: 4318, | ||
| additionalContainerPorts: [16686, 4317], | ||
| healthcheck: ['CMD-SHELL', 'wget --spider http://0.0.0.0:14269'], | ||
| }); | ||
| }); | ||
| const jaegerUrls = { | ||
| get http() { | ||
| return `http://0.0.0.0:${jaeger.port}/v1/traces`; | ||
| }, | ||
| get grpc() { | ||
| return `http://0.0.0.0:${jaeger.additionalPorts[4317]}`; | ||
| }, | ||
| }; | ||
|
|
||
| memtest( | ||
| { | ||
| cwd, | ||
| query, | ||
| }, | ||
| async () => | ||
| gateway({ | ||
| supergraph: await supergraph(), | ||
| env: { | ||
| OTLP_EXPORTER_TYPE, | ||
| OTLP_EXPORTER_URL: jaegerUrls[OTLP_EXPORTER_TYPE], | ||
| OTLP_SERVICE_NAME: `memtest-${OTLP_EXPORTER_TYPE}`, | ||
| }, | ||
| }), | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding a container teardown step.
The container is launched but never explicitly stopped. This can introduce resource leaks, especially for CI runs or local environments.
describe(`OpenTelemetry ${OTLP_EXPORTER_TYPE} exporter`, () => {
let jaeger: Container;
beforeAll(async () => {
jaeger = await container({
// ...
});
});
+ afterAll(async () => {
+ await jaeger.stop?.();
+ });
memtest(
{
cwd,
query,
},
async () =>
gateway({
// ...
}),
);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (['grpc', 'http'] as const).forEach((OTLP_EXPORTER_TYPE) => { | |
| describe(`OpenTelemetry ${OTLP_EXPORTER_TYPE} exporter`, () => { | |
| let jaeger: Container; | |
| beforeAll(async () => { | |
| jaeger = await container({ | |
| name: `jaeger-${OTLP_EXPORTER_TYPE}`, | |
| image: 'jaegertracing/all-in-one:1.56', | |
| env: { | |
| COLLECTOR_OTLP_ENABLED: 'true', | |
| }, | |
| containerPort: 4318, | |
| additionalContainerPorts: [16686, 4317], | |
| healthcheck: ['CMD-SHELL', 'wget --spider http://0.0.0.0:14269'], | |
| }); | |
| }); | |
| const jaegerUrls = { | |
| get http() { | |
| return `http://0.0.0.0:${jaeger.port}/v1/traces`; | |
| }, | |
| get grpc() { | |
| return `http://0.0.0.0:${jaeger.additionalPorts[4317]}`; | |
| }, | |
| }; | |
| memtest( | |
| { | |
| cwd, | |
| query, | |
| }, | |
| async () => | |
| gateway({ | |
| supergraph: await supergraph(), | |
| env: { | |
| OTLP_EXPORTER_TYPE, | |
| OTLP_EXPORTER_URL: jaegerUrls[OTLP_EXPORTER_TYPE], | |
| OTLP_SERVICE_NAME: `memtest-${OTLP_EXPORTER_TYPE}`, | |
| }, | |
| }), | |
| ); | |
| }); | |
| }); | |
| (['grpc', 'http'] as const).forEach((OTLP_EXPORTER_TYPE) => { | |
| describe(`OpenTelemetry ${OTLP_EXPORTER_TYPE} exporter`, () => { | |
| let jaeger: Container; | |
| beforeAll(async () => { | |
| jaeger = await container({ | |
| name: `jaeger-${OTLP_EXPORTER_TYPE}`, | |
| image: 'jaegertracing/all-in-one:1.56', | |
| env: { | |
| COLLECTOR_OTLP_ENABLED: 'true', | |
| }, | |
| containerPort: 4318, | |
| additionalContainerPorts: [16686, 4317], | |
| healthcheck: ['CMD-SHELL', 'wget --spider http://0.0.0.0:14269'], | |
| }); | |
| }); | |
| afterAll(async () => { | |
| await jaeger.stop?.(); | |
| }); | |
| const jaegerUrls = { | |
| get http() { | |
| return `http://0.0.0.0:${jaeger.port}/v1/traces`; | |
| }, | |
| get grpc() { | |
| return `http://0.0.0.0:${jaeger.additionalPorts[4317]}`; | |
| }, | |
| }; | |
| memtest( | |
| { | |
| cwd, | |
| query, | |
| }, | |
| async () => | |
| gateway({ | |
| supergraph: await supergraph(), | |
| env: { | |
| OTLP_EXPORTER_TYPE, | |
| OTLP_EXPORTER_URL: jaegerUrls[OTLP_EXPORTER_TYPE], | |
| OTLP_SERVICE_NAME: `memtest-${OTLP_EXPORTER_TYPE}`, | |
| }, | |
| }), | |
| ); | |
| }); | |
| }); |
| * be activated for the specific module in code. | ||
| * | ||
| * For example, `isDebug('loadtest')` will only activate when the | ||
| * variable `loadtest` is listed in the `DEBUG` enviorment variable. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix the typo in the JSDoc.
Use "environment" instead of "enviorment".
- * variable `loadtest` is listed in the `DEBUG` enviorment variable.
+ * variable `loadtest` is listed in the `DEBUG` environment variable.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * variable `loadtest` is listed in the `DEBUG` enviorment variable. | |
| * variable `loadtest` is listed in the `DEBUG` environment variable. |
| export function isDebug(module?: string) { | ||
| if (module) { | ||
| const modules = String(process.env['DEBUG']) | ||
| .split(',') | ||
| .map((m) => m.trim()); | ||
| return modules.includes(module); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Handle unset or empty DEBUG environment variable more robustly.
When process.env['DEBUG'] is not defined or empty, String(process.env['DEBUG']) becomes "undefined" or "". Consider a fallback to an empty array to avoid unintended matches.
export function isDebug(module?: string) {
if (module) {
- const modules = String(process.env['DEBUG'])
+ const debugStr = process.env['DEBUG'] ?? '';
+ if (!debugStr) return false;
+ const modules = debugStr
.split(',')
.map((m) => m.trim());
return modules.includes(module);
}
return boolEnv('DEBUG');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function isDebug(module?: string) { | |
| if (module) { | |
| const modules = String(process.env['DEBUG']) | |
| .split(',') | |
| .map((m) => m.trim()); | |
| return modules.includes(module); | |
| } | |
| export function isDebug(module?: string) { | |
| if (module) { | |
| const debugStr = process.env['DEBUG'] ?? ''; | |
| if (!debugStr) return false; | |
| const modules = debugStr | |
| .split(',') | |
| .map((m) => m.trim()); | |
| return modules.includes(module); | |
| } | |
| return boolEnv('DEBUG'); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
internal/perf/package.json(1 hunks)internal/perf/src/chart.ts(1 hunks)internal/perf/src/loadtest.ts(1 hunks)internal/perf/src/memtest.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
internal/perf/package.jsoninternal/perf/src/loadtest.tsinternal/perf/src/chart.tsinternal/perf/src/memtest.ts
🪛 Biome (1.9.4)
internal/perf/src/loadtest.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 3-3: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
⏰ Context from checks skipped due to timeout of 90000ms (30)
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: Leaks / Node v20
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: Leaks / Node v18
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Snapshot / snapshot
- GitHub Check: bun / 1000 items
- GitHub Check: Unit / Node v23
- GitHub Check: bun / 100 items
- GitHub Check: Unit / Node v22
- GitHub Check: bun / 10 items
- GitHub Check: Unit / Bun
- GitHub Check: node / 1000 items
- GitHub Check: Unit / Node v20
- GitHub Check: node / 100 items
- GitHub Check: Unit / Node v18
- GitHub Check: Bundle
- GitHub Check: bun
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: node / 10 items
- GitHub Check: Build
- GitHub Check: node
🔇 Additional comments (4)
internal/perf/src/memtest.ts (1)
36-98: Ensure the server is properly shut down after the test completes.This code doesn't explicitly handle a teardown step to stop the server once the memory test finishes. Leaving the server running might consume resources unnecessarily, especially in local or CI environments.
internal/perf/package.json (3)
1-5: Module Configuration and MetadataThe module is correctly set up as an ES module and marked as private. The main entry point (
"./src/index.ts") is defined appropriately, ensuring proper module resolution.
6-12: Dependencies SetupThe dependencies—including
canvas,chart.js,chartjs-plugin-trendline,parse-duration, andregression—are declared with clear version constraints. These libraries appear well-chosen for performance and data analysis tasks. Just ensure they have been validated against your overall performance and memory testing requirements.
13-16: Development DependenciesThe development dependencies for
@types/k6and@types/regressionare appropriately specified for improved type safety and tooling support. This aligns with the module’s focus on performance and memory testing.
| loadtestSlopeThreshold?: number; | ||
| } | ||
|
|
||
| export function memtest(opts: MemtestOptions, setup: () => Promise<Server>) { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add documentation in the console project for this new functionality.
This function introduces a new memory testing feature. Consider creating or updating documentation in the graphql-hive/console repository to inform users about the usage and configuration of memtest.
Do you want me to open a follow-up issue in the graphql-hive/console repository to ensure the feature is well documented?
internal/perf/src/chart.ts
Outdated
| const canvas = createCanvas(800, 400, 'svg'); | ||
|
|
||
| const chartConfig: ChartConfiguration = { | ||
| type: 'line', | ||
| data: { | ||
| labels, | ||
| datasets: datasets.map(({ label, data }) => ({ | ||
| label, | ||
| data, | ||
| trendlineLinear: { | ||
| width: 1, | ||
| lineStyle: 'dashed', | ||
| }, | ||
| })), | ||
| }, | ||
| options: { | ||
| responsive: false, // because we're rendering the chart statically | ||
| scales: { | ||
| y: { | ||
| ticks: { | ||
| callback: options.yTicksCallback, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| plugins: [ | ||
| chartTrendline, | ||
| { | ||
| id: 'set-white-background', | ||
| beforeDraw: (chart) => { | ||
| chart.ctx.fillStyle = 'white'; | ||
| chart.ctx.fillRect(0, 0, chart.width, chart.height); | ||
| chart.ctx.restore(); | ||
| }, | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| new Chart( | ||
| // @ts-expect-error canvas types are of a different instance, but they fit | ||
| canvas.getContext('2d'), | ||
| chartConfig, | ||
| ); | ||
|
|
||
| return canvas; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Allow optional chart dimensions via parameters.
Consider introducing parameters for the chart's width and height (currently hardcoded to 800×400), allowing for flexibility in various test cases or custom outputs.
-export function createLineChart(
- labels: (number | string | null)[],
- datasets: LineChartDataset[],
- options: LineChartOptions = {},
-): Canvas {
- const canvas = createCanvas(800, 400, 'svg');
+export function createLineChart(
+ labels: (number | string | null)[],
+ datasets: LineChartDataset[],
+ options: LineChartOptions = {},
+ canvasWidth = 800,
+ canvasHeight = 400
+): Canvas {
+ const canvas = createCanvas(canvasWidth, canvasHeight, 'svg');
...
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const canvas = createCanvas(800, 400, 'svg'); | |
| const chartConfig: ChartConfiguration = { | |
| type: 'line', | |
| data: { | |
| labels, | |
| datasets: datasets.map(({ label, data }) => ({ | |
| label, | |
| data, | |
| trendlineLinear: { | |
| width: 1, | |
| lineStyle: 'dashed', | |
| }, | |
| })), | |
| }, | |
| options: { | |
| responsive: false, // because we're rendering the chart statically | |
| scales: { | |
| y: { | |
| ticks: { | |
| callback: options.yTicksCallback, | |
| }, | |
| }, | |
| }, | |
| }, | |
| plugins: [ | |
| chartTrendline, | |
| { | |
| id: 'set-white-background', | |
| beforeDraw: (chart) => { | |
| chart.ctx.fillStyle = 'white'; | |
| chart.ctx.fillRect(0, 0, chart.width, chart.height); | |
| chart.ctx.restore(); | |
| }, | |
| }, | |
| ], | |
| }; | |
| new Chart( | |
| // @ts-expect-error canvas types are of a different instance, but they fit | |
| canvas.getContext('2d'), | |
| chartConfig, | |
| ); | |
| return canvas; | |
| } | |
| export function createLineChart( | |
| labels: (number | string | null)[], | |
| datasets: LineChartDataset[], | |
| options: LineChartOptions = {}, | |
| canvasWidth = 800, | |
| canvasHeight = 400 | |
| ): Canvas { | |
| const canvas = createCanvas(canvasWidth, canvasHeight, 'svg'); | |
| const chartConfig: ChartConfiguration = { | |
| type: 'line', | |
| data: { | |
| labels, | |
| datasets: datasets.map(({ label, data }) => ({ | |
| label, | |
| data, | |
| trendlineLinear: { | |
| width: 1, | |
| lineStyle: 'dashed', | |
| }, | |
| })), | |
| }, | |
| options: { | |
| responsive: false, // because we're rendering the chart statically | |
| scales: { | |
| y: { | |
| ticks: { | |
| callback: options.yTicksCallback, | |
| }, | |
| }, | |
| }, | |
| }, | |
| plugins: [ | |
| chartTrendline, | |
| { | |
| id: 'set-white-background', | |
| beforeDraw: (chart) => { | |
| chart.ctx.fillStyle = 'white'; | |
| chart.ctx.fillRect(0, 0, chart.width, chart.height); | |
| chart.ctx.restore(); | |
| }, | |
| }, | |
| ], | |
| }; | |
| new Chart( | |
| // @ts-expect-error canvas types are of a different instance, but they fit | |
| canvas.getContext('2d'), | |
| chartConfig, | |
| ); | |
| return canvas; | |
| } |
internal/perf/src/loadtest.ts
Outdated
| debugLog(`Loadtesting...`); | ||
| state = 'loadtest'; | ||
| const [, waitForExit] = await spawn( | ||
| { | ||
| cwd, | ||
| ...procOptions, | ||
| signal, | ||
| }, | ||
| 'k6', | ||
| 'run', | ||
| `--vus=${vus}`, | ||
| `--duration=${duration}ms`, | ||
| `--env=URL=${server.protocol}://localhost:${server.port}/graphql`, | ||
| `--env=QUERY=${query}`, | ||
| path.join(__dirname, 'loadtest-script.ts'), | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider clarifying behavior if k6 isn’t installed.
If k6 is not available on the environment’s PATH, this call fails with a cryptic error. A more user-friendly error or a pre-flight check might help.
internal/perf/src/loadtest.ts
Outdated
| import fs from 'fs/promises'; | ||
| import path from 'path'; | ||
| import { setTimeout } from 'timers/promises'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the Node.js builtin “node:” import protocol for clarity.
Static analysis recommends using the node: prefix for the built-in modules to avoid ambiguity and align with best practices.
-import fs from 'fs/promises';
-import path from 'path';
-import { setTimeout } from 'timers/promises';
+import fs from 'node:fs/promises';
+import path from 'node:path';
+import { setTimeout } from 'node:timers/promises';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import fs from 'fs/promises'; | |
| import path from 'path'; | |
| import { setTimeout } from 'timers/promises'; | |
| import fs from 'node:fs/promises'; | |
| import path from 'node:path'; | |
| import { setTimeout } from 'node:timers/promises'; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 3-3: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
internal/perf/package.json
Outdated
| { | ||
| "name": "@internal/perf", | ||
| "type": "module", | ||
| "private": true, | ||
| "main": "./src/index.ts", | ||
| "dependencies": { | ||
| "canvas": "^3.1.0", | ||
| "chart.js": "^4.4.7", | ||
| "chartjs-plugin-trendline": "^2.1.6", | ||
| "parse-duration": "^2.0.0", | ||
| "regression": "^2.0.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/k6": "^0.54.2", | ||
| "@types/regression": "^2" | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing Test Script and Documentation Update
The enriched summary mentioned a new script ("test:mem": "vitest --project memtest") to support memory testing locally, but it is currently absent from this configuration. Consider adding an appropriate "scripts" section with the test:mem command to facilitate local testing of the memtest functionality. Additionally, as this module implements new memory leak detection features, please update the corresponding documentation (for example, in the related console project) so that users have clear guidance on how to use these new capabilities.
[assertive]
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
internal/perf/src/memtest.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
internal/perf/src/memtest.ts
🪛 GitHub Check: bun
internal/perf/src/memtest.ts
[failure] 95-95: e2e/federation-example/federation-example.memtest.ts > should not have a memory increase trend
AssertionError: No memory decrease detected during calmdown: expected 0 to be less than or equal to -10
❯ internal/perf/src/memtest.ts:95:10
🪛 GitHub Actions: Memtest
internal/perf/src/memtest.ts
[error] 95-95: AssertionError: No memory decrease detected during calmdown: expected 0 to be less than or equal to -10
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: Examples / Convert operation-field-permissions
- GitHub Check: Examples / Convert type-merging-batching
- GitHub Check: Examples / Convert openapi-subscriptions
- GitHub Check: Examples / Convert openapi-javascript-wiki
- GitHub Check: Examples / Convert interface-additional-resolvers
- GitHub Check: Examples / Convert federation-mixed
- GitHub Check: Binary built on windows-latest
- GitHub Check: Binary built on macos-14
- GitHub Check: Bun Docker image
- GitHub Check: Binary built on macos-13
- GitHub Check: Node Docker image
- GitHub Check: Binary built on ubuntu-latest
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Leaks / Node v20
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Leaks / Node v18
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: bun / 10 items
🔇 Additional comments (2)
internal/perf/src/memtest.ts (2)
36-36: Add documentation for the new functionality in the console project.Since
memtestis a new core feature, ensure that documentation is added or updated in thegraphql-hive/consolerepository to guide users on setup, configuration, and usage.
50-96: Gracefully stop the server after the test.As noted in a previous review, consider wrapping the loadtest call in a
try/finallyblock to stop the created server instance even if the test fails. This helps avoid leaving resources open in local or CI environments.🧰 Tools
🪛 GitHub Check: bun
[failure] 95-95: e2e/federation-example/federation-example.memtest.ts > should not have a memory increase trend
AssertionError: No memory decrease detected during calmdown: expected 0 to be less than or equal to -10
❯ internal/perf/src/memtest.ts:95:10🪛 GitHub Actions: Memtest
[error] 95-95: AssertionError: No memory decrease detected during calmdown: expected 0 to be less than or equal to -10
internal/perf/src/memtest.ts
Outdated
| .soft(calmdownSlope, `No memory decrease detected during calmdown`) | ||
| .toBeLessThanOrEqual(-10); |
There was a problem hiding this comment.
Pipeline fails due to strict memory decrease threshold.
The memtest pipeline is failing because .toBeLessThanOrEqual(-10) expects a slope of -10 or lower, yet the actual slope is 0. Confirm whether this indicates a genuine memory leak or if the threshold is simply too strict. Consider reducing the threshold to a more reasonable value if you’re not expecting such a large drop in memory usage:
- .toBeLessThanOrEqual(-10);
+ .toBeLessThanOrEqual(0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .soft(calmdownSlope, `No memory decrease detected during calmdown`) | |
| .toBeLessThanOrEqual(-10); | |
| .soft(calmdownSlope, `No memory decrease detected during calmdown`) | |
| .toBeLessThanOrEqual(0); |
🧰 Tools
🪛 GitHub Check: bun
[failure] 95-95: e2e/federation-example/federation-example.memtest.ts > should not have a memory increase trend
AssertionError: No memory decrease detected during calmdown: expected 0 to be less than or equal to -10
❯ internal/perf/src/memtest.ts:95:10
🪛 GitHub Actions: Memtest
[error] 95-95: AssertionError: No memory decrease detected during calmdown: expected 0 to be less than or equal to -10
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
internal/perf/src/loadtest.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
internal/perf/src/loadtest.ts
🪛 Biome (1.9.4)
internal/perf/src/loadtest.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 3-3: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
⏰ Context from checks skipped due to timeout of 90000ms (23)
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: Unit / Bun
- GitHub Check: Bundle
- GitHub Check: Leaks / Node v22
- GitHub Check: bun / 1000 items
- GitHub Check: bun / 100 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: bun / 10 items
- GitHub Check: Leaks / Node v20
- GitHub Check: node / 1000 items
- GitHub Check: Leaks / Node v18
- GitHub Check: node / 100 items
- GitHub Check: node / 10 items
- GitHub Check: bun
- GitHub Check: node
🔇 Additional comments (3)
internal/perf/src/loadtest.ts (3)
1-3: Use the Node.js built-in modules with thenode:prefix.This aligns with the best practice recommended by the static analysis tool and was previously flagged in the past review comments.
-import fs from 'fs/promises'; -import path from 'path'; -import { setTimeout } from 'timers/promises'; +import fs from 'node:fs/promises'; +import path from 'node:path'; +import { setTimeout } from 'node:timers/promises';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
[error] 3-3: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
93-113: Consider a user-friendly check ifk6is not installed.Spawning
k6without verifying its presence may cause a cryptic error ifk6is missing from the environment. As noted in a previous review, adding a pre-flight check helps catch this scenario early and improve the developer experience.
49-54: Confirm Node.js version support forAbortSignal.timeoutand[Symbol.dispose].Using
AbortSignal.timeout()andusing { [Symbol.dispose]() }is only supported in newer Node.js versions (≥17 forAbortSignal.timeout(), ≥20 for[Symbol.dispose]). Ensure the CI environment or deployment target meets these requirements.Please run a web query for the official Node.js documentation to confirm these are fully supported in your environment:
✅ Verification successful
🌐 Web query:
Node.js versions that support `[Symbol.dispose]` and `AbortSignal.timeout`💡 Result:
Node.js support for
Symbol.disposeandAbortSignal.timeout()can be summarized as follows:Symbol.dispose Support
- Native implementation: Available since Node.js v18.18 and v20.4 as built-in polyfills while awaiting full V8 engine integration[1][7].
- Full support: Added via TypeScript 5.2 compatibility with runtime polyfills[4][9].
- Key use cases:
- Automatically disposing resources like file handles or timers using
usingdeclarations[7][10].- Simplifying cleanup logic for objects like
Timeout(e.g.,using setInterval(...))[7][10].AbortSignal.timeout() Support
- Introduced natively in Node.js v18+ via updates to the
timers/promisesAPI[2][8].- Enables direct timeout configuration for operations like HTTP requests:
await fetch(url, { signal: AbortSignal.timeout(3000) });
- Throws a
TimeoutErrorDOMException on expiration[5][8].Version Compatibility Table
Feature Supported Node.js Versions Notes Symbol.dispose≥v18.18 / ≥v20.4 Polyfilled; requires TypeScript ≥5.2[1][4][9]. AbortSignal.timeout≥v18 Integrated with fetch/timeout APIs[2][5][8]. For production use:
- Ensure TypeScript polyfills (
Symbol.dispose/Symbol.asyncDispose) are included if targeting older runtimes[1][9].- Prefer native implementations in newer LTS releases (e.g., ≥v20)[10].
Citations:
- 1: https://www.npmjs.com/package/@shigen/polyfill-symbol-dispose
- 2: https://betterstack.com/community/guides/scaling-nodejs/understanding-abortcontroller/
- 3: Deprecate AbortSignal on resources that are not actions nodejs/node#48725
- 4: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-2.html
- 5: https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/timeout_static
- 6: https://nodejs.org/api/child_process.html
- 7: https://jonathan-frere.com/posts/disposables-in-javascript/
- 8: https://betterstack.com/community/guides/scaling-nodejs/nodejs-timeouts/
- 9: https://medium.aegis-techno.fr/js-explained-symbol-dispose-and-symbol-asyncdispose-cdd48c7d7f4b?gi=9d8ced112cf9
- 10: https://nodejs.org/api/timers.html
Review Verification: Node.js Versions Confirmed
The web query confirms that:
AbortSignal.timeout()is available in Node.js v18 and later.[Symbol.dispose]is supported via built-in polyfills from Node.js v18.18 and has full native support in Node.js v20.4.Ensure your CI and deployment environments meet these version requirements.
internal/perf/src/loadtest.ts
Outdated
| if (isDebug('loadtest')) { | ||
| const chart = createLineChart( | ||
| memoryInMBSnapshots.total.map( | ||
| (_, i) => `${i + memorySnapshotWindow / 1000}. sec`, | ||
| ), | ||
| [ | ||
| { | ||
| label: 'Idle', | ||
| data: memoryInMBSnapshots.idle, | ||
| }, | ||
| { | ||
| label: 'Loadtest', | ||
| data: [ | ||
| ...memoryInMBSnapshots.idle.map(() => null), // skip idle data | ||
| ...memoryInMBSnapshots.loadtest, | ||
| ], | ||
| }, | ||
| { | ||
| label: 'Calmdown', | ||
| data: [ | ||
| ...memoryInMBSnapshots.idle.map(() => null), // skip idle data | ||
| ...memoryInMBSnapshots.loadtest.map(() => null), // skip loadtest data | ||
| ...memoryInMBSnapshots.calmdown, | ||
| ], | ||
| }, | ||
| ], | ||
| { | ||
| yTicksCallback: (tickValue) => `${tickValue} MB`, | ||
| }, | ||
| ); | ||
| await fs.writeFile( | ||
| path.join(cwd, 'loadtest-memory-snapshots.svg'), | ||
| chart.toBuffer(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document this new loadtesting feature in the console project.
Given that this is a new piece of functionality, it’s helpful for teams or end-users to have accompanying documentation. Consider opening a follow-up issue in the graphql-hive/console repository to add relevant usage instructions or references so that it’s discoverable in your docs.
Shall I open the issue on your behalf?
internal/perf/src/loadtest.ts
Outdated
| function debugLog(msg: string) { | ||
| if (isDebug('loadtest')) { | ||
| console.log(`[loadtest] ${msg}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add tests or validation for debug logging.
Currently, there’s no test coverage verifying the contents or behavior of the debugLog function. Consider adding at least minimal tests to ensure that debug output is only produced under isDebug('loadtest') contexts.
.github/workflows/memtest.yml
Outdated
| strategy: | ||
| matrix: | ||
| e2e_runner: | ||
| - node | ||
| # - bun TODO: fix getting memory snaps for bun | ||
| name: ${{matrix.e2e_runner}} | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
| - name: Install k6 | ||
| run: | | ||
| mkdir -p "$HOME/.local/bin" | ||
| cd "$HOME/.local/bin" | ||
| curl https://github.com/grafana/k6/releases/download/${{ env.K6_VERSION }}/k6-${{ env.K6_VERSION }}-linux-amd64.tar.gz -L | tar xvz --strip-components 1 | ||
| echo "$PWD" >> $GITHUB_PATH | ||
| - name: Set up env | ||
| uses: the-guild-org/shared-config/setup@v1 | ||
| with: | ||
| node-version-file: .node-version | ||
| - name: Test | ||
| run: yarn test:mem | ||
| env: | ||
| E2E_GATEWAY_RUNNER: ${{matrix.e2e_runner}} |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
.github/workflows/memtest.yml (1)
16-42: Security: Missing 'permissions' Declaration.
The workflow currently lacks an explicitpermissionsblock. For improved security, it is recommended to restrict token scopes by adding a top-level permissions section. For example:+permissions: + contents: readThis change minimizes the access token's privileges and aligns with security best practices.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 18-41: Workflow does not contain permissions
Actions Job or Workflow does not set permissions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/memtest.yml(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
.github/workflows/memtest.yml
[warning] 18-41: Workflow does not contain permissions
Actions Job or Workflow does not set permissions
⏰ Context from checks skipped due to timeout of 90000ms (44)
- GitHub Check: Examples / Convert operation-field-permissions
- GitHub Check: Examples / Convert type-merging-batching
- GitHub Check: Examples / Convert subscriptions-with-transforms
- GitHub Check: Examples / Convert programmatic-batching
- GitHub Check: Examples / Convert openapi-subscriptions
- GitHub Check: Examples / Convert openapi-javascript-wiki
- GitHub Check: Examples / Convert openapi-arg-rename
- GitHub Check: Examples / Convert openapi-additional-resolvers
- GitHub Check: Examples / Convert json-schema-subscriptions
- GitHub Check: Examples / Convert interface-additional-resolvers
- GitHub Check: Examples / Convert hmac-auth-https
- GitHub Check: Examples / Convert federation-subscriptions-passthrough
- GitHub Check: Examples / Convert apq-subgraphs
- GitHub Check: Examples / Convert file-upload
- GitHub Check: Examples / Convert federation-mixed
- GitHub Check: Examples / Convert federation-example
- GitHub Check: Examples / Convert extra-fields
- GitHub Check: Binary built on ubuntu-24.04-arm
- GitHub Check: Binary built on windows-latest
- GitHub Check: Binary built on macos-14
- GitHub Check: Binary built on macos-13
- GitHub Check: Binary built on ubuntu-latest
- GitHub Check: Bun Docker image
- GitHub Check: Node Docker image
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: bun / 1000 items
- GitHub Check: bun / 100 items
- GitHub Check: Leaks / Node v20
- GitHub Check: bun / 10 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: node / 1000 items
- GitHub Check: Leaks / Node v18
- GitHub Check: node / 100 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: node / 10 items
- GitHub Check: node
🔇 Additional comments (6)
.github/workflows/memtest.yml (6)
1-7: Workflow Trigger Configuration is Clear.
The workflow name and event triggers (push on the main branch and pull requests) are well defined and easy to follow.
8-10: Concurrency Settings are Well Configured.
The concurrency block uses a dynamic group key (${{ github.workflow }}-${{ github.ref }}) withcancel-in-progress: trueto ensure that only the latest run per branch is maintained.
12-15: Environment Variables are Appropriately Configured.
DefiningNODE_NO_WARNINGSand pinning theK6_VERSION(v0.56.0) helps ensure consistent behavior during runtime and tool installation.
26-27: Checkout Step is Properly Configured.
The repository checkout usingactions/checkout@v4is standard and correctly implemented.
34-37: Action Version Pinning.
TheSet up envstep usesthe-guild-org/shared-config/setup@v1, which is not pinned to a specific commit hash. For enhanced security and reproducibility, consider pinning this action to an exact commit. For example:-uses: the-guild-org/shared-config/setup@v1 +uses: the-guild-org/shared-config/setup@<commit-hash>
38-41: Test Step is Clear.
The final step invokesyarn test:memto run the memory test suite, using the matrix-defined runner (E2E_GATEWAY_RUNNER). Ensure that the associated test script exists and is correctly configured.🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 18-41: Workflow does not contain permissions
Actions Job or Workflow does not set permissions
| - name: Install k6 | ||
| run: | | ||
| mkdir -p "$HOME/.local/bin" | ||
| cd "$HOME/.local/bin" | ||
| curl https://github.com/grafana/k6/releases/download/${{ env.K6_VERSION }}/k6-${{ env.K6_VERSION }}-linux-amd64.tar.gz -L | tar xvz --strip-components 1 | ||
| echo "$PWD" >> $GITHUB_PATH |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Install k6: Consider Enhanced Error Handling.
The script for installing k6 correctly creates the necessary directory, downloads, and extracts the binary. To further improve robustness, consider adding error handling by enabling immediate failure on errors (using set -e) and employing curl -f to fail if the download does not succeed. For example:
-run: |
- mkdir -p "$HOME/.local/bin"
- cd "$HOME/.local/bin"
- curl https://github.com/grafana/k6/releases/download/${{ env.K6_VERSION }}/k6-${{ env.K6_VERSION }}-linux-amd64.tar.gz -L | tar xvz --strip-components 1
- echo "$PWD" >> $GITHUB_PATH
+run: |
+ set -e
+ mkdir -p "$HOME/.local/bin"
+ cd "$HOME/.local/bin"
+ curl -f https://github.com/grafana/k6/releases/download/${{ env.K6_VERSION }}/k6-${{ env.K6_VERSION }}-linux-amd64.tar.gz -L | tar xvz --strip-components 1
+ echo "$PWD" >> $GITHUB_PATH📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Install k6 | |
| run: | | |
| mkdir -p "$HOME/.local/bin" | |
| cd "$HOME/.local/bin" | |
| curl https://github.com/grafana/k6/releases/download/${{ env.K6_VERSION }}/k6-${{ env.K6_VERSION }}-linux-amd64.tar.gz -L | tar xvz --strip-components 1 | |
| echo "$PWD" >> $GITHUB_PATH | |
| - name: Install k6 | |
| run: | | |
| set -e | |
| mkdir -p "$HOME/.local/bin" | |
| cd "$HOME/.local/bin" | |
| curl -f https://github.com/grafana/k6/releases/download/${{ env.K6_VERSION }}/k6-${{ env.K6_VERSION }}-linux-amd64.tar.gz -L | tar xvz --strip-components 1 | |
| echo "$PWD" >> $GITHUB_PATH |
| curl https://github.com/grafana/k6/releases/download/${{ env.K6_VERSION }}/k6-${{ env.K6_VERSION }}-linux-amd64.tar.gz -L | tar xvz --strip-components 1 | ||
| echo "$PWD" >> $GITHUB_PATH | ||
| - name: Set up env | ||
| uses: the-guild-org/shared-config/setup@v1 |
Check warning
Code scanning / CodeQL
Unpinned tag for a non-immutable Action in workflow Medium
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/memtest.yml(1 hunks)internal/perf/src/loadtest.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
internal/perf/src/loadtest.ts
🪛 GitHub Check: CodeQL
.github/workflows/memtest.yml
[warning] 18-42: Workflow does not contain permissions
Actions Job or Workflow does not set permissions
[warning] 35-35: Unpinned tag for a non-immutable Action in workflow
Unpinned 3rd party Action 'Memtest' step Uses Step uses 'the-guild-org/shared-config/setup' with ref 'v1', not a pinned commit hash
🪛 Biome (1.9.4)
internal/perf/src/loadtest.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 3-3: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
🔇 Additional comments (5)
internal/perf/src/loadtest.ts (4)
1-3: Use thenode:prefix for built-in modules.This new file still imports Node.js built-ins without the
node:protocol. It's recommended to adopt the explicit protocol for clarity.-import fs from 'fs/promises'; -import path from 'path'; -import { setTimeout } from 'timers/promises'; +import fs from 'node:fs/promises'; +import path from 'node:path'; +import { setTimeout } from 'node:timers/promises';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
[error] 3-3: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
32-63: Document the new loadtesting feature in the console project.Since this is new functionality, consider adding or updating documentation in the
consolerepo to help users discover and leverage the loadtest utility.
93-113: Clarify behavior if k6 is not installed.The
spawncall could fail silently ifk6isn’t on the PATH. Adding a check or a more descriptive error message would help.
159-163: Add tests for debug logging.Currently, there is no coverage verifying correct debug output. Consider adding minimal tests to ensure logs are generated only under
isDebug('loadtest')..github/workflows/memtest.yml (1)
28-33: Enhance error handling when installing k6.Use a fail-fast approach to better handle unsuccessful downloads:
-run: | +run: | + set -e mkdir -p "$HOME/.local/bin" cd "$HOME/.local/bin" - curl https://github.com/grafana/k6/releases/download/${{ env.K6_VERSION }}/k6-${{ env.K6_VERSION }}-linux-amd64.tar.gz -L | tar xvz --strip-components 1 + curl -f https://github.com/grafana/k6/releases/download/${{ env.K6_VERSION }}/k6-${{ env.K6_VERSION }}-linux-amd64.tar.gz -L | tar xvz --strip-components 1 echo "$PWD" >> $GITHUB_PATH
| NODE_NO_WARNINGS: 1 | ||
| K6_VERSION: v0.56.0 | ||
|
|
||
| jobs: |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Explicitly set workflow permissions.
Security best practices recommend specifying permissions even if using none or read-only. Consider adding a block like:
permissions:
contents: read| curl https://github.com/grafana/k6/releases/download/${{ env.K6_VERSION }}/k6-${{ env.K6_VERSION }}-linux-amd64.tar.gz -L | tar xvz --strip-components 1 | ||
| echo "$PWD" >> $GITHUB_PATH | ||
| - name: Set up env | ||
| uses: the-guild-org/shared-config/setup@v1 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Pin the Action version to a commit SHA.
Using an unpinned reference (@v1) can lead to unexpected behavior if the action changes. Pinning the version helps ensure reproducible workflows.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 35-35: Unpinned tag for a non-immutable Action in workflow
Unpinned 3rd party Action 'Memtest' step Uses Step uses 'the-guild-org/shared-config/setup' with ref 'v1', not a pinned commit hash
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (9)
e2e/opentelemetry/opentelemetry.memtest.ts (1)
10-24:⚠️ Potential issueAdd container teardown step.
The Jaeger container is launched but never explicitly stopped. This can lead to resource leaks, especially during CI runs or local testing.
Apply this diff to add the cleanup:
describe(`OpenTelemetry ${OTLP_EXPORTER_TYPE} exporter`, () => { let jaeger: Container; beforeAll(async () => { jaeger = await container({ // ... }); }); + afterAll(async () => { + await jaeger?.stop(); + });internal/perf/src/chart.ts (1)
28-28: 🧹 Nitpick (assertive)Parameterize the canvas dimensions.
This was suggested in a previous review but hasn’t been addressed. Allowing custom width/height parameters would improve flexibility.
- const canvas = createCanvas(1366, 768, 'svg'); +export function createLineChart( + labels: (number | string | null)[], + datasets: LineChartDataset[], + options: LineChartOptions = {}, + width = 1366, + height = 768 +): Canvas { + const canvas = createCanvas(width, height, 'svg'); ... }internal/perf/src/loadtest.ts (2)
118-125: 🧹 Nitpick (assertive)Check if k6 is installed or provide a user-friendly error message.
A previous comment suggested verifying the presence of k6. If k6 is missing, an explicit warning or pre-flight check could be more helpful than a cryptic spawn error.
Would you like assistance in adding a pre-flight check to handle missing k6?
1-2: 🧹 Nitpick (assertive)Use the Node.js "node:" import protocol.
Static analysis recommends adding the
node:prefix to built-in modules for clarity.-import path from 'path'; -import { setTimeout } from 'timers/promises'; +import path from 'node:path'; +import { setTimeout } from 'node:timers/promises';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
internal/perf/src/memtest.ts (1)
41-41: 🧹 Nitpick (assertive)Document this new memtest feature in the console project.
Following the guidelines, new functionalities typically need documentation there. You could open a follow-up issue in
graphql-hive/consoleto make it discoverable.internal/perf/package.json (1)
1-17:⚠️ Potential issueAdd missing test:mem script.
The workflow uses
yarn test:membut the script is missing from package.json.{ "name": "@internal/perf", "type": "module", "private": true, "main": "./src/index.ts", + "scripts": { + "test:mem": "vitest --project memtest" + }, "dependencies": {.github/workflows/memtest.yml (3)
1-11: 🛠️ Refactor suggestionAdd permissions block for security.
Following security best practices, explicitly set permissions for the workflow.
name: Memtest on: push: branches: - main pull_request: +permissions: + contents: read + concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true
28-33: 🧹 Nitpick (assertive)Enhance k6 installation reliability.
Add error handling to ensure the installation fails fast on errors.
run: | + set -e mkdir -p "$HOME/.local/bin" cd "$HOME/.local/bin" - curl https://github.com/grafana/k6/releases/download/${{ env.K6_VERSION }}/k6-${{ env.K6_VERSION }}-linux-amd64.tar.gz -L | tar xvz --strip-components 1 + curl -f https://github.com/grafana/k6/releases/download/${{ env.K6_VERSION }}/k6-${{ env.K6_VERSION }}-linux-amd64.tar.gz -L | tar xvz --strip-components 1 echo "$PWD" >> $GITHUB_PATH
35-37: 🛠️ Refactor suggestionPin the action version to a commit SHA.
Using an unpinned reference (@v1) can lead to unexpected behavior if the action changes.
- name: Set up env - uses: the-guild-org/shared-config/setup@v1 + uses: the-guild-org/shared-config/setup@5c5a9a9 with: node-version-file: .node-version🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 35-35: Unpinned tag for a non-immutable Action in workflow
Unpinned 3rd party Action 'Memtest' step Uses Step uses 'the-guild-org/shared-config/setup' with ref 'v1', not a pinned commit hash
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
.github/workflows/bench.yml(1 hunks).github/workflows/memtest.yml(1 hunks).gitignore(1 hunks)e2e/federation-example/federation-example.memtest.ts(1 hunks)e2e/opentelemetry/opentelemetry.memtest.ts(1 hunks)internal/e2e/src/tenv.ts(3 hunks)internal/examples/src/convert.ts(1 hunks)internal/perf/package.json(1 hunks)internal/perf/src/chart.ts(1 hunks)internal/perf/src/index.ts(1 hunks)internal/perf/src/loadtest-script.ts(1 hunks)internal/perf/src/loadtest.ts(1 hunks)internal/perf/src/memtest.ts(1 hunks)internal/proc/src/index.ts(3 hunks)internal/testing/src/env.ts(1 hunks)package.json(1 hunks)tsconfig.json(1 hunks)vitest.projects.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
internal/perf/src/index.tstsconfig.jsonvitest.projects.tse2e/federation-example/federation-example.memtest.tse2e/opentelemetry/opentelemetry.memtest.tsinternal/perf/src/loadtest-script.tsinternal/perf/package.jsoninternal/proc/src/index.tsinternal/testing/src/env.tsinternal/e2e/src/tenv.tspackage.jsoninternal/examples/src/convert.tsinternal/perf/src/chart.tsinternal/perf/src/memtest.tsinternal/perf/src/loadtest.ts
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/federation-example/federation-example.memtest.tse2e/opentelemetry/opentelemetry.memtest.ts
🪛 GitHub Check: CodeQL
.github/workflows/memtest.yml
[warning] 18-42: Workflow does not contain permissions
Actions Job or Workflow does not set permissions
[warning] 35-35: Unpinned tag for a non-immutable Action in workflow
Unpinned 3rd party Action 'Memtest' step Uses Step uses 'the-guild-org/shared-config/setup' with ref 'v1', not a pinned commit hash
🪛 Biome (1.9.4)
internal/perf/src/memtest.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
internal/perf/src/loadtest.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
⏰ Context from checks skipped due to timeout of 90000ms (31)
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Leaks / Node v20
- GitHub Check: Leaks / Node v18
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: bun / 1000 items
- GitHub Check: Snapshot / snapshot
- GitHub Check: Unit / Bun
- GitHub Check: Bundle
- GitHub Check: bun / 100 items
- GitHub Check: Unit / Node v23
- GitHub Check: Unit / Node v22
- GitHub Check: bun / 10 items
- GitHub Check: Build
- GitHub Check: node / 1000 items
- GitHub Check: Types
- GitHub Check: Unit / Node v20
- GitHub Check: node / 100 items
- GitHub Check: Unit / Node v18
- GitHub Check: Format
- GitHub Check: node / 10 items
- GitHub Check: node
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (20)
e2e/opentelemetry/opentelemetry.memtest.ts (2)
1-9: LGTM! Clean and well-structured setup.The imports and initial setup follow the established e2e testing patterns and include all necessary utilities.
1-1: Add documentation in the console repository.Since this PR adds new functionality for memory leak detection, corresponding documentation should be added in the
graphql-hive/consolerepository.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check for documentation PRs in the console repository gh search prs \ --repo graphql-hive/console \ --json title,url \ --jq '.[] | select(.title | contains("memory") or contains("leak") or contains("GW-154"))' \ --state allLength of output: 3480
Documentation missing in the console repository
Our verification confirms that no related documentation PR has been found for the memory leak detection functionality in the
graphql-hive/consolerepository. Please ensure that corresponding documentation is added to properly support this new functionality.
- No open or closed documentation PRs matching keywords (“memory”, “leak”, “GW-154”) exist in
graphql-hive/console.internal/perf/src/chart.ts (1)
1-21: Interfaces look good.The definitions for
LineChartDatasetandLineChartOptionsare straightforward and well-documented.internal/perf/src/memtest.ts (1)
137-138: Confirm the strict memory decrease threshold.Your test requires the calmdown slope ≤ -10. If the pipeline fails because the slope is not that negative, you may need to adjust it or verify the expected behavior.
- .toBeLessThanOrEqual(-10); + .toBeLessThanOrEqual(0);internal/perf/src/index.ts (1)
1-1: LGTM! Clean and concise export statement.The export statement follows TypeScript best practices for re-exporting all entities from a module.
e2e/federation-example/federation-example.memtest.ts (1)
9-18: Add idle memory verification.Your PR objectives mention an idle memory check after load testing, but it's not implemented in this test.
internal/perf/src/loadtest-script.ts (2)
6-14: Consider adding type validation for environment variables.While the existence check is good, consider validating the URL format and query structure.
16-21: Consider enhancing response validation.The current response validation is basic. Consider adding more comprehensive checks.
vitest.projects.ts (1)
47-55: LGTM! The memtest configuration is properly structured.The new memtest configuration correctly extends the base configuration and maintains consistency with other test suites by using the same timeout settings.
However, consider documenting the purpose of this test suite.
internal/testing/src/env.ts (3)
9-9: Fix the typo in the JSDoc.Use "environment" instead of "enviorment".
- * variable `loadtest` is listed in the `DEBUG` enviorment variable. + * variable `loadtest` is listed in the `DEBUG` environment variable.
11-17: Consider adding input validation for the module parameter.The function could benefit from validating the module name format.
export function isDebug(module?: string) { + if (module && !/^[a-z0-9-_]+$/i.test(module)) { + throw new Error('Module name must contain only alphanumeric characters, hyphens, and underscores'); + } if (module) { return boolEnv(`DEBUG_${module}`); } return boolEnv('DEBUG'); }
11-17: Handle unset or emptyDEBUGenvironment variable more robustly.When
process.env['DEBUG']is not defined or empty,String(process.env['DEBUG'])becomes "undefined" or "". Consider a fallback to an empty array to avoid unintended matches.export function isDebug(module?: string) { if (module) { - const modules = String(process.env['DEBUG']) + const debugStr = process.env['DEBUG'] ?? ''; + if (!debugStr) return false; + const modules = debugStr .split(',') .map((m) => m.trim()); return modules.includes(module); } return boolEnv('DEBUG'); }internal/proc/src/index.ts (2)
11-13: LGTM! Good addition of process lifecycle management.The
waitForExitproperty enhances theProcinterface by providing a Promise-based way to track process completion, which is a clean and idiomatic approach in async JavaScript/TypeScript.
22-25: LGTM! Well-structured server interface.The
Serverinterface is a clean extension ofProcthat properly encapsulates server-specific properties. The interface follows the Interface Segregation Principle by adding only the essential properties needed for server processes.internal/e2e/src/tenv.ts (2)
709-714: LGTM! Clean Service interface implementation.The implementation correctly spreads the Proc properties and adds the required Service-specific properties in a concise manner.
873-873: LGTM! Proper container lifecycle management.The addition of
waitForExitusingctr.wait()provides proper container lifecycle management, aligning with the Proc interface changes.internal/examples/src/convert.ts (1)
183-184: LGTM! Consistent file exclusion pattern.The addition of
.memtest.file exclusion follows the existing pattern and properly supports the new memory testing functionality..gitignore (1)
15-15: LGTM!The new ignore pattern correctly excludes memory test snapshot SVGs generated during testing.
.github/workflows/bench.yml (1)
24-24: LGTM!The simplified job name improves readability in the GitHub Actions UI.
tsconfig.json (1)
17-18: LGTM!The path mappings are correctly configured for the new performance testing module.
| const jaegerUrls = { | ||
| get http() { | ||
| return `http://0.0.0.0:${jaeger.port}/v1/traces`; | ||
| }, | ||
| get grpc() { | ||
| return `http://0.0.0.0:${jaeger.additionalPorts[4317]}`; | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add URL validation for Jaeger endpoints.
The URLs are constructed without validation. Consider adding checks to ensure the ports are available and the URLs are properly formatted.
const jaegerUrls = {
get http() {
+ const url = `http://0.0.0.0:${jaeger.port}/v1/traces`;
+ if (!jaeger.port) throw new Error('HTTP port not available');
- return `http://0.0.0.0:${jaeger.port}/v1/traces`;
+ return url;
},
get grpc() {
+ const port = jaeger.additionalPorts[4317];
+ if (!port) throw new Error('gRPC port not available');
+ const url = `http://0.0.0.0:${port}`;
- return `http://0.0.0.0:${jaeger.additionalPorts[4317]}`;
+ return url;
},
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const jaegerUrls = { | |
| get http() { | |
| return `http://0.0.0.0:${jaeger.port}/v1/traces`; | |
| }, | |
| get grpc() { | |
| return `http://0.0.0.0:${jaeger.additionalPorts[4317]}`; | |
| }, | |
| }; | |
| const jaegerUrls = { | |
| get http() { | |
| const url = `http://0.0.0.0:${jaeger.port}/v1/traces`; | |
| if (!jaeger.port) throw new Error('HTTP port not available'); | |
| return url; | |
| }, | |
| get grpc() { | |
| const port = jaeger.additionalPorts[4317]; | |
| if (!port) throw new Error('gRPC port not available'); | |
| const url = `http://0.0.0.0:${port}`; | |
| return url; | |
| }, | |
| }; |
| beforeAll(async () => { | ||
| jaeger = await container({ | ||
| name: `jaeger-${OTLP_EXPORTER_TYPE}`, | ||
| image: 'jaegertracing/all-in-one:1.56', |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Jaeger image version.
The Jaeger image version is hardcoded to '1.56'. Consider implementing version verification to ensure compatibility and security updates.
🏁 Script executed:
#!/bin/bash
# Description: Check for latest Jaeger image version and security advisories
# Check DockerHub for latest versions
curl -s https://hub.docker.com/v2/repositories/jaegertracing/all-in-one/tags\?page_size=5 | jq -r '.results[].name'
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: CONTAINER, package: "jaegertracing/all-in-one") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Length of output: 953
Jaeger Image Version Outdated – Action Needed
- Current Issue: The file
e2e/opentelemetry/opentelemetry.memtest.tsis using the hardcoded image tagjaegertracing/all-in-one:1.56, which is older than the versions currently available on DockerHub (e.g.,1.66.0,1.65.0, etc.). - Recommendation: Update the image version or implement a dynamic version verification mechanism to ensure compatibility with the latest releases and security updates.
- Additional Note: The security advisories check failed due to an invalid API parameter (
ecosystemset toCONTAINER). This query needs correction if security vulnerability checks are to be integrated.
| memtest( | ||
| { | ||
| cwd, | ||
| query, | ||
| }, | ||
| async () => | ||
| gateway({ | ||
| supergraph: await supergraph(), | ||
| env: { | ||
| OTLP_EXPORTER_TYPE, | ||
| OTLP_EXPORTER_URL: jaegerUrls[OTLP_EXPORTER_TYPE], | ||
| OTLP_SERVICE_NAME: `memtest-${OTLP_EXPORTER_TYPE}`, | ||
| }, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document memory testing strategy and thresholds.
The memory test configuration lacks documentation about the testing strategy and explicit memory leak detection criteria. Based on the PR objectives, this should include memory increase slope and idle memory checks.
Add JSDoc comments to document the testing strategy:
+ /**
+ * Memory test configuration for OpenTelemetry exporters
+ *
+ * Strategy:
+ * 1. Monitor memory increase slope during load testing
+ * 2. Check idle memory after load testing
+ * 3. Fail if memory increase exceeds threshold or doesn't stabilize
+ *
+ * @threshold Maximum allowed memory increase rate (MB/min)
+ * @idleTime Time to wait for memory stabilization (ms)
+ * @idleThreshold Maximum allowed memory fluctuation during idle (MB)
+ */
memtest(
{
cwd,
query,
+ threshold: 5, // MB/min
+ idleTime: 30000, // 30 seconds
+ idleThreshold: 1, // 1 MB
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| memtest( | |
| { | |
| cwd, | |
| query, | |
| }, | |
| async () => | |
| gateway({ | |
| supergraph: await supergraph(), | |
| env: { | |
| OTLP_EXPORTER_TYPE, | |
| OTLP_EXPORTER_URL: jaegerUrls[OTLP_EXPORTER_TYPE], | |
| OTLP_SERVICE_NAME: `memtest-${OTLP_EXPORTER_TYPE}`, | |
| }, | |
| }), | |
| ); | |
| /** | |
| * Memory test configuration for OpenTelemetry exporters | |
| * | |
| * Strategy: | |
| * 1. Monitor memory increase slope during load testing | |
| * 2. Check idle memory after load testing | |
| * 3. Fail if memory increase exceeds threshold or doesn't stabilize | |
| * | |
| * @threshold Maximum allowed memory increase rate (MB/min) | |
| * @idleTime Time to wait for memory stabilization (ms) | |
| * @idleThreshold Maximum allowed memory fluctuation during idle (MB) | |
| */ | |
| memtest( | |
| { | |
| cwd, | |
| query, | |
| threshold: 5, // MB/min | |
| idleTime: 30000, // 30 seconds | |
| idleThreshold: 1, // 1 MB | |
| }, | |
| async () => | |
| gateway({ | |
| supergraph: await supergraph(), | |
| env: { | |
| OTLP_EXPORTER_TYPE, | |
| OTLP_EXPORTER_URL: jaegerUrls[OTLP_EXPORTER_TYPE], | |
| OTLP_SERVICE_NAME: `memtest-${OTLP_EXPORTER_TYPE}`, | |
| }, | |
| }), | |
| ); |
internal/perf/package.json
Outdated
| "dependencies": { | ||
| "canvas": "^3.1.0", | ||
| "chart.js": "^4.4.7", | ||
| "chartjs-plugin-trendline": "^2.1.6", | ||
| "parse-duration": "^2.0.0", | ||
| "regression": "^2.0.1" | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Dependencies look good but consider pinning versions.
The dependencies align well with the memory testing requirements. However, consider pinning the versions for better reproducibility.
"dependencies": {
- "canvas": "^3.1.0",
- "chart.js": "^4.4.7",
- "chartjs-plugin-trendline": "^2.1.6",
- "parse-duration": "^2.0.0",
- "regression": "^2.0.1"
+ "canvas": "3.1.0",
+ "chart.js": "4.4.7",
+ "chartjs-plugin-trendline": "2.1.6",
+ "parse-duration": "2.0.0",
+ "regression": "2.0.1"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "dependencies": { | |
| "canvas": "^3.1.0", | |
| "chart.js": "^4.4.7", | |
| "chartjs-plugin-trendline": "^2.1.6", | |
| "parse-duration": "^2.0.0", | |
| "regression": "^2.0.1" | |
| }, | |
| "dependencies": { | |
| - "canvas": "^3.1.0", | |
| - "chart.js": "^4.4.7", | |
| - "chartjs-plugin-trendline": "^2.1.6", | |
| - "parse-duration": "^2.0.0", | |
| - "regression": "^2.0.1" | |
| + "canvas": "3.1.0", | |
| + "chart.js": "4.4.7", | |
| + "chartjs-plugin-trendline": "2.1.6", | |
| + "parse-duration": "2.0.0", | |
| + "regression": "2.0.1" | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (11)
internal/perf/src/chart.ts (2)
22-73: 🧹 Nitpick (assertive)Add corresponding console documentation for new charting functionality
This new charting utility is useful, but may not be sufficiently discoverable across teams.(Same suggestion as a past comment: consider adding or updating documentation in the
graphql-hive/consolerepository so that teams know how to generate and use these charts.)
1-5: 🧹 Nitpick (assertive)Consider making canvas dimensions configurable
Currently, the canvas is fixed at1366 × 768. For flexibility in different environments, you could add parameters to set custom canvas width and height.- const canvas = createCanvas(1366, 768, 'svg'); + export function createLineChart( + labels: (number | string | null)[], + datasets: LineChartDataset[], + options: LineChartOptions = {}, + canvasWidth = 1366, + canvasHeight = 768 + ): Canvas { + const canvas = createCanvas(canvasWidth, canvasHeight, 'svg'); ... }internal/perf/src/inspector.ts (1)
1-2: 🧹 Nitpick (assertive)Use the Node.js import protocol
Static analysis notes that Node.js built-in imports should use thenode:prefix for clarity.-import fs from 'fs/promises'; -import { setTimeout } from 'timers/promises'; +import fs from 'node:fs/promises'; +import { setTimeout } from 'node:timers/promises';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
internal/perf/src/loadtest.ts (3)
128-135: 🧹 Nitpick (assertive)Check for the presence of
k6
Ifk6isn’t installed, the load test fails in unclear ways.(Same suggestion noted before: add a pre-flight check or handle an error gracefully if
k6isn’t found on thePATH.)
44-148: 🧹 Nitpick (assertive)Add documentation for new loadtesting feature
This is a significant new feature that might need a usage explanation in your docs orgraphql-hive/console.(Same suggestion from previous reviews: track or open a follow-up issue to document these capabilities.)
1-2: 🧹 Nitpick (assertive)Adopt
node:prefix for built-in modules
Again, static analysis recommends using thenode:protocol for clarity with Node.js built-in modules.-import path from 'path'; -import { setTimeout } from 'timers/promises'; +import path from 'node:path'; +import { setTimeout } from 'node:timers/promises';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
internal/perf/package.json (1)
1-22: 🧹 Nitpick (assertive)Add test:mem script and consider pinning dependency versions.
The dependencies align well with the memory testing requirements. However, consider pinning the versions for better reproducibility and adding the test script.
Apply this diff to pin versions and add the test script:
{ "name": "@internal/perf", "type": "module", "private": true, "main": "./src/index.ts", + "scripts": { + "test:mem": "vitest --project memtest" + }, "dependencies": { "@graphql-tools/delegate": "workspace:^", "@memlab/core": "patch:@memlab/core@npm%3A1.1.39#~/.yarn/patches/@memlab-core-npm-1.1.39-34d29dabbd.patch", - "canvas": "^3.1.0", - "chart.js": "^4.4.7", - "chartjs-plugin-trendline": "^2.1.6", - "memlab": "^1.1.56", - "parse-duration": "^2.0.0", - "regression": "^2.0.1", - "ws": "^8.18.0" + "canvas": "3.1.0", + "chart.js": "4.4.7", + "chartjs-plugin-trendline": "2.1.6", + "memlab": "1.1.56", + "parse-duration": "2.0.0", + "regression": "2.0.1", + "ws": "8.18.0" }, "devDependencies": { - "@types/k6": "^0.54.2", - "@types/regression": "^2", - "@types/ws": "^8.5.12" + "@types/k6": "0.54.2", + "@types/regression": "2.0.0", + "@types/ws": "8.5.12" } }.github/workflows/memtest.yml (4)
35-35: 🛠️ Refactor suggestionPin the Shared Config Action Version
The
Set up envstep uses an unpinned reference (@v1) forthe-guild-org/shared-config/setup. Pinning to a specific commit hash ensures reproducibility and avoids unexpected changes.Example suggestion:
-uses: the-guild-org/shared-config/setup@v1 +uses: the-guild-org/shared-config/setup@<commit-hash>🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 35-35: Unpinned tag for a non-immutable Action in workflow
Unpinned 3rd party Action 'Memtest' step Uses Step uses 'the-guild-org/shared-config/setup' with ref 'v1', not a pinned commit hash
1-7: 🧹 Nitpick (assertive)
⚠️ Potential issueAdd Explicit Workflow Permissions
Security best practices recommend explicitly setting workflow permissions. Consider adding a permissions block (e.g.,
contents: read) at the top of the workflow to avoid potential security risks.Suggested diff:
+permissions: + contents: readPlace this block near the top (after the
on:section) of the workflow file.
29-33: 🧹 Nitpick (assertive)Enhance k6 Installation Resilience
The
Install k6step currently lacks error handling. To improve resilience, addset -eat the beginning and usecurl -fto ensure the command fails if the download is unsuccessful.Suggested diff:
-run: | - mkdir -p "$HOME/.local/bin" - cd "$HOME/.local/bin" - curl https://github.com/grafana/k6/releases/download/${{ env.K6_VERSION }}/k6-${{ env.K6_VERSION }}-linux-amd64.tar.gz -L | tar xvz --strip-components 1 - echo "$PWD" >> $GITHUB_PATH +run: | + set -e + mkdir -p "$HOME/.local/bin" + cd "$HOME/.local/bin" + curl -f https://github.com/grafana/k6/releases/download/${{ env.K6_VERSION }}/k6-${{ env.K6_VERSION }}-linux-amd64.tar.gz -L | tar xvz --strip-components 1 + echo "$PWD" >> $GITHUB_PATH
24-25: 🧹 Nitpick (assertive)Enforce a Job-level Timeout
To avoid potential hangs or overly long test executions, consider adding a job-level timeout. For example, adding
timeout-minutes: 30immediately afterruns-on: ubuntu-latestcan help prevent runaway jobs.Example placement:
runs-on: ubuntu-latest +timeout-minutes: 30 steps:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
.yarn/patches/@memlab-core-npm-1.1.39-34d29dabbd.patchis excluded by!**/.yarn/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (21)
.github/workflows/bench.yml(1 hunks).github/workflows/memtest.yml(1 hunks).gitignore(1 hunks)e2e/cloudflare-workers/cloudflare-workers.e2e.ts(1 hunks)e2e/federation-example/federation-example.memtest.ts(1 hunks)e2e/opentelemetry/opentelemetry.memtest.ts(1 hunks)internal/e2e/src/tenv.ts(9 hunks)internal/examples/src/convert.ts(1 hunks)internal/perf/package.json(1 hunks)internal/perf/src/chart.ts(1 hunks)internal/perf/src/heap.ts(1 hunks)internal/perf/src/index.ts(1 hunks)internal/perf/src/inspector.ts(1 hunks)internal/perf/src/loadtest-script.ts(1 hunks)internal/perf/src/loadtest.ts(1 hunks)internal/perf/src/memtest.ts(1 hunks)internal/proc/src/index.ts(3 hunks)internal/testing/src/env.ts(1 hunks)package.json(1 hunks)tsconfig.json(1 hunks)vitest.projects.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
internal/perf/src/heap.tsinternal/examples/src/convert.tse2e/federation-example/federation-example.memtest.tsinternal/testing/src/env.tsinternal/perf/src/index.tsvitest.projects.tspackage.jsone2e/opentelemetry/opentelemetry.memtest.tstsconfig.jsone2e/cloudflare-workers/cloudflare-workers.e2e.tsinternal/perf/src/loadtest-script.tsinternal/perf/package.jsoninternal/proc/src/index.tsinternal/perf/src/chart.tsinternal/perf/src/loadtest.tsinternal/perf/src/memtest.tsinternal/perf/src/inspector.tsinternal/e2e/src/tenv.ts
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/federation-example/federation-example.memtest.tse2e/opentelemetry/opentelemetry.memtest.tse2e/cloudflare-workers/cloudflare-workers.e2e.ts
🪛 Biome (1.9.4)
e2e/cloudflare-workers/cloudflare-workers.e2e.ts
[error] 88-88: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 90-90: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
internal/perf/src/loadtest.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
internal/perf/src/memtest.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
internal/perf/src/inspector.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
🪛 GitHub Check: CodeQL
.github/workflows/memtest.yml
[warning] 18-42: Workflow does not contain permissions
Actions Job or Workflow does not set permissions
[warning] 35-35: Unpinned tag for a non-immutable Action in workflow
Unpinned 3rd party Action 'Memtest' step Uses Step uses 'the-guild-org/shared-config/setup' with ref 'v1', not a pinned commit hash
⏰ Context from checks skipped due to timeout of 90000ms (29)
- GitHub Check: Unit / Node v23
- GitHub Check: Unit / Node v22
- GitHub Check: Unit / Bun
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: Unit / Node v20
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: Unit / Node v18
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: Snapshot / snapshot
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Build
- GitHub Check: Leaks / Node v20
- GitHub Check: bun / 1000 items
- GitHub Check: bun / 100 items
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: bun / 10 items
- GitHub Check: Leaks / Node v18
- GitHub Check: node / 1000 items
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Bundle
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: node / 100 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: node
- GitHub Check: node / 10 items
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (25)
internal/perf/src/inspector.ts (1)
39-81: Ensure robust error handling for heap snapshot writing
If theHeapProfiler.takeHeapSnapshotcall fails or returns an invalid response, the code might remain in a waiting state. Consider adding additional checks for response integrity and possible errors.Would you like a script to search the codebase for other usage or error handling for
takeHeapSnapshotto confirm coverage?internal/perf/src/loadtest.ts (2)
60-62: Approved: Minimal duration check
It’s good practice to disallow very short durations to prevent meaningless load tests. This check is straightforward and clear.
71-71:✅ Verification successful
Verify TS version support for
usingstatement
TypeScript’susingsyntax is stable in recent releases. Ensure your environment targets a TS version >= 5.2.
🌐 Web query:
Is the `using` keyword stable in TypeScript 5.2 or later?💡 Result:
The
usingkeyword was officially introduced as a stable feature in TypeScript 5.2, released in August 2023[4][9]. It enables explicit resource management by automatically disposing of objects with aSymbol.disposemethod at the end of their scope, simplifying cleanup tasks like closing file handles or database connections[1][4][5].Key points:
- Stability: The feature is part of TypeScript 5.2's stable release[4][9], not experimental.
- Implementation: Based on the TC39 Explicit Resource Management proposal (Stage 3)[5], making it likely to become a future ECMAScript standard.
- Syntax:
using resource = new TempFile(".tmp"); // Automatically calls resource[Symbol.dispose]() when out of scope- Behavior: Works with both synchronous (
Symbol.dispose) and asynchronous (Symbol.asyncDispose) cleanup[2][4].- Adoption: Already supported in tooling like Prettier as of July 2023[10].
The feature simplifies code by eliminating manual
try/finallyblocks for resource cleanup[1][4]. For example:// Before let file; try { file = openFile(); } finally { file.close(); } // After TypeScript 5.2 using file = openFile(); // Automatic disposalThis functionality is now considered stable and production-ready in TypeScript 5.2 and later versions[4][9].
Citations:
- 1: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-2.html
- 2: https://prographers.com/blog/using-keyword-in-typescript-5-2
- 3: https://www.hatimeria.com/blog/article/the-amazing-using-keyword-from-typescript-5-2
- 4: https://devblogs.microsoft.com/typescript/announcing-typescript-5-2/
- 5: https://www.totaltypescript.com/typescript-5-2-new-keyword-using
- 6: Add support for using keyword (TypeScript 5.2) TypeStrong/ts-loader#1628
- 7: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-3.html
- 8: https://devblogs.microsoft.com/typescript/announcing-typescript-5-2-beta/
- 9: https://www.infoworld.com/article/2338724/typescript-52-brings-explicit-resource-management.html
- 10: Support TypeScript 5.2 prettier/prettier#15004
TS 5.2 Using Statement Confirmed
The
usingkeyword is officially stable as of TypeScript 5.2. The code on line 71 ininternal/perf/src/loadtest.tsis valid, provided that your project’s configuration targets TypeScript 5.2 or later. No further changes are needed in this area—just ensure your environment'stsconfig.json(or equivalent settings) specifies a compatible TS version.internal/perf/src/memtest.ts (2)
138-142: Verify the strictness of the calmdown slope threshold
A slope threshold of-10might cause false positives if memory usage doesn't decrease enough. Consider adjusting the threshold or documenting why-10is appropriate.
147-167: Linear regression slope calculation is well-implemented
The usage of theregressionlibrary and returningresult.equation[0]is correct and clear.internal/e2e/src/tenv.ts (5)
17-17: ImportingServerfrom@internal/procpromotes consistency
Centralizing the server definition in a single module helps maintain a clean architecture.
343-343: Spawn command parsing looks correct
Splitting the command string or array is straightforward. No issues detected here.
526-526: Use of--inspect-port=0is beneficial
Allocating the next available port prevents conflicts in shared environments. Good approach.
717-722: NewServiceobject structure is consistent
Mergingprocwith additional properties keeps the code DRY and clear.
881-885: Reinforce error message for container kill
This was previously flagged. Consider providing actionable guidance such as 'Use container.stop()' to explain alternative approaches.kill() { - throw new Error('Cannot send signals to containers.'); + throw new Error('Cannot send signals to containers. Use container.stop() instead.'); }internal/perf/src/index.ts (1)
1-1: Re-exporting loadtest utilities is clear and concise
This simple export helps consolidate the module's public API.e2e/federation-example/federation-example.memtest.ts (1)
9-18: Add idle memory verification or documentation.Your PR objectives mention an idle memory check after load testing and possible docs in
console. Consider incorporating a post-loadtest idle check or opening a follow-up issue to add relevant documentation ingraphql-hive/console.internal/perf/src/loadtest-script.ts (2)
6-14: Consider adding type validation for environment variables.While the existence check is good, consider validating the URL format and query structure.
16-21: Consider enhancing response validation.The current response validation is basic. Consider adding more comprehensive checks.
vitest.projects.ts (1)
47-55: LGTM! The memtest configuration is properly structured.The new memtest configuration correctly extends the base configuration and maintains consistency with other test suites by using the same timeout settings.
However, consider documenting the purpose of this test suite.
e2e/opentelemetry/opentelemetry.memtest.ts (3)
13-24: Add container cleanup to prevent resource leaks.The container is launched but never explicitly stopped. This can lead to resource leaks, especially in CI environments.
Apply this diff:
beforeAll(async () => { jaeger = await container({ name: `jaeger-${OTLP_EXPORTER_TYPE}`, image: 'jaegertracing/all-in-one:1.56', env: { COLLECTOR_OTLP_ENABLED: 'true', }, containerPort: 4318, additionalContainerPorts: [16686, 4317], healthcheck: ['CMD-SHELL', 'wget --spider http://0.0.0.0:14269'], }); }); + afterAll(async () => { + await jaeger.stop?.(); + });
25-32: Add URL validation for Jaeger endpoints.The URLs are constructed without validation. Consider adding checks to ensure the ports are available and the URLs are properly formatted.
Apply this diff:
const jaegerUrls = { get http() { + const url = `http://0.0.0.0:${jaeger.port}/v1/traces`; + if (!jaeger.port) throw new Error('HTTP port not available'); - return `http://0.0.0.0:${jaeger.port}/v1/traces`; + return url; }, get grpc() { + const port = jaeger.additionalPorts[4317]; + if (!port) throw new Error('gRPC port not available'); + const url = `http://0.0.0.0:${port}`; - return `http://0.0.0.0:${jaeger.additionalPorts[4317]}`; + return url; }, };
34-48: Document memory testing strategy and thresholds.Based on the PR objectives, this test should include memory increase slope and idle memory checks. The configuration lacks documentation about the testing strategy and explicit memory leak detection criteria.
Add JSDoc comments to document the testing strategy:
+ /** + * Memory test configuration for OpenTelemetry exporters + * + * Strategy: + * 1. Monitor memory increase slope during load testing + * 2. Check idle memory after load testing + * 3. Fail if memory increase exceeds threshold or doesn't stabilize + * + * @threshold Maximum allowed memory increase rate (MB/min) + * @idleTime Time to wait for memory stabilization (ms) + * @idleThreshold Maximum allowed memory fluctuation during idle (MB) + */ memtest( { cwd, query, + threshold: 5, // MB/min + idleTime: 30000, // 30 seconds + idleThreshold: 1, // 1 MB },internal/testing/src/env.ts (2)
9-9: Fix the typo in the JSDoc.The word "enviorment" is misspelled.
Apply this diff:
- * variable `loadtest` is listed in the `DEBUG` enviorment variable. + * variable `loadtest` is listed in the `DEBUG` environment variable.
11-17: Improve input validation and string handling.The function needs better input validation and more robust string handling.
Apply this diff:
export function isDebug(module?: string) { + if (module && !/^[a-z0-9-_]+$/i.test(module)) { + throw new Error('Module name must contain only alphanumeric characters, hyphens, and underscores'); + } if (module) { - const modules = String(process.env['DEBUG']) + const debugStr = process.env['DEBUG'] ?? ''; + if (!debugStr) return false; + const modules = debugStr .split(',') .map((m) => m.trim()); return modules.includes(module); } return boolEnv('DEBUG'); }internal/proc/src/index.ts (1)
89-91: Add error handling for kill signal.The
killmethod should handle invalid signals and process states.Apply this diff:
kill(signal) { + if (signal && !['SIGTERM', 'SIGKILL', 'SIGINT'].includes(signal)) { + throw new Error(`Invalid signal: ${signal}`); + } + if (!child.pid) { + throw new Error('Process is not running'); + } child.kill(signal); },e2e/cloudflare-workers/cloudflare-workers.e2e.ts (1)
81-92: Use template literals for string concatenation.The string concatenation can be improved using template literals.
Apply this diff:
await spawn([ 'yarn', 'wrangler', 'dev', '--port', port.toString(), '--var', - 'OTLP_EXPORTER_URL:' + env.OTLP_EXPORTER_URL, + `OTLP_EXPORTER_URL:${env.OTLP_EXPORTER_URL}`, '--var', - 'OTLP_SERVICE_NAME:' + env.OTLP_SERVICE_NAME, + `OTLP_SERVICE_NAME:${env.OTLP_SERVICE_NAME}`, ...(isDebug() ? ['--var', 'DEBUG:1'] : []), ]);🧰 Tools
🪛 Biome (1.9.4)
[error] 88-88: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 90-90: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
internal/examples/src/convert.ts (1)
183-184: LGTM! Exclusion of memtest files is correctly implemented.The addition of the
.memtest.exclusion condition aligns with the PR's memory leak detection objectives and follows the established pattern for file exclusions..gitignore (1)
15-16: LGTM! Memory test artifacts are correctly ignored.The new entries appropriately ignore memory test related files:
- Memory snapshot SVG files
- Heap snapshot files
.github/workflows/bench.yml (1)
24-24: LGTM! Workflow improvements enhance readability.The job name simplification improves readability in the checks overview, and the removal of redundant CI variable (as noted in past reviews) is correct since GitHub Actions sets it by default.
| export interface LineChartOptions { | ||
| /** | ||
| * The tick label callbacks of {@link LineChartDataset.data y} entries. | ||
| * | ||
| * TODO: separate the tick callback for each data. | ||
| */ | ||
| yTicksCallback?: (tickValue: number | string) => string; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remember the TODO
The TODO for separating the tick callback per dataset indicates a planned feature. If each dataset needs distinct formatting for the y-axis labels, consider adding a property like datasets[i].tickCallback.
Would you like me to propose a code snippet for multiple tick callbacks?
| let debuggerUrl = ''; | ||
| while (!debuggerUrl) { | ||
| await setTimeout(100); | ||
| for (const line of proc.getStd('err').split('\n')) { | ||
| debuggerUrl = line.split('Debugger listening on ')?.[1] || ''; | ||
| if (debuggerUrl) { | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prevent infinite loop if debuggerUrl is never found
If the process fails to print the debugger URL, the while (!debuggerUrl) loop could run indefinitely. Consider adding a maximum retry count or timeout to avoid hanging.
let attempts = 0;
while (!debuggerUrl) {
+ if (attempts++ > 1000) {
+ throw new Error("Debugger URL not found within reasonable time");
+ }
await setTimeout(100);
...
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let debuggerUrl = ''; | |
| while (!debuggerUrl) { | |
| await setTimeout(100); | |
| for (const line of proc.getStd('err').split('\n')) { | |
| debuggerUrl = line.split('Debugger listening on ')?.[1] || ''; | |
| if (debuggerUrl) { | |
| break; | |
| } | |
| } | |
| } | |
| let debuggerUrl = ''; | |
| let attempts = 0; | |
| while (!debuggerUrl) { | |
| if (attempts++ > 1000) { | |
| throw new Error("Debugger URL not found within reasonable time"); | |
| } | |
| await setTimeout(100); | |
| for (const line of proc.getStd('err').split('\n')) { | |
| debuggerUrl = line.split('Debugger listening on ')?.[1] || ''; | |
| if (debuggerUrl) { | |
| break; | |
| } | |
| } | |
| } |
| duration = 180_000, | ||
| calmdown = 30_000, | ||
| onMemorySnapshot, | ||
| ...loadtestOpts | ||
| } = opts; | ||
| it( | ||
| 'should have stable memory usage', | ||
| { | ||
| timeout: idle + duration + calmdown + 30_000, // allow 30s for the test teardown (compensate for heap snapshots) | ||
| }, | ||
| async ({ expect }) => { | ||
| const server = await setup(); | ||
|
|
||
| const startTime = new Date() | ||
| .toISOString() | ||
| // replace time colons with dashes to make it a valid filename | ||
| .replaceAll(':', '-') | ||
| // remove milliseconds | ||
| .split('.')[0]; | ||
|
|
||
| const snapshots = await loadtest({ | ||
| ...loadtestOpts, | ||
| cwd, | ||
| memorySnapshotWindow, | ||
| idle, | ||
| duration, | ||
| calmdown, | ||
| server, | ||
| async onMemorySnapshot(memoryUsageInMB, phase, snapshots) { | ||
| if (isDebug('memtest')) { | ||
| const chart = createLineChart( | ||
| snapshots.total.map( | ||
| (_, i) => `${i + memorySnapshotWindow / 1000}. sec`, | ||
| ), | ||
| [ | ||
| { | ||
| label: 'Idle', | ||
| data: snapshots.idle, | ||
| }, | ||
| ...(snapshots.loadtest.length | ||
| ? [ | ||
| { | ||
| label: 'Loadtest', | ||
| data: [ | ||
| ...snapshots.idle.map((val, i, arr) => | ||
| i === arr.length - 1 ? val : null, | ||
| ), // skip idle data except for the last point to make a connection in the chart | ||
| ...snapshots.loadtest, | ||
| ], | ||
| }, | ||
| ] | ||
| : []), | ||
| ...(snapshots.calmdown.length | ||
| ? [ | ||
| { | ||
| label: 'Calmdown', | ||
| data: [ | ||
| ...snapshots.idle.map(() => null), // skip idle data | ||
| ...snapshots.loadtest.map((val, i, arr) => | ||
| i === arr.length - 1 ? val : null, | ||
| ), // skip loadtest data except for the last point to make a connection in the chart | ||
| ...snapshots.calmdown, | ||
| ], | ||
| }, | ||
| ] | ||
| : []), | ||
| ], | ||
| { | ||
| yTicksCallback: (tickValue) => `${tickValue} MB`, | ||
| }, | ||
| ); | ||
| await fs.writeFile( | ||
| path.join(cwd, `memtest-memory-snapshots_${startTime}.svg`), | ||
| chart.toBuffer(), | ||
| ); | ||
| } | ||
| return onMemorySnapshot?.(memoryUsageInMB, phase, snapshots); | ||
| }, | ||
| }); | ||
|
|
||
| const idleSlope = calculateRegressionSlope(snapshots.idle); | ||
| debugLog(`server memory idle regression slope: ${idleSlope}`); | ||
| expect | ||
| .soft(idleSlope, 'Memory increase detected while idling') | ||
| .toBeLessThanOrEqual(0); | ||
|
|
||
| const loadtestSlope = calculateRegressionSlope(snapshots.loadtest); | ||
| debugLog(`server memory loadtest regression slope: ${loadtestSlope}`); | ||
| expect | ||
| .soft(loadtestSlope, 'Memory never stopped growing during loadtest') | ||
| .toBeLessThanOrEqual(1); | ||
|
|
||
| const calmdownSlope = calculateRegressionSlope(snapshots.calmdown); | ||
| debugLog(`server memory calmdown regression slope: ${calmdownSlope}`); | ||
| expect | ||
| .soft(calmdownSlope, 'No memory decrease detected during calmdown') | ||
| .toBeLessThanOrEqual(-10); | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider splitting the primary test into smaller blocks
The logic within this single test function is substantial, which can impact readability and maintenance. Splitting it into smaller, descriptive test blocks can help manage complexity.
| import fs from 'fs/promises'; | ||
| import path from 'path'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce "node:" import protocol for built-in modules
Use the node: prefix for built-in modules to align with recommended Node.js import practices.
- import fs from 'fs/promises';
- import path from 'path';
+ import fs from 'node:fs/promises';
+ import path from 'node:path';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import fs from 'fs/promises'; | |
| import path from 'path'; | |
| import fs from 'node:fs/promises'; | |
| import path from 'node:path'; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
| export interface GatewayOptions extends ProcOptions { | ||
| port?: number; | ||
| /** Extra args to pass to the process. */ | ||
| args?: (string | number | boolean)[]; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Flexible args property introduced
Allowing (string | number | boolean)[] is broad. If possible, consider narrowing it or introducing more specific argument handling.
| @@ -163,6 +162,8 @@ | |||
| } | |||
|
|
|||
| export interface ComposeOptions extends ProcOptions { | |||
| /** Extra args to pass to the process. */ | |||
| args?: (string | number | boolean)[]; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Broad argument type for ComposeOptions
Consistency with other interfaces is good. Opportunities remain for typed or validated arguments if your use cases require more precision.
| export async function parseHeap(file: string) { | ||
| await HeapParser.parse(file); | ||
| // TODO: implement and use | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Implement heap parsing functionality.
The function is exported but not fully implemented. The TODO comment suggests that additional functionality is needed.
Would you like me to help implement the heap parsing functionality? I can:
- Parse the heap data into a structured format
- Add memory usage analysis
- Generate a report with memory trends
| { | ||
| "name": "@internal/perf", | ||
| "type": "module", | ||
| "private": true, | ||
| "main": "./src/index.ts", | ||
| "dependencies": { | ||
| "@graphql-tools/delegate": "workspace:^", | ||
| "@memlab/core": "patch:@memlab/core@npm%3A1.1.39#~/.yarn/patches/@memlab-core-npm-1.1.39-34d29dabbd.patch", | ||
| "canvas": "^3.1.0", | ||
| "chart.js": "^4.4.7", | ||
| "chartjs-plugin-trendline": "^2.1.6", | ||
| "memlab": "^1.1.56", | ||
| "parse-duration": "^2.0.0", | ||
| "regression": "^2.0.1", | ||
| "ws": "^8.18.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/k6": "^0.54.2", | ||
| "@types/regression": "^2", | ||
| "@types/ws": "^8.5.12" | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
💡 Verification agent
🧩 Analysis chain
Documentation needed for memory leak detection feature.
Since this PR (GW-154) introduces memory leak detection functionality, documentation should be added to help users understand and utilize this feature.
Let's check if documentation exists in the console project:
Consider creating documentation that covers:
- How to use the memory leak detection feature
- Understanding memory snapshots and heap dumps
- Interpreting test results and visualizations
🏁 Script executed:
#!/bin/bash
# Description: Search for documentation related to memory leak detection in the console project.
gh api graphql -f query='
query {
search(query:"repo:graphql-hive/console memory leak detection", type:ISSUE, first:5) {
nodes {
... on PullRequest {
title
url
state
}
}
}
}'Length of output: 917
Action Required: Add Documentation for Memory Leak Detection Feature
The search query in the console project did not return any documentation related to memory leak detection. Since GW-154 introduces this new functionality, please add user-facing documentation that covers the following aspects:
- Usage: Steps for enabling and using the memory leak detection feature.
- Technical Details: Explanation of memory snapshots, heap dumps, and how the underlying mechanism works.
- Result Interpretation: Guidance on how to interpret test results and visualizations.
Ensure the documentation is added in the appropriate location to help users take full advantage of the new feature.
| "@internal/perf": ["./internal/perf/src/index.ts"], | ||
| "@internal/perf/memtest": ["./internal/perf/src/memtest.ts"], |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Confirm New Path Mappings for Memory Test Integration
The new path mappings for @internal/perf and @internal/perf/memtest are correctly defined to support the memtest functionality. Ensure that any related documentation (e.g., in the console project) is updated to reflect these additions.
| "test:leaks": "cross-env \"LEAK_TEST=1\" jest --detectOpenHandles --detectLeaks", | ||
| "test:mem": "vitest --project memtest" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document the New Memory Testing Script
A new script "test:mem": "vitest --project memtest" has been added to expand testing capabilities. Please ensure that this new memory testing functionality is documented in the console project so that users understand its purpose and usage.
It might be helpful to open a follow-up issue or PR to update the documentation accordingly.
Completes GW-154
TODO