PROM-39: Add type and unit labels to OTLP endpoint #16630

ArthurSens · 2025-05-25T12:33:53Z

Implements the addition of __type__ and __unit__ labels when receiving OTLP metrics.

aknuds1

I think some refactoring would be of help (same as Copilot suggests). Also, shouldn't there be a flag for enabling this new behaviour?

storage/remote/otlptranslator/prometheusremotewrite/helper.go

Copilot

Pull Request Overview

This PR implements support for adding type and unit labels to OTLP metrics. Key changes include the addition of a typeAndUnitLabels flag in both the test cases and the OTLP write handler, updates to various metric conversion functions to emit these new labels, and corresponding test updates.

Introduces a new flag (typeAndUnitLabels) to conditionally add type and unit labels.
Updates test cases and converter functions to include the new labels when enabled.
Adjusts helper functions and metadata handling for consistent label creation.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
storage/remote/write_test.go	Updated test cases to include typeAndUnitLabels flag and expected label values.
storage/remote/write_handler.go	Added new OTLPOptions flag and propagated it to handler configuration.
storage/remote/otlptranslator/prometheusremotewrite/number_data_points.go	Modified gauge and sum data point functions to append type and unit labels.
storage/remote/otlptranslator/prometheusremotewrite/metrics_to_prw.go	Updated metric metadata usage for consistent label injection.
storage/remote/otlptranslator/prometheusremotewrite/histograms.go & helper.go	Updated histogram conversion functions and introduced helper addTypeAndUnitLabels for label creation.

storage/remote/otlptranslator/prometheusremotewrite/number_data_points.go

ArthurSens · 2025-06-06T13:34:30Z

shouldn't there be a flag for enabling this new behaviour?

It is supposed to be behind the feature gate added in #16228. Your question made me realize that I haven't updated main.go to pass this information to the OTLP HTTP handler though, let me take a look at that

ywwg · 2025-06-10T17:27:35Z

@carrieedwards FYI this might be of interest

Copilot

Pull Request Overview

This PR implements the addition of type and unit labels to incoming OTLP metrics. Key changes include propagating a new flag (AddTypeAndUnitLabels) through the Options, API, and OTLP write handler, updating the conversion functions to attach the new labels, and augmenting test cases to cover scenarios both with and without type/unit label injection.

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
web/web.go	Added new flag AddTypeAndUnitLabels to Options and passed it along during handler construction.
web/api/v1/api.go	Propagated the new boolean flag to the OTLP write handler initialization in the API setup.
storage/remote/write_handler.go	Extended OTLPOptions and rwExporter to include the type and unit labels flag.
storage/remote/write_test.go	Updated tests to include the new typeAndUnitLabels parameter and expected label changes.
Various otlptranslator files	Updated conversion logic to use new metadata format and attach type and unit labels when configured.
cmd/prometheus/main.go	Enabled the experimental flag for type and unit labels via command-line configuration.

Copilot · 2025-06-11T16:18:35Z

storage/remote/otlptranslator/prometheusremotewrite/helper.go

+	labels = append(labels, prompb.Label{Name: "__type__", Value: strings.ToLower(metadata.Type.String())})
+	labels = append(labels, prompb.Label{Name: "__unit__", Value: metadata.Unit})


Consider extracting the label key strings "type" and "unit" into constants to ensure consistency and ease future maintenance.

Suggested change

labels = append(labels, prompb.Label{Name: "__type__", Value: strings.ToLower(metadata.Type.String())})

labels = append(labels, prompb.Label{Name: "__unit__", Value: metadata.Unit})

labels = append(labels, prompb.Label{Name: typeLabelKey, Value: strings.ToLower(metadata.Type.String())})

labels = append(labels, prompb.Label{Name: unitLabelKey, Value: metadata.Unit})

I agree with Copilot that it would be nice to have constants for these, analogously to how we have a __name__ constant.

I'd like to do so once a new version of Common is released with this patch

Copilot · 2025-06-11T16:18:36Z

storage/remote/otlptranslator/prometheusremotewrite/helper.go


