fix broken internal metrics names and make them optional #203
Conversation
| @@ -227,7 +231,7 @@ func (r *scopeRegistry) removeWithRLock(subscopeBucket *scopeBucket, key string) | |||
|
|
|||
| // Records internal Metrics' cardinalities. | |||
| func (r *scopeRegistry) reportInternalMetrics() { | |||
| if r.skipInternalMetrics { | |||
| if r.internalMetricsOption != SendInternalMetrics { | |||
There was a problem hiding this comment.
we can eventually set a default option if it's unset
scope_registry.go
Outdated
| counterCardinalityName = "tally.internal.counter-cardinality" | ||
| gaugeCardinalityName = "tally.internal.gauge-cardinality" | ||
| histogramCardinalityName = "tally.internal.histogram-cardinality" | ||
| counterCardinalityName = "tally-internal-counter-cardinality" |
There was a problem hiding this comment.
The fact that this can cause a panic is not great. We should handle an invalid metric names gracefully. We could add sanitazation to the code where the metric is being registered. Though it probably makes sense to have it as a separate PR.
There was a problem hiding this comment.
created #204 to track this request
scope_registry.go
Outdated
| counterCardinalityName = "tally.internal.counter-cardinality" | ||
| gaugeCardinalityName = "tally.internal.gauge-cardinality" | ||
| histogramCardinalityName = "tally.internal.histogram-cardinality" | ||
| counterCardinalityName = "tally-internal-counter-cardinality" |
There was a problem hiding this comment.
I don't think either dots or dashes are accepted by prometheus reporter. At least they not legal characters according to Prom data model
There was a problem hiding this comment.
will change to underscore
scope_registry.go
Outdated
| gaugeCardinalityName = "tally.internal.gauge-cardinality" | ||
| histogramCardinalityName = "tally.internal.histogram-cardinality" | ||
| counterCardinalityName = "tally-internal-counter-cardinality" | ||
| gaugeCardinalityName = "tally-internal-gauge-cardinality" |
There was a problem hiding this comment.
can we please have a test for that?
funny enough we have a comment in Prom reporter example about dashes and dots
https://github.com/uber-go/tally/blob/master/prometheus/example/prometheus_main.go#L37
There was a problem hiding this comment.
I'll have the scope's sanitizer process it during runtime instead. This way we can be agnostic about whatever scope/reporter/sanitizer is underneath.
* improved internal metrics config setup * internal metric names are sanitized at runtime
* improved internal metrics config setup * internal metric names are sanitized at runtime
This addresses #198