Handle prometheus names and labels consistently #6049
Conversation
There was a problem hiding this comment.
I'm thinking if we should fix this starting from 1.13.x or just in main (starting from 1.15.x).
The behavior is changing (well, it's a bug) for those who use PrometheusNamingConvention directly, but otherwise the change should be transparent for others except the error message.
| + " set of tag keys. There is already an existing meter named '" + id.getName() | ||
| + " set of tag keys. There is already an existing meter named '" + getConventionName(id) | ||
| + "' containing tag keys [" | ||
| + String.join(", ", collectorMap.get(getConventionName(id)).getTagKeys()) | ||
| + "]. The meter you are attempting to register" + " has keys [" | ||
| + getConventionTags(id).stream().map(Tag::getKey).collect(joining(", ")) + "]."); | ||
| + "]. The meter you are attempting to register" + " has keys [" + String.join(", ", tagKeys) | ||
| + "]."); |
There was a problem hiding this comment.
This makes the message to use _, previously it was using .; with the 0.x client we are using them mixed (name uses ., tags use _).
So this will be something like:
[...] There is already an existing meter named 'my_counter' containing tag keys [test_k1]. The meter you are attempting to register has keys [test_k2]."
| @@ -85,7 +85,7 @@ else if (!conventionName.endsWith("_seconds")) { | |||
| */ | |||
| @Override | |||
| public String tagKey(String key) { | |||
| return PrometheusNaming.sanitizeLabelName(key); | |||
| return PrometheusNaming.sanitizeLabelName(PrometheusNaming.prometheusName(key)); | |||
There was a problem hiding this comment.
This is the change that makes label names consistent to metric names inside of the registry and for users who directly want to use this NamingConvention.
There was a problem hiding this comment.
I wonder why we need to do this since we're already calling the Prometheus library's sanitizeLabelName. I would expect that to do any needed transformation of a label name. I wonder if its current behavior is intentional or not. It seems possible that there could be subtle intended differences in the rules about metric names vs labels, so using both here could be problematic in the future if it isn't already.
There was a problem hiding this comment.
For the record: we asked @fstab, for now we should do prometheusName + sanitizeXyz.
844200b to
91e567d
Compare
Closes gh-5923