Append missing _total suffix when registering Counter metrics. #177
Conversation
This ensures consistent metric names across the Prometheus registry, the MetricsContainer and across the JSON and Prometheus exposition formats available at /metrics. Also adds basic logging and fixes docs. See: jitsi/jitsi-videobridge#1942
Fixes JSON output showing up in text/html when querying via browser.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #177 +/- ##
============================================
+ Coverage 22.78% 23.24% +0.45%
- Complexity 122 124 +2
============================================
Files 32 32
Lines 1119 1123 +4
Branches 101 102 +1
============================================
+ Hits 255 261 +6
+ Misses 846 845 -1
+ Partials 18 17 -1
Continue to review full report at Codecov.
|
| @@ -28,6 +30,7 @@ open class MetricsContainer @JvmOverloads constructor( | |||
| /** the registry used to register metrics */ | |||
| val registry: CollectorRegistry = CollectorRegistry.defaultRegistry | |||
| ) { | |||
| private val logger: Logger = LoggerImpl(javaClass.name) | |||
| @@ -110,17 +113,23 @@ open class MetricsContainer @JvmOverloads constructor( | |||
| /** the optional initial value of the metric */ | |||
| initialValue: Long = 0 | |||
| ): CounterMetric { | |||
| if (metrics.containsKey(name)) { | |||
| val newName = when (name.endsWith("_total")) { | |||
There was a problem hiding this comment.
Why use "when" instead of "if"?
There was a problem hiding this comment.
I think I was playing around and forgot to revert it to "if". Should I go ahead and change it to this?
val newName = if (name.endsWith("_total")) name else "${name}_total".also {
logger.warn("Counter '$name' was renamed to '$it' to ensure consistent metric naming.")
}Also, it currently logs whenever a metric is renamed (which currently applies to all of our CounterMetric). I think it's better to log this behavior even though it's the most common, as opposed to logging whenever a metric name is unchanged.
There was a problem hiding this comment.
Agreed. But make it INFO or DEBUG
| logger.warn("Counter '$name' was renamed to '$it' to ensure consistent metric naming.") | ||
| } | ||
| } | ||
| if (metrics.containsKey(newName)) { |
There was a problem hiding this comment.
If there is another metric with "name" the Prometheus lib will throw, right? What do you think about handling that case here, so it's clear that it's expected and the warning message is clear
There was a problem hiding this comment.
Yes, it throws an IllegalArgumentException. So were you thinking of trying to return a newly-registered metric, and catch the exception and deal with it?
There was a problem hiding this comment.
No, I was thinking of catching the case and throwing our own exception. Just a suggestion I'm not sure this is a good idea or worth doing.
* Append missing _total suffix when registering Counter metrics. (jitsi/jicoco#177) * Disable all HTTP TRACE requests. (jitsi/jicoco#179)
* Append missing _total suffix when registering Counter metrics. (jitsi/jicoco#177) * Disable all HTTP TRACE requests. (jitsi/jicoco#179)
As discussed in this jitsi-videobridge PR.