OTLP receiver: Allow colons in non-standard units #15710
Conversation
aknuds1
force-pushed
the
arve/otel-units
branch
from
6470609 to
c83c45f
Compare
Signed-off-by: Arve Knudsen <[email protected]>
aknuds1
force-pushed
the
arve/otel-units
branch
from
c83c45f to
475b7ff
Compare
|
For another PR: Have we tried using something different than a regex to match allowed characters? Could something like this be more efficient? if r >= 'a' || r <= 'z' || r >= 'A' || r <= 'Z' || r >= '0' || r <= '9' || r == ':' { .... |
|
@ArthurSens I just know that Prometheus uses regexps to validate metric/label names. I suppose you could eventually show through a benchmark that it would be significantly faster through some alternative implementation. |
| @@ -120,18 +119,19 @@ func BuildCompliantName(metric pmetric.Metric, namespace string, addMetricSuffix | |||
| return metricName | |||
| } | |||
|
|
|||
| var nonMetricNameCharRE = regexp.MustCompile(`[^a-zA-Z0-9:]`) | |||
There was a problem hiding this comment.
Noting in passing: this (pulling the initialization out to start of program) should also be done for the other two regexp.MustCompile in this file. Check that relevant benchmarks exist, since they ought to be showing a performance hit.
There was a problem hiding this comment.
Thanks for the tip @bboreham.
There was a problem hiding this comment.
I've made the changes, and am benchmarking to see if an effect can be seen.
Where? Not here. |
@bboreham nice find, I probably forgot that @ArthurSens I guess the fact that |
True. #11936 added |
Will do! Hopefully next week |
Modify the classical mode OTel metric name translation logic so colons are allowed in non-standard units. On inspecting the code, I noticed that a mistake had been made previously in not allowing this character (i.e. the label name character set was applied, not the metric name character set).
Corresponding tests are added, plus one for the case where the OTel "per" unit is completely invalid.