+// addTypeAndUnitLabels appends type and unit labels to the given labels slice if the setting is enabled.
+func addTypeAndUnitLabels(labels []prompb.Label, metadata prompb.MetricMetadata) []prompb.Label {
+	labels = append(labels, prompb.Label{Name: "__type__", Value: strings.ToLower(metadata.Type.String())})


Consider checking whether a label with the name "type" (and similarly "unit") already exists in the labels slice before appending, to avoid potential duplication.

I think this copilot comment is worth looking into - is there a possibility that the user adds a __type__ label to their series and then this additional __type__ label is also added?

Hmmm interesting, I just tried doing that myself and got a big error on my face "Invalid label for series. Duplicated label: xxxxx". This is coming from the prwv1 appender.

Thanks for paying extra attention here

I think we should be overwriting the type and unit labels if they do exist, to be consistent with the proposal:

Generally clients should never expose type and unit labels directly as it's a special label starting with __. However, it can happen by accident or intentionally to support ingestion methods without metadata fields (e.g. PRW 1.0). That's why any existing user provided labels for __unit__ and __type__ should be overridden by the existing metadata mechanisms in current exposition and ingestion formats, otherwise we keep them on.

aknuds1

Please see comments.

storage/remote/otlptranslator/prometheusremotewrite/helper.go

aknuds1 · 2025-06-11T16:32:55Z

storage/remote/otlptranslator/prometheusremotewrite/helper.go

+	labels = append(labels, prompb.Label{Name: "__type__", Value: strings.ToLower(metadata.Type.String())})
+	labels = append(labels, prompb.Label{Name: "__unit__", Value: metadata.Unit})


I agree with Copilot that it would be nice to have constants for these, analogously to how we have a __name__ constant.

ArthurSens · 2025-06-11T16:51:46Z

I won't be able to work on this until June 25th 😬

@carrieedwards, @dashpole, @bwplotka -- If you happen to have the time, feel free to continue! If not, I'll do it once I'm back :)

dashpole · 2025-06-18T15:35:31Z

I'm going to try and pick this up

dashpole · 2025-06-24T00:27:55Z

Superseded by #16770

ArthurSens · 2025-07-02T17:48:54Z

After a few discussions with David and the rest of the OTel-Prometheus working group, I'm reopening this PR. Migrating the ingestion path from RWv1 to RWv2 is not as beneficial as expected.

fionaliao

one comment around possible duplicate metadata labels, otherwise looks good

fionaliao · 2025-07-10T18:31:10Z

storage/remote/otlptranslator/prometheusremotewrite/helper.go


