wu-sheng commented on PR #13930:
URL: https://github.com/apache/skywalking/pull/13930#issuecomment-4828100323
## Review
**Verdict: approach is correct; 2 blockers to fix before merge** (both are
accidental rebase/edit artifacts unrelated to Node.js metrics), plus a
doc-accuracy nit.
I verified the OAP side against the agent at the pinned commit `36df516`.
### ✅ Core adoption is correct
- **All 6 meter names match exactly** between `nodejs-runtime.yaml` and the
agent's `RuntimeMetricsCollector.toMeterData()`:
`instance_nodejs_{process_cpu,heap_used,heap_total,heap_limit,rss,external_memory}`.
No missing/extra meters.
- **Label model is right.** The agent emits `MeterSingleValue` with no
labels — service/instance ride on the `MeterData` envelope
(`setService`/`setServiceinstance`), and OAP's `SampleBuilder.build()` injects
`service`/`instance` labels from it, so `expSuffix: instance(['service'],
['instance'], Layer.GENERAL)` resolves correctly. Same convention as
`go-runtime.yaml`/`python-runtime.yaml`.
- **`process_cpu` as a gauge passthrough is correct** — the agent
pre-computes the percentage (user+system, normalized by logical CPU count and
elapsed time), so `rate()`/`increase()` would be wrong here.
- **E2E instance-property changes match the new agent**: `hostname`
(`os.hostname()`) added; `Process No.` relaxed `"1"` → `{{ notEmpty .value }}`
(now `process.pid`, no longer guaranteed PID 1).
- Unit test data, the 6 e2e MQE assertions, the asserted `config-dump.yml`
ordering, license headers, single-line changelog bullets, and the README anchor
`#nodejs-runtime-metrics` all check out.
### 🔴 Blocker 1 — `test/e2e-v2/script/env`: duplicate keys silently
downgrade the Go agent
A bad rebase left duplicate entries:
```
SW_AGENT_NODEJS_COMMIT=36df516… # intended
SW_AGENT_GO_COMMIT=19a9fa9… # master's value (go-agent #250)
SW_AGENT_NODEJS_COMMIT=36df516… # DUPLICATE (redundant)
SW_AGENT_GO_COMMIT=afa75a3… # DUPLICATE, different & OLDER (go-agent
#236)
```
When sourced, the last assignment wins, so `SW_AGENT_GO_COMMIT` becomes
`afa75a3` (#236), reverting master's `19a9fa9` (#250) — a silent, unrelated
Go-agent downgrade in e2e.
**Fix:** drop the two duplicate lines; keep
`SW_AGENT_NODEJS_COMMIT=36df516…` once and `SW_AGENT_GO_COMMIT=19a9fa9…` once.
### 🔴 Blocker 2 — `docs/menu.yml`: Telegraf Metrics lost its `path`
Inserting the Node.js entry accidentally deleted the `path:` line above
"Apdex Threshold":
```yaml
- name: "Telegraf Metrics" # path was removed
- name: "Apdex Threshold"
path: "/en/setup/backend/apdex-threshold"
```
`docs/en/setup/backend/telegraf-receiver.md` still exists, so this orphans a
valid page / breaks the menu link.
**Fix:** restore `path: "/en/setup/backend/telegraf-receiver"` under
"Telegraf Metrics".
### 🟡 Minor
- **`dashboards-nodejs-runtime.md` env-var names.** The table leads with
`SW_AGENT_RUNTIME_METRICS_*`, but per the agent (`AgentConfig.ts` precedence +
README) the canonical names are `SW_AGENT_NODEJS_RUNTIME_METRICS_*`;
`SW_AGENT_RUNTIME_METRICS_*` is itself a *deprecated alias*. Lead the table
with the `NODEJS_` names and list `SW_AGENT_RUNTIME_METRICS_*` /
`SW_AGENT_NVM_METRICS_*` / `SW_AGENT_NVM_JVM_*` together as deprecated aliases.
(Works today, but documents a deprecated form.)
- **`docs/en/debugging/config_dump.md`.** The sample inserts
`nodejs-runtime` mid-list; the actual order (application.yml / asserted
`config-dump.yml`) appends it at the end. That sample was already stale
(missing `ruby-runtime`, `php-runtime`). Illustrative only — align or leave,
but it's inconsistent.
- **Cross-repo (informational).** Horizon UI #87 (the widgets) is still open
— the instance dashboard won't render the 6 panels until it merges; worth
coordinating so the doc's "UI location" section isn't dead on arrival.
The `node:12 → node:20` Dockerfile bump is correct and required (the new
agent declares `engines.node >= 20`; node:12 is EOL).
--
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]
