Use metrics unit in user scope #2759
Conversation
| @@ -104,7 +104,7 @@ type ( | |||
| RecordTimer(timer string, d time.Duration) | |||
| // RecordDistribution records a distribution (wrapper on top of timer) for the given | |||
| // metric name | |||
| RecordDistribution(id string, d int) | |||
There was a problem hiding this comment.
we really need to consolidate userScope with internal metric client.
There was a problem hiding this comment.
This will break OSS users existing implementation of custom metrics reporter I think? We need to call it out in the release notes if that's the case.
There was a problem hiding this comment.
Yes. We need to mention in the release note in 1.17
There was a problem hiding this comment.
currently, it is not possible to implement custom metrics reporter. because the people don't have access to internal metrics definition which is needed to implement that metrics.Client which takes a scopeId (int), and the implementation need to translate to string but cannot access those internal definition.
| @@ -323,6 +366,13 @@ func convertPrometheusConfigToTally( | |||
| } | |||
| } | |||
|
|
|||
| func setDefaultPerUnitHistogramBoundaries(clientConfig *ClientConfig) { | |||
| if clientConfig.PerUnitHistogramBoundaries != nil && len(clientConfig.PerUnitHistogramBoundaries) > 0 { | |||
There was a problem hiding this comment.
client config might have only overwrite one unit type
There was a problem hiding this comment.
I think we should keep it backward compatible. So if there is partial customized boundaries, it will use the customized boundaries with defined unit and others fall back to the default boundaries.
There was a problem hiding this comment.
I don't think it will break anything. It just uses different bucket boundaries which should not break any metrics. The new ones in this PR is better than the old default one. For example bytes, the old default have buckets for 0.1 byte.
| 2000000 * ms, | ||
| 5000000 * ms, | ||
| 10000000 * ms, |
There was a problem hiding this comment.
I assume this is recent change after 1.16?
| @@ -104,7 +104,7 @@ type ( | |||
| RecordTimer(timer string, d time.Duration) | |||
| // RecordDistribution records a distribution (wrapper on top of timer) for the given | |||
| // metric name | |||
| RecordDistribution(id string, d int) | |||
There was a problem hiding this comment.
This will break OSS users existing implementation of custom metrics reporter I think? We need to call it out in the release notes if that's the case.
What changed?
Use metrics unit in user scope
Why?
The current user scope uses the default unify bucket and we should allow define metrics unit
How did you test it?
Potential risks
Is hotfix candidate?