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}
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-tools/batch-delegate |
9.0.32-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/batch-execute |
9.0.13-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/delegate |
10.2.14-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-common |
0.0.4-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-graphql-ws |
2.0.4-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-http |
1.3.0-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/federation |
3.1.5-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/fusion-runtime |
0.11.3-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway |
1.11.0-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/logger-json |
0.0.3-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/logger-winston |
1.0.2-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/nestjs |
1.0.0-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/plugin-aws-sigv4 |
1.0.0-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/hmac-upstream-signature |
1.2.22-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-opentelemetry |
1.3.45-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-prometheus |
1.3.33-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway-runtime |
1.5.0-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/stitch |
9.4.19-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/stitching-directives |
3.1.29-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-common |
0.7.31-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-http |
0.6.35-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-http-callback |
0.5.22-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-ws |
1.0.5-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/wrap |
10.0.32-alpha-2976c121e26c9e8a34ddadf1099e5d0be8e24bad |
npm ↗︎ unpkg ↗︎ |
📝 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
|
|
|
||
| env: | ||
| NODE_NO_WARNINGS: 1 | ||
| CI: true |
There was a problem hiding this comment.
| - 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.
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.
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.
| 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)
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
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: 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.
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
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
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.
3 participants
Completes GW-154
TODO
Fix getting memory stats for BunNot using memory status, but heap sampling profile