metalmatze / transmission-exporter

Prometheus exporter for Transmission metrics, written in Go.
MIT License
99 stars 40 forks source link

Status should not be a gauge? #19

Open yegle opened 4 years ago

yegle commented 4 years ago

I'm new to Prometheus so my understanding might be wrong. I don't think the status should be a gauge? Code that I'm referring to (note the conversion from t.Status to float):

https://github.com/metalmatze/transmission-exporter/blob/df361375117a6ebbcb5186e55df94e4d0fcddcb6/cmd/transmission-exporter/torrent_collector.go#L144-L149

The status field in the Transmission rpc spec: https://github.com/transmission/transmission/blob/46b3e6c8dae02531b1eb8907b51611fb9229b54a/extras/rpc-spec.txt#L221

Reading transmission.h, it looks like this is an enum that corresponding to https://github.com/transmission/transmission/blob/46b3e6c8dae02531b1eb8907b51611fb9229b54a/libtransmission/transmission.h#L1814-L1824

So if my reading is correct, what needs to be changed is: The descriptor for the torrent status metric should have a status field which is the string representation of the tr_torrent_activity enum, and the value for the gauge should be 1.

        Status: prometheus.NewDesc(
            namespace+collectorNamespace+"status",
            "Status of a torrent",
            []string{"id", "name", "status"},
            nil,
        ch <- prometheus.MustNewConstMetric(
            tc.Status,
            prometheus.GaugeValue,
            1,
            id, t.Name, enumToString(t.Status)
        )

Does it make sense? If yes I can probably send a PR to fix it.

metalmatze commented 4 years ago

It makes sense both ways I'd say. I don't really have a strong opinion on this very metric. However, keep in mind that the current way it is implemented results in n time series for n statuses whereas the approach you propose results in 7*n. It might make a bigger difference for people having a lot of torrents. As the number of statuses is limited to 7, I'd still be fine with changing it to that.