wu-sheng commented on PR #139:
URL:
https://github.com/apache/skywalking-nodejs/pull/139#issuecomment-4823054572
Thanks for the update. I rechecked the previous lifecycle issues against
`d67e866f44b550d4eacd1a304032ea09ad142ba0`.
The prior blockers are fixed in my local probes:
- Original `agent.destroy()` grpc-js watcher crash no longer reproduces.
- Late heartbeat, trace, and meter callbacks after shutdown no longer crash
when the service registry has been cleared.
- `reportError()` no longer leaves local services stuck in `DISCONNECT`
while the shared channel still reports ready.
- `npm run build` passes.
- `npx jest tests/runtime/RuntimeMetricsCollector.test.ts --runInBand`
passes.
I found one remaining compatibility/cost-control bug in config handling.
## Deprecated programmatic runtime metric options are declared but ignored
The new config type declares deprecated programmatic aliases such as
`nvmMetricsReporterActive`, but `agent.start({ nvmMetricsReporterActive: false
})` does not disable runtime metrics. The canonical field already has a default
value before `finalizeConfig()` runs, so `applyDeprecatedRuntimeMetricConfig()`
never copies the deprecated option.
```ts
// index.ts
// BUG: options are merged into the already-defaulted singleton config.
// runtimeMetricsReporterActive is already true by default at this point.
Object.assign(config, options);
finalizeConfig(config);
```
```ts
// AgentConfig.ts
// BUG: this only copies the deprecated alias when the canonical field is
// undefined. In the real singleton config it is already defaulted, so
// nvmMetricsReporterActive: false is ignored.
function applyDeprecatedRuntimeMetricConfig(config: AgentConfig): void {
if (config.runtimeMetricsReporterActive === undefined) {
if (config.nvmMetricsReporterActive !== undefined) {
config.runtimeMetricsReporterActive = config.nvmMetricsReporterActive;
} else if (config.nvmJvmReporterActive !== undefined) {
config.runtimeMetricsReporterActive = config.nvmJvmReporterActive;
}
}
}
```
I verified this locally:
```text
agent.start({ nvmMetricsReporterActive: false })
runtimeMetricsReporterActive after start true
```
Env aliases are handled earlier in the default config construction, so this
is specifically the programmatic `AgentConfig` path. For an agent, this matters
because runtime metrics create extra timers and collection/report work; a
documented/deprecated disable option should not silently fail.
Suggested Node-side fix: normalize deprecated aliases on the incoming
`options` object before merging it into the default singleton, so explicit user
options win over defaults.
```ts
// Sketch: normalize user-provided options before Object.assign(config,
options).
function normalizeDeprecatedRuntimeMetricOptions(options: AgentConfig):
AgentConfig {
if (options.runtimeMetricsReporterActive === undefined) {
options.runtimeMetricsReporterActive =
options.nvmMetricsReporterActive ?? options.nvmJvmReporterActive;
}
if (options.runtimeMetricsCollectPeriod === undefined) {
options.runtimeMetricsCollectPeriod =
options.nvmMetricsCollectPeriod ?? options.nvmJvmMetricsCollectPeriod;
}
if (options.runtimeMetricsReportPeriod === undefined) {
options.runtimeMetricsReportPeriod =
options.nvmMetricsReportPeriod ?? options.nvmJvmMetricsReportPeriod;
}
if (options.runtimeMetricsBufferSize === undefined) {
options.runtimeMetricsBufferSize =
options.nvmMetricsBufferSize ?? options.nvmJvmMetricsBufferSize;
}
return options;
}
```
Please also add a small config test for this path:
```ts
// Expected behavior: deprecated programmatic disable should still disable
// MeterSender registration unless the canonical option is explicitly set.
agent.start({ nvmMetricsReporterActive: false });
expect(config.runtimeMetricsReporterActive).toBe(false);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
