-
Notifications
You must be signed in to change notification settings - Fork 5
feat: UnitNamer API #35
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.
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
Conversation
Which provides an API to translate units from OTel conventions to Prometheus conventions Signed-off-by: Arthur Silva Sens <[email protected]>
Signed-off-by: Arthur Silva Sens <[email protected]>
Some extra information: In the foreseeable future, I can also see a need for such an API once client_golang supports the exposure of Unit metadata (See old PR prometheus/client_golang#1392 that added such a feature but was rejected due to lack of use-cases). While it was rejected in the past, OTel-GO SDK and OTel Collector's Prometheus exporter will only be able to expose unit metadata once client_golang adds support for it. Second extra information: To make sure we have consistent APIs with MetricNamer and UnitNamer, I've updated our test to make sure that whatever is built from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces the UnitNamer API to assist in building compliant unit names separate from metric names when constructing metadata for Prometheus Remote Write exporters. Key changes include:
- Adding a new helper struct (UnitNamer) with its Build method in unit_namer.go.
- Refactoring unit mapping and cleanup functions out of metric_namer.go into unit_namer.go.
- Expanding test coverage in metric_namer_test.go to verify the correlation between metric names and unit names when suffixes are enabled.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
unit_namer.go | New file implementing UnitNamer and associated unit-processing functions. |
metric_namer_test.go | Updated tests to validate both MetricNamer and UnitNamer functionality. |
metric_namer.go | Removed redundant unit-processing functions, consolidating logic in UnitNamer. |
Comments suppressed due to low confidence (1)
unit_namer.go:106
- Ensure that the variable multipleUnderscoresRE is properly defined or imported within the package so that the cleanUpUnit function works as intended without runtime errors.
return strings.TrimPrefix(multipleUnderscoresRE.ReplaceAllString(
heads up @ywwg, this PR is blocking adoption in otel-collector. No need to rush here, but I'd like to proceed at some point 😇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great!
@@ -138,7 +138,8 @@ func TestMetricNamer_Build(t *testing.T) { | |||
Unit: "", | |||
Type: MetricTypeGauge, | |||
}, | |||
expected: "test@namespace_metric", // TODO: should be "test_namespace_metric" | |||
expectedMetricName: "test@namespace_metric", // TODO: should be "test_namespace_metric" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did we file an issue for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think we have!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR adds the helper struct
UnitNamer
, which follows fundamentals similar toMetricNamer
andLabelNamer
.When building metadata information, for example:
The unit information is sent separately from the metric name, and that's when this API will be useful.