+// addTypeAndUnitLabels appends type and unit labels to the given labels slice if the setting is enabled.
+func addTypeAndUnitLabels(labels []prompb.Label, metadata prompb.MetricMetadata) []prompb.Label {
+	labels = append(labels, prompb.Label{Name: "__type__", Value: strings.ToLower(metadata.Type.String())})


I think this copilot comment is worth looking into - is there a possibility that the user adds a __type__ label to their series and then this additional __type__ label is also added?

ArthurSens · 2025-07-17T18:35:49Z

The conflicts should go away once #16878 is merged

fionaliao

LGTM

Signed-off-by: Arthur Silva Sens <[email protected]>

Co-authored-by: Arve Knudsen <[email protected]> Signed-off-by: Arthur Silva Sens <[email protected]>

Signed-off-by: Arthur Silva Sens <[email protected]>

Copilot

Pull Request Overview

This PR implements the addition of __type__ and __unit__ labels to OTLP metrics when receiving OTLP data. The feature is controlled by a new configuration option that can be enabled via the "type-and-unit-labels" feature flag, which affects both scraping and OTLP ingestion.

Adds a new AddTypeAndUnitLabels configuration option that flows through the web handler to the OTLP translator
Implements label addition functionality that appends metric type and unit information as special labels
Updates all affected function signatures to pass metadata instead of just metric names to support the new labeling feature

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
web/web.go	Adds `AddTypeAndUnitLabels` option to web handler configuration
web/api/v1/api.go	Updates API constructor to accept and pass through the new configuration parameter
web/api/v1/errors_test.go	Updates test to provide required parameter for API constructor
storage/remote/write_handler.go	Implements OTLP handler configuration and passes setting to translator
storage/remote/write_test.go	Adds comprehensive test cases for both enabled and disabled type/unit label scenarios
storage/remote/otlptranslator/prometheusremotewrite/metrics_to_prw.go	Updates translator to use metadata objects and conditionally add type/unit labels
storage/remote/otlptranslator/prometheusremotewrite/number_data_points.go	Modifies number data point handling to support new labeling feature
storage/remote/otlptranslator/prometheusremotewrite/histograms.go	Updates histogram handling for new labeling functionality
storage/remote/otlptranslator/prometheusremotewrite/helper.go	Implements core `addTypeAndUnitLabels` function and updates related functions
storage/remote/otlptranslator/prometheusremotewrite/helper_test.go	Adds unit tests for the new label addition functionality
cmd/prometheus/main.go	Enables the feature for both scraping and OTLP when feature flag is set

Comments suppressed due to low confidence (1)

storage/remote/write_test.go:454

The expected metric name has changed from "test_counter_total" to "test_counter_bytes_total" but there's no test case to verify the unit is properly included in the metric name when type and unit labels are disabled. Consider adding a test case that explicitly verifies unit inclusion in metric names.

					l: labels.New(labels.Label{Name: "__name__", Value: "test_counter_bytes_total"},

storage/remote/otlptranslator/prometheusremotewrite/helper.go

aknuds1

Mainly I think there should be consistent terminology for this feature between scraping and remote write subsystems. Also, I would like to avoid the code duplication introduced by this PR.

storage/remote/otlptranslator/prometheusremotewrite/metrics_to_prw.go

storage/remote/otlptranslator/prometheusremotewrite/helper.go

storage/remote/write_handler.go

storage/remote/write_test.go

web/web.go

carrieedwards

LGTM!

Signed-off-by: Arthur Silva Sens <[email protected]>

ArthurSens · 2025-07-25T11:53:08Z

Comments resolved, so I'm merging!

* PROM-39: Add type and unit labels to OTLP endpoint Signed-off-by: Arthur Silva Sens <[email protected]> * Extract label addition into helper function Signed-off-by: Arthur Silva Sens <[email protected]> * Wire feature flag and web handler configuration Signed-off-by: Arthur Silva Sens <[email protected]> * Apply suggestions from code review Co-authored-by: Arve Knudsen <[email protected]> Signed-off-by: Arthur Silva Sens <[email protected]> * Use lowercase for units too Signed-off-by: Arthur Silva Sens <[email protected]> * Use otlptranslator.UnitNamer to build units Signed-off-by: Arthur Silva Sens <[email protected]> * Address copilot's comment Signed-off-by: Arthur Silva Sens <[email protected]> * Verify label presence before adding them Signed-off-by: Arthur Silva Sens <[email protected]> * Overwrite type/unit labels when already set Signed-off-by: Arthur Silva Sens <[email protected]> * sed/addTypeAndUnitLabels/enableTypeAndUnitLabels/ Signed-off-by: Arthur Silva Sens <[email protected]> * Reduce duplicated code Signed-off-by: Arthur Silva Sens <[email protected]> --------- Signed-off-by: Arthur Silva Sens <[email protected]> Co-authored-by: Arve Knudsen <[email protected]> Signed-off-by: Andrew Hall <[email protected]>

ArthurSens requested review from aknuds1, bwplotka, cstyan, jesusvazquez and tomwilkie as code owners May 25, 2025 12:33

aknuds1 requested a review from Copilot May 26, 2025 15:17

This comment was marked as outdated.

Sign in to view

aknuds1 reviewed Jun 4, 2025

View reviewed changes

storage/remote/otlptranslator/prometheusremotewrite/helper.go Outdated Show resolved Hide resolved

ArthurSens mentioned this pull request Jun 6, 2025

[meta] PROM-39 type-and-unit-labels stability #16610

Open

8 tasks

ArthurSens force-pushed the type-unit-otlp-endpoint branch from 47f49b0 to fed77d1 Compare June 6, 2025 13:30

aknuds1 requested a review from Copilot June 6, 2025 13:31

Copilot AI reviewed Jun 6, 2025

View reviewed changes

storage/remote/otlptranslator/prometheusremotewrite/number_data_points.go Outdated Show resolved Hide resolved

storage/remote/otlptranslator/prometheusremotewrite/number_data_points.go Outdated Show resolved Hide resolved

ArthurSens force-pushed the type-unit-otlp-endpoint branch 2 times, most recently from 11ff522 to 1758891 Compare June 6, 2025 13:36

aknuds1 requested a review from Copilot June 6, 2025 13:36

This comment was marked as outdated.

Sign in to view

aknuds1 requested a review from Copilot June 6, 2025 13:40

This comment was marked as outdated.

Sign in to view

aknuds1 requested a review from Copilot June 11, 2025 16:17

Copilot AI reviewed Jun 11, 2025

View reviewed changes

aknuds1 self-requested a review June 11, 2025 16:19

aknuds1 reviewed Jun 11, 2025

View reviewed changes

dashpole mentioned this pull request Jun 23, 2025

PROM-39: [new] Add type and unit labels to OTLP endpoint #16770

Closed

dashpole closed this Jun 24, 2025

This comment was marked as outdated.

Sign in to view

fionaliao reviewed Jul 10, 2025

View reviewed changes

ArthurSens requested a review from fionaliao July 10, 2025 21:59

fionaliao approved these changes Jul 18, 2025

View reviewed changes

ArthurSens and others added 9 commits July 18, 2025 13:47

PROM-39: Add type and unit labels to OTLP endpoint

daa149b

Signed-off-by: Arthur Silva Sens <[email protected]>

Extract label addition into helper function

4fc4171

Signed-off-by: Arthur Silva Sens <[email protected]>

Wire feature flag and web handler configuration

94aef44

Signed-off-by: Arthur Silva Sens <[email protected]>

Apply suggestions from code review

9856899

Co-authored-by: Arve Knudsen <[email protected]> Signed-off-by: Arthur Silva Sens <[email protected]>

Use lowercase for units too

bb3d23c

Signed-off-by: Arthur Silva Sens <[email protected]>

Use otlptranslator.UnitNamer to build units

119ac1c

Signed-off-by: Arthur Silva Sens <[email protected]>

Address copilot's comment

07e5219

Signed-off-by: Arthur Silva Sens <[email protected]>

Verify label presence before adding them

a13468a

Signed-off-by: Arthur Silva Sens <[email protected]>

Overwrite type/unit labels when already set

b83b0bc

Signed-off-by: Arthur Silva Sens <[email protected]>

ArthurSens force-pushed the type-unit-otlp-endpoint branch from 11e806e to b83b0bc Compare July 18, 2025 16:48

aknuds1 requested a review from Copilot July 21, 2025 13:15

Copilot AI reviewed Jul 21, 2025

View reviewed changes

storage/remote/otlptranslator/prometheusremotewrite/helper.go Show resolved Hide resolved

aknuds1 reviewed Jul 21, 2025

View reviewed changes

carrieedwards approved these changes Jul 21, 2025

View reviewed changes

ArthurSens force-pushed the type-unit-otlp-endpoint branch from f235f68 to 9d04e0a Compare July 21, 2025 22:47

ArthurSens added 2 commits July 21, 2025 19:55

sed/addTypeAndUnitLabels/enableTypeAndUnitLabels/

f7cd7ce

Signed-off-by: Arthur Silva Sens <[email protected]>

Reduce duplicated code

05925a5

Signed-off-by: Arthur Silva Sens <[email protected]>

ArthurSens force-pushed the type-unit-otlp-endpoint branch from 9d04e0a to 05925a5 Compare July 21, 2025 22:56

ArthurSens merged commit 2c04f2d into main Jul 25, 2025
45 checks passed

ArthurSens deleted the type-unit-otlp-endpoint branch July 25, 2025 11:53

This was referenced Aug 7, 2025

Support Prometheus type and unit labels in OTLP receiver cortexproject/cortex#6952

Closed

Support type and unit labels in Thanos Engine thanos-io/promql-engine#613

Open

krajorama mentioned this pull request Aug 12, 2025

OTLP to directly write to an interface which can hide storage details #16951

Merged

		labels = append(labels, prompb.Label{Name: "__type__", Value: strings.ToLower(metadata.Type.String())})
		labels = append(labels, prompb.Label{Name: "__unit__", Value: metadata.Unit})

PROM-39: Add type and unit labels to OTLP endpoint #16630

PROM-39: Add type and unit labels to OTLP endpoint #16630

Uh oh!

Conversation

ArthurSens commented May 25, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

aknuds1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

ArthurSens commented Jun 6, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

ywwg commented Jun 10, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aknuds1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArthurSens commented Jun 11, 2025

Uh oh!

dashpole commented Jun 18, 2025

Uh oh!

dashpole commented Jun 24, 2025

Uh oh!

ArthurSens commented Jul 2, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

fionaliao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArthurSens commented Jul 17, 2025

Uh oh!

fionaliao left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

aknuds1 left a comment •

edited

Loading