feat: add process info diagnostic for cpu, memory, and system metrics#10204
feat: add process info diagnostic for cpu, memory, and system metrics#10204GiladShoham wants to merge 3 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new getProcessInfo diagnostic function to report detailed process and system metrics alongside the existing getBitVersion diagnostic. The function collects process uptime, PID, memory usage, CPU usage, and system information (total/free memory, CPU count, load average, hostname). Comprehensive unit tests are included to validate the new functionality.
Changes:
- Added
getProcessInfostatic method toDiagnosticMainclass that collects process and system metrics - Registered
getProcessInfoalongsidegetBitVersionin the diagnostic slot - Updated diagnostic route verb from READ to WRITE and added explicit return statement
- Disabled GraphQL diagnostic resolver (returns empty object instead of diagnostic data)
- Added comprehensive unit tests for both
getBitVersionandgetProcessInfomethods
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scopes/harmony/diagnostic/diagnostic.spec.ts | New test file with comprehensive tests for getBitVersion and getProcessInfo functions |
| scopes/harmony/diagnostic/diagnostic.main.runtime.ts | Added getProcessInfo method and registered it in the provider |
| scopes/harmony/diagnostic/diagnostic.route.ts | Changed verb to WRITE and added explicit return to res.json |
| scopes/harmony/diagnostic/diagnostic.graphql.ts | Commented out diagnostic data retrieval, now returns empty object |
| method = 'GET'; | ||
| route = '/_diagnostic'; | ||
| verb = Verb.READ; | ||
| verb = Verb.WRITE; |
There was a problem hiding this comment.
The verb should be Verb.READ instead of Verb.WRITE. The /_diagnostic endpoint only reads diagnostic data without modifying any state. Based on the codebase patterns:
Verb.READis used for GET requests that query data without side effects (e.g., fetch routes, search routes, query routes)Verb.WRITEis used for POST requests that modify state (e.g., create, delete, put routes)
This is a GET request that only reads diagnostic information, so it should use Verb.READ.
| verb = Verb.WRITE; | |
| verb = Verb.READ; |
| // return this.diagnosticMain.getDiagnosticData(); | ||
| return {}; |
There was a problem hiding this comment.
The GraphQL resolver for _diagnostic has been commented out and now returns an empty object. This breaks the GraphQL API functionality. If the intention is to disable this temporarily, it should either be removed completely or have a clear explanation/TODO comment. If this is intentional and permanent, the schema and resolver should be removed entirely.
This change is not mentioned in the PR description and appears to be unrelated to adding the getProcessInfo diagnostic function.
| export type { DocumentNode } from 'graphql'; | ||
| export type { SchemaDirectives } from '@graphql-modules/core'; |
There was a problem hiding this comment.
schema.ts now both imports DocumentNode/SchemaDirectives for local use and re-exports them from their original modules. This duplicates module references and can be simplified by exporting the already-imported types (or by using import('...').TypeName in the Schema type and keeping only the re-export). Simplifying keeps the file clearer and avoids redundant statements.
| export type { DocumentNode } from 'graphql'; | |
| export type { SchemaDirectives } from '@graphql-modules/core'; | |
| export type { DocumentNode, SchemaDirectives }; |
| it('should return process uptime as a positive number', () => { | ||
| const result = DiagnosticMain.getProcessInfo(); | ||
| expect(result.uptime).to.be.a('number'); | ||
| expect(result.uptime).to.be.greaterThan(0); | ||
| }); |
There was a problem hiding this comment.
Some assertions in these unit tests may be brittle across environments (e.g. process.uptime() can be extremely close to 0 early in the process lifetime; some memory fields can be 0 depending on runtime/Node/V8 behavior). To reduce flakiness, consider asserting >= 0 for uptime/memory and focusing on shape/type guarantees rather than strict positivity.
| it('should return free memory less than or equal to total memory', () => { | ||
| const result = DiagnosticMain.getProcessInfo(); | ||
| expect(result.system.freeMemory).to.be.at.most(result.system.totalMemory); | ||
| expect(result.system.freeMemory).to.be.greaterThan(0); |
There was a problem hiding this comment.
This test asserts freeMemory is > 0. In some constrained/containerized environments it may be possible to observe 0 (or near-0) free memory, which would make the test flaky even though the implementation is fine. Consider asserting >= 0 and <= totalMemory only.
| expect(result.system.freeMemory).to.be.greaterThan(0); | |
| expect(result.system.freeMemory).to.be.at.least(0); |
| expect(result.memory).to.have.property('heapTotal').that.is.a('number'); | ||
| expect(result.memory).to.have.property('heapUsed').that.is.a('number'); | ||
| expect(result.memory).to.have.property('external').that.is.a('number'); | ||
| expect(result.memory).to.have.property('arrayBuffers').that.is.a('number'); |
There was a problem hiding this comment.
This test expects memory.arrayBuffers to exist and be a number. Since the repo supports Node >=12.22.0, and process.memoryUsage() may not provide arrayBuffers on older Node versions, the test can fail even if the diagnostic is otherwise correct. Consider making the assertion conditional (only when the field exists) or treating it as optional.
| expect(result.memory).to.have.property('arrayBuffers').that.is.a('number'); | |
| if ('arrayBuffers' in result.memory && result.memory.arrayBuffers !== undefined) { | |
| expect(result.memory.arrayBuffers).to.be.a('number'); | |
| } |
| static getProcessInfo() { | ||
| const memUsage = process.memoryUsage(); | ||
| const cpuUsage = process.cpuUsage(); | ||
| return { | ||
| uptime: process.uptime(), | ||
| pid: process.pid, | ||
| nodeVersion: process.version, | ||
| platform: process.platform, | ||
| arch: process.arch, | ||
| memory: { | ||
| rss: memUsage.rss, | ||
| heapTotal: memUsage.heapTotal, | ||
| heapUsed: memUsage.heapUsed, | ||
| external: memUsage.external, | ||
| arrayBuffers: memUsage.arrayBuffers, | ||
| }, | ||
| cpu: { | ||
| user: cpuUsage.user, | ||
| system: cpuUsage.system, | ||
| }, |
There was a problem hiding this comment.
getProcessInfo() exposes CPU usage as the raw output of process.cpuUsage() (user/system in microseconds). Without units in the field names, this is easy to misinterpret as percentages or milliseconds. Consider either converting to a clearer unit (e.g. milliseconds) or renaming/annotating the fields to include the unit so downstream consumers don’t misread the numbers.
| expect(result.memory.rss).to.be.greaterThan(0); | ||
| expect(result.memory.heapTotal).to.be.greaterThan(0); | ||
| expect(result.memory.heapUsed).to.be.greaterThan(0); |
There was a problem hiding this comment.
These tests assume several memory fields are strictly > 0 (rss, heapTotal, heapUsed). Depending on Node/V8 and the runtime environment, some of these can be 0 (especially in minimal/embedded environments), which can cause unnecessary flakes. Prefer >= 0 (or only assert presence/type) unless strict positivity is required by the contract.
| expect(result.memory.rss).to.be.greaterThan(0); | |
| expect(result.memory.heapTotal).to.be.greaterThan(0); | |
| expect(result.memory.heapUsed).to.be.greaterThan(0); | |
| expect(result.memory.rss).to.be.at.least(0); | |
| expect(result.memory.heapTotal).to.be.at.least(0); | |
| expect(result.memory.heapUsed).to.be.at.least(0); |
| heapTotal: memUsage.heapTotal, | ||
| heapUsed: memUsage.heapUsed, | ||
| external: memUsage.external, | ||
| arrayBuffers: memUsage.arrayBuffers, |
There was a problem hiding this comment.
process.memoryUsage().arrayBuffers is not available on all supported Node versions. This repo’s root package.json allows Node >=12.22.0, and on Node 12 memoryUsage() may not include arrayBuffers, making this field undefined at runtime. Consider making arrayBuffers optional/conditional (only include when present) or provide a safe fallback to avoid runtime incompatibility.
| arrayBuffers: memUsage.arrayBuffers, | |
| ...(typeof (memUsage as any).arrayBuffers === 'number' | |
| ? { arrayBuffers: (memUsage as any).arrayBuffers } | |
| : {}), |
Add
getProcessInfodiagnostic that reports process uptime, pid, memory usage, CPU usage, and system info (total/free memory, CPU count, load average, hostname). Registered alongsidegetBitVersionin the diagnostic slot. Includes unit tests.