Skip to content

OTLP receiver: Allow colons in non-standard units #15710

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ArthurSens merged 1 commit into from
Dec 23, 2024
+14 −5
Merged

OTLP receiver: Allow colons in non-standard units #15710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

* [CHANGE] Notifier: Increment the prometheus_notifications_errors_total metric by the number of affected alerts rather than by one per batch of affected alerts. #15428
* [ENHANCEMENT] OTLP receiver: Convert also metric metadata. #15416
* [BUGFIX] OTLP receiver: Allow colons in non-standard units. #15710

## 3.0.1 / 2024-11-28

Expand Down
8 changes: 4 additions & 4 deletions storage/remote/otlptranslator/prometheus/normalize_name.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"strings"
"unicode"

"github.com/prometheus/prometheus/util/strutil"
"go.opentelemetry.io/collector/pdata/pmetric"
)

Expand Down Expand Up @@ -120,18 +119,19 @@ func BuildCompliantName(metric pmetric.Metric, namespace string, addMetricSuffix
return metricName
}

var nonMetricNameCharRE = regexp.MustCompile(`[^a-zA-Z0-9:]`)
Copy link
Copy Markdown
Member

@bboreham bboreham Dec 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@aknuds1 aknuds1 Dec 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip @bboreham.

Copy link
Copy Markdown
Contributor Author

@aknuds1 aknuds1 Dec 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the changes, and am benchmarking to see if an effect can be seen.

Copy link
Copy Markdown
Contributor Author

@aknuds1 aknuds1 Dec 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened a PR with your suggested optimization @bboreham - simple, but effective.


// Build a normalized name for the specified metric.
func normalizeName(metric pmetric.Metric, namespace string, allowUTF8 bool) string {
var nameTokens []string
var separators []string
if !allowUTF8 {
nonTokenMetricCharRE := regexp.MustCompile(`[^a-zA-Z0-9:]`)
// Split metric name into "tokens" (of supported metric name runes).
// Note that this has the side effect of replacing multiple consecutive underscores with a single underscore.
// This is part of the OTel to Prometheus specification: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.38.0/specification/compatibility/prometheus_and_openmetrics.md#otlp-metric-points-to-prometheus.
nameTokens = strings.FieldsFunc(
metric.Name(),
func(r rune) bool { return nonTokenMetricCharRE.MatchString(string(r)) },
func(r rune) bool { return nonMetricNameCharRE.MatchString(string(r)) },
)
} else {
translationFunc := func(r rune) bool { return !unicode.IsLetter(r) && !unicode.IsDigit(r) && r != ':' }
Expand Down Expand Up @@ -229,7 +229,7 @@ func cleanUpUnit(unit string) string {
// This is part of the OTel to Prometheus specification: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.38.0/specification/compatibility/prometheus_and_openmetrics.md#otlp-metric-points-to-prometheus.
multipleUnderscoresRE := regexp.MustCompile(`__+`)
return strings.TrimPrefix(multipleUnderscoresRE.ReplaceAllString(
strutil.SanitizeLabelName(unit),
nonMetricNameCharRE.ReplaceAllString(unit, "_"),
"_",
), "_")
}
Expand Down
10 changes: 9 additions & 1 deletion storage/remote/otlptranslator/prometheus/normalize_name_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ func TestWhiteSpaces(t *testing.T) {

func TestNonStandardUnit(t *testing.T) {
require.Equal(t, "system_network_dropped", normalizeName(createGauge("system.network.dropped", "{packets}"), "", false))
// The normal metric name character set is allowed in non-standard units.
require.Equal(t, "system_network_dropped_nonstandard:_1", normalizeName(createGauge("system.network.dropped", "nonstandard:_1"), "", false))
}

func TestNonStandardUnitCounter(t *testing.T) {
Expand Down Expand Up @@ -68,6 +70,12 @@ func TestHertz(t *testing.T) {
func TestPer(t *testing.T) {
require.Equal(t, "broken_metric_speed_km_per_hour", normalizeName(createGauge("broken.metric.speed", "km/h"), "", false))
require.Equal(t, "astro_light_speed_limit_meters_per_second", normalizeName(createGauge("astro.light.speed_limit", "m/s"), "", false))
// The normal metric name character set is allowed in non-standard units.
require.Equal(t, "system_network_dropped_non_per_standard:_1", normalizeName(createGauge("system.network.dropped", "non/standard:_1"), "", false))

t.Run("invalid per unit", func(t *testing.T) {
require.Equal(t, "broken_metric_speed_km", normalizeName(createGauge("broken.metric.speed", "km/°"), "", false))
})
}

func TestPercent(t *testing.T) {
Expand All @@ -89,7 +97,7 @@ func TestAllowUTF8(t *testing.T) {
})
t.Run("disallow UTF8", func(t *testing.T) {
require.Equal(t, "unsupported_metric_temperature_F", normalizeName(createGauge("unsupported.metric.temperature", "°F"), "", false))
require.Equal(t, "unsupported_metric_weird", normalizeName(createGauge("unsupported.metric.weird", "+=.:,!* & #"), "", false))
require.Equal(t, "unsupported_metric_weird", normalizeName(createGauge("unsupported.metric.weird", "+=.,!* & #"), "", false))
require.Equal(t, "unsupported_metric_redundant_test_per_C", normalizeName(createGauge("unsupported.metric.redundant", "__test $/°C"), "", false))
require.Equal(t, "metric_with_foreign_characters", normalizeName(createGauge("metric_with_字符_foreign_characters", "ど"), "", false))
})
Expand Down