Closed wac- closed 2 years ago
I'm willing to accept this as-is, but wanted to ask: what do you think about taking it one step further and make Frequency
a float64
like the other numeric values? Now that it really is a number instead of a number-with-unit, that seems like a more appropriate data type.
I've tried this out and concluded that it makes more sense as-is. Metric.WithLabelValues(...)
requires the label values to be strings, so we'd just wind up with a possibly lossy string -> float64 -> string conversion like:
for ch, d := range s.Downstream {
downstreamSNRMetric.WithLabelValues(string(ch), strconv.FormatFloat(d.Frequency, ...), d.Modulation).Set(d.SNR)
downstreamPowerLevelMetric.WithLabelValues(string(ch), strconv.FormatFloat(d.Frequency, ...), d.Modulation).Set(d.PowerLevel)
codewordsUnerroredMetric.WithLabelValues(string(ch)).Set(d.Unerrored)
codewordsCorrectableMetric.WithLabelValues(string(ch)).Set(d.Correctable)
codewordsUncorrectableMetric.WithLabelValues(string(ch)).Set(d.Uncorrectable)
...with some amount of fiddling around to get FormatFloat
configured to export in a readable way.
Converting the frequency data to a float or integer type seems unnecessary until the frequency itself is exported as a metric.
If that happens, uint32
or even int32
would be safe types since a lot would need to change about DOCSIS deployments for frequencies over 2 GHz to be used. Surely we'll have fiber optics and flying cars by then.
Surely we'll have fiber optics and flying cars by then.
Or at least I'll stop caring about surfer
by then :)
Removing the " Hz" text from the
frequency_hz
label makes it a little easier to order the frequencies in a way that shows the relationship between signal loss and the central frequency of the channel being used. This also makes all the modem modules consistent with one another.Lines now look like:
instead of:
Unit tests are changed to match and passing.
This may break people's pre-existing dashboards if they're manually trimming the " Hz" off the label by blindly shortening the label string. (source: this broke my Grafana dashboard, because I was doing this.)