feat: metrics instrumentation via typed EventEmitter - #1751
Conversation
|
This appears very targeted for Prometheus use (it is also not a no-op). We should also be very careful about possible performance degradation (be it small but widespread, or very large). Since this is "extra", it should have a very minimal impact overall. |
|
Hi @Nerivec! Thank you for your feedback - I mostly just opened this PR to see if you were open to this and get feedback on the direction to go.
I tried to make it as non-prometheus specific as I could - e.g. not encoding counter, gauge or histogram specifics, but instead specific events to measure. I'll admit I'm only familiar with Prometheus and OpenTelemetry, but if you have any other suggestions please let me know.
I tried to make the calling sites as simple as possible (e.g. not having each one check if the metrics received is null but instead of having a no-op receiver) - but I could change that? WDYT? I could also change the instrumentation of the adapter so that it only gets added if there is a metrics callback?
100%. By "the usual ZH events API" do you mean e.g. making the Metrics interface follow the same pattern as ControllerEventMap and emit? |
| requestQueueDuration: [data: RequestQueueDurationPayload]; | ||
| } | ||
|
|
||
| export const metrics = new EventEmitter<MetricsEventMap>(); |
There was a problem hiding this comment.
Use of this in e.g. Z2M would require to import {metrics} from "zigbee-herdsman";, i.e. entirely separate from Controller (main entrypoint). I think that's fine for this purpose though.
Note: need to be sure we can easily add a mock in Z2M tests (similar to Controller), no matter the design we end up with.
There was a problem hiding this comment.
Should also identify available adapter's protocol-specific metrics.
I'm not sure if any other than ember currently have any, but currently we have a 3rd party tool to parse this from logs. Would be much better integrated with metrics directly.
|
Thanks for the review feedback! I'll work through it and integrate the changes into Koenkk/zigbee2mqtt#31645. I've got a build of the HA add on for testing in my local setup too, and need to build some dashboards etc.. so probably a few more days before I've got it working end to end. |
94c8e6d to
ee1dc06
Compare
ee1dc06 to
4de1f05
Compare
8258c2a to
b6bdeea
Compare
|
I've got this all working end to end now, used in a PR in z2m (Koenkk/zigbee2mqtt#31645), built a add on container, deployed at home and built a nice set of dashboards in Grafana:
I think it could be ready for proper review? I've done a manual review myself. The thing to flag is none of the send metrics were working - intercepting send calls at the controller didn't work as the seems to be 20+ sites that call the adapater itself. I've tried to make the most minimal change, but putting the instrumentation in the base call, but would be keen on you thoughts. Thanks! |
b6bdeea to
3bcdc1c
Compare
…metrics Publishes a Zigbee2MQTT build carrying two unmerged upstream PRs — the Prometheus exporter extension (Koenkk/zigbee2mqtt#31645) and the zigbee-herdsman metrics instrumentation it consumes (Koenkk/zigbee-herdsman#1751). Both fork branches are rebased onto the current stable releases (zigbee2mqtt 2.13.0, zigbee-herdsman v10.8.0), with herdsman pinned by commit rather than by branch so image builds are reproducible. The add-on definition and image build previously lived on the zigbee2mqtt fork branch as a "DO NOT COMMIT" commit; they move here, which leaves the upstream PR clean. Structure mirrors the official zigbee2mqtt/hassio-zigbee2mqtt repository, whose docs and artwork this derives from (Apache-2.0). First image: tomwilkie/zigbee2mqtt-prometheus-amd64:2.13.0-dd6a6b19 Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>
3bcdc1c to
244a2b2
Compare
Instrument adapter send operations and the controller to emit a single discriminated `metric` event from the base Adapter class, exposing send counts, retries and durations for observability. Adds src/utils/metrics.ts with the MetricType definitions and full test coverage across the ember, z-stack and zoh adapters, controller and device flows. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>
244a2b2 to
5305a82
Compare

Summary
Metricsinterface /setMetrics/noopMetricspattern with a typedEventEmitter<MetricsEventMap>singleton exported asmetricssendZclFrameToEndpoint,sendZdo,sendZclFrameToGroup,sendZclFrameToAll) directly in the controller via a sharedinstrumentSendhelper, with timing and success/failure statusenqueuedAt) stored directly onRequestalongsideexpires, replacing a parallelWeakMaponRequestQueuemetrics.on("adapterSendZclUnicast", (data) => ...)etc. — metrics are always emitted unconditionallyTest plan
npm test— all 1744 tests passmetrics.on(...)events and verify they fire on adapter sends🤖 Generated with Claude Code