[reciever/prometheusremotewritereciever] handle otel_scope_name and otel_scope_version in prw reciever #37791

Vandit1604 · 2025-02-08T12:51:13Z

This PR ensures that when Prometheus samples lack the otel_scope_name or otel_scope_version labels, the receiver fills in default values. If otel_scope_name is missing, it defaults to opentelemetry-collector. If otel_scope_version is missing, it uses the collector’s build version. This change makes sure every metric has a complete instrumentation scope as required.

more here: Metrics which do not have an otel_scope_name or otel_scope_version label MUST be assigned an instrumentation scope identifying the entity performing the translation from Prometheus to OpenTelemetry

Vandit1604 · 2025-02-08T12:53:27Z

Should I also handle otel_scope_info in this?

Labels on otel_scope_info metric points other than otel_scope_name and otel_scope_version, MUST be added as scope attributes to the scope with the matching name and version.

found this in the OTLP spec here

dashpole · 2025-02-10T18:52:19Z

Should I also handle otel_scope_info in this?

Not right now. That spec needs to be updated because scope attributes were made identifying

ArthurSens

Thanks for starting the work here :)

We'll also need tests to verify that we're not breaking anything. Can you add one test to ensure that two time series without scope labels end up together and with the correct scope attributes?

ArthurSens · 2025-02-10T20:15:03Z

receiver/prometheusremotewritereceiver/receiver.go

 	// More: https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/#:~:text=Metrics%20which%20do%20not%20have%20an%20otel_scope_name%20or%20otel_scope_version%20label%20MUST%20be%20assigned%20an%20instrumentation%20scope%20identifying%20the%20entity%20performing%20the%20translation%20from%20Prometheus%20to%20OpenTelemetry%20(e.g.%20the%20collector%E2%80%99s%20prometheus%20receiver)
+	if scopeName == "" {
+		scopeName = "opentelemetry-collector"


Can't we take the scope name from settings, just like you did with the version?

something like this ?

scopeName = prw.settings.BuildInfo.Command

Um, how about adding scopedetail func? Which could fetch data from collector setting

I think Vandit needs a bit more detail about what a scopedetails func is. Looking at your PR #37871, I like the idea, but let's explain why this is good. :)

This is also unrelated to this specific thread, so let's start a new review for this idea

something like this ?

scopeName = prw.settings.BuildInfo.Command

When reading the doc string for settings.BuildInfo.Description, I see:

Description is the full name of the collector, e.g. "OpenTelemetry Collector"

So I think the description is the most appropriate one.

Understood thanks :)

ArthurSens · 2025-02-10T20:15:49Z

receiver/prometheusremotewritereceiver/receiver.go

@@ -230,15 +234,24 @@ func addCounterDatapoints(_ pmetric.ResourceMetrics, _ labels.Labels, _ writev2.
 	// TODO: Implement this function
 }

+var defaultBuildVersion string = "unknown"


I'm not sure if we want to do this. Have you seen this requirement somewhere?

Not specifically, I added this as a fallback in case settings.BuildInfo.Version is empty or unavailable. Is this not the correct approach? Happy to adjust if there's a better way to handle it.

ArthurSens

I think we're over engineering a bit with the fallbacks, there's nothing in the spec asking for this so we can just use empty strings.

ArthurSens · 2025-02-12T14:49:23Z

receiver/prometheusremotewritereceiver/receiver.go

 	// More: https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/#:~:text=Metrics%20which%20do%20not%20have%20an%20otel_scope_name%20or%20otel_scope_version%20label%20MUST%20be%20assigned%20an%20instrumentation%20scope%20identifying%20the%20entity%20performing%20the%20translation%20from%20Prometheus%20to%20OpenTelemetry%20(e.g.%20the%20collector%E2%80%99s%20prometheus%20receiver)
+	if scopeName == "" {
+		scopeName = "opentelemetry-collector"


I think Vandit needs a bit more detail about what a scopedetails func is. Looking at your PR #37871, I like the idea, but let's explain why this is good. :)

This is also unrelated to this specific thread, so let's start a new review for this idea

ArthurSens · 2025-02-12T14:55:43Z

receiver/prometheusremotewritereceiver/receiver.go

 	// More: https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/#:~:text=Metrics%20which%20do%20not%20have%20an%20otel_scope_name%20or%20otel_scope_version%20label%20MUST%20be%20assigned%20an%20instrumentation%20scope%20identifying%20the%20entity%20performing%20the%20translation%20from%20Prometheus%20to%20OpenTelemetry%20(e.g.%20the%20collector%E2%80%99s%20prometheus%20receiver)
+	if scopeName == "" {
+		scopeName = "opentelemetry-collector"


something like this ?

scopeName = prw.settings.BuildInfo.Command

When reading the doc string for settings.BuildInfo.Description, I see:

Description is the full name of the collector, e.g. "OpenTelemetry Collector"

So I think the description is the most appropriate one.

Vandit1604 · 2025-02-12T17:43:50Z

Hi @ArthurSens I have added tests and changelog entry as well.

Please review in your own time.
Thank you!

perebaj · 2025-02-12T23:43:50Z

receiver/prometheusremotewritereceiver/receiver.go

+var (
+	buildVersion string = ""
+	buildName    string = ""
+)


Maybe, for better readability, consider moving these variables to the top of the file, closer to where they are used

According to this comment, we don't even need to set those vars. So, ignore my suggestion 🙃

perebaj · 2025-02-13T00:00:12Z

receiver/prometheusremotewritereceiver/receiver_test.go

+			expectedStats: remote.WriteResponseStats{},
+		},
+		{
+			name: "missing otel_scope_name/version falls back to buildName/buildVersion",


Aren't both otel_scope_name/version falls back to buildName/buildVersion and valid request testing the same scenario? In both cases, we’re not passing otel_scope_name/version and expect the values to default to buildName/buildVersion. Instead, we could create a test case where we explicitly set settings.BuildInfo.Description/Version and validate whether the values are correctly assigned.

perebaj · 2025-02-13T00:03:26Z

receiver/prometheusremotewritereceiver/receiver.go

+	if settings.BuildInfo.Version != "" {
+		buildVersion = settings.BuildInfo.Version
+	}
+
+	if settings.BuildInfo.Description != "" {
+		buildName = settings.BuildInfo.Description
+	}
+
 	return &prometheusRemoteWriteReceiver{


Regardless of whether settings.BuildInfo.Version or settings.BuildInfo.Description are empty, we should always assign their values to buildName and version. The determining factor for whether we use them or not will be whether scope_name or version are empty. I think that we can simplify the ifs here.

Suggested change

if settings.BuildInfo.Version != "" {

buildVersion = settings.BuildInfo.Version

}

if settings.BuildInfo.Description != "" {

buildName = settings.BuildInfo.Description

}

return &prometheusRemoteWriteReceiver{

buildVersion = settings.BuildInfo.Version

buildName = settings.BuildInfo.Description

return &prometheusRemoteWriteReceiver{

True :)

We don't even need the extra variables, actually. We can update the function like this:

func (prwr * prometheusRemoteWriteReceiver) addGaugeDatapoints(..... { scopeName := prwr.settings.BuildInfo.Description scopeVersion := prwr.settings.BuildInfo.Version }

If the settings are empty, the scope variables will be empty. If settings had proper content, then the scope will be populated correctly

ArthurSens · 2025-02-13T12:22:51Z

Could someone with permissions trigger tests here? Maybe @dashpole :)

Vandit1604 · 2025-02-13T16:56:07Z

Updated the test with the case custom BuildInfo used when no scope provided to address @perebaj 's comment. Also updated the code to only override with collector build details if ls.Get() returns an empty string.

perebaj · 2025-02-16T22:36:05Z

receiver/prometheusremotewritereceiver/receiver_test.go

+				m1 := sm1.Metrics().AppendEmpty().SetEmptyGauge()
+				dp1 := m1.DataPoints().AppendEmpty()
+				dp1.Attributes().PutStr("d", "e")
+				dp1.Attributes().PutStr("foo", "bar")


do you have any reason to change sm1Attributes := sm1.Metrics().AppendEmpty().SetEmptyGauge().DataPoints().AppendEmpty().Attributes() ->

m1 := sm1.Metrics().AppendEmpty().SetEmptyGauge()
dp1 := m1.DataPoints().AppendEmpty()
dp1.Attributes().PutStr(...)
dp1.Attributes().PutStr(...)

?

Seems that the previous implementation it's ok

receiver/prometheusremotewritereceiver/receiver_test.go

perebaj

Some suggestions ✌🏽

perebaj · 2025-02-17T14:33:06Z

receiver/prometheusremotewritereceiver/receiver_test.go

-				sm.Scope().SetName("")
-				sm.Scope().SetVersion("")
+				sm.Scope().SetName(defaultBuildName)
+				sm.Scope().SetVersion(defaultBuildVersion)


Did using the empty string instead of defaultBuildName/Version cause the test to fail?

Hey @Vandit1604 I think that is only thing that is missing before we merge it 🙃

And some conflicts related to this PR that was merged recently

ArthurSens

Thank you for being so diligent with the tests, I love it!

One extra comment besides the ones below: Could we keep all the scope-related tests together? What I mean is to move the test duplicated scope name and version further below, after valid request

ArthurSens · 2025-02-21T20:45:47Z

receiver/prometheusremotewritereceiver/receiver_test.go

+const (
+	defaultBuildName    = "defaultBuildName"
+	defaultBuildVersion = "defaultBuildVersion"
+)


Could we remove this?

ArthurSens · 2025-02-21T20:53:45Z

receiver/prometheusremotewritereceiver/receiver.go

 			addCounterDatapoints(rm, ls, ts)
 		case writev2.Metadata_METRIC_TYPE_GAUGE:
-			addGaugeDatapoints(rm, ls, ts)
+			prw.addGaugeDatapoints(rm, ls, ts)
 		case writev2.Metadata_METRIC_TYPE_SUMMARY:
 			addSummaryDatapoints(rm, ls, ts)


Could we do the same for all other as well? I know they are not implemented yet, but we'll definitely do the same thing when implementing them.

What I mean that we have something like this:

switch ts.Metadata.Type { case writev2.Metadata_METRIC_TYPE_COUNTER: --> prw.addCounterDatapoints(rm, ls, ts) case writev2.Metadata_METRIC_TYPE_GAUGE: prw.addGaugeDatapoints(rm, ls, ts) case writev2.Metadata_METRIC_TYPE_SUMMARY: --> prw.addSummaryDatapoints(rm, ls, ts) case writev2.Metadata_METRIC_TYPE_HISTOGRAM: --> prw.addHistogramDatapoints(rm, ls, ts) default: badRequestErrors = errors.Join(badRequestErrors, fmt.Errorf("unsupported metric type %q for metric %q", ts.Metadata.Type, ls.Get(labels.MetricName))) }```

ArthurSens · 2025-02-21T20:55:40Z

receiver/prometheusremotewritereceiver/receiver_test.go

+				// For job "service-x/test", no explicit otel_scope is provided.
+				// fall back to BuildInfo defaults.


Actually I believe we don't want to fallback to anything. There's nothing in the specification saying that 🤔

ArthurSens · 2025-02-21T20:58:19Z

receiver/prometheusremotewritereceiver/receiver_test.go

-				rmAttributes1.PutStr("service.namespace", "service-x")
-				rmAttributes1.PutStr("service.name", "test")
-				rmAttributes1.PutStr("service.instance.id", "107cn001")
-				sm1 := rm1.ScopeMetrics().AppendEmpty()
+				rm := expected.ResourceMetrics().AppendEmpty()
+				parseJobAndInstance(rm.Resource().Attributes(), "service-x/test", "107cn001")


hmmmm I see what you wanted to do here; it makes the code a lot easier to read, BUT! then we're not testing the function parseJobAndInstance() anymore.

If we accidentally break this function in the future, if we call it during the preparation of our expected result, we will never notice the difference.

Could we keep manually setting those attributes?

ArthurSens · 2025-02-21T20:58:31Z

receiver/prometheusremotewritereceiver/receiver_test.go

-				rmAttributes1 := rm1.Resource().Attributes()
-				rmAttributes1.PutStr("service.namespace", "service-x")
-				rmAttributes1.PutStr("service.name", "test")
-				rmAttributes1.PutStr("service.instance.id", "107cn001")
+				parseJobAndInstance(rm1.Resource().Attributes(), "service-x/test", "107cn001")


same thing here

ArthurSens · 2025-02-21T21:01:16Z

receiver/prometheusremotewritereceiver/receiver_test.go

 				sm2Attributes := sm1.Metrics().AppendEmpty().SetEmptyGauge().DataPoints().AppendEmpty().Attributes()
 				sm2Attributes.PutStr("d", "e")
 				sm2Attributes.PutStr("foo", "bar")

+				// For job "foo" with instance "bar", fallback to BuildInfo.


No need for a fallback

ArthurSens · 2025-02-21T21:02:20Z

receiver/prometheusremotewritereceiver/receiver_test.go

+			expectedStats: remote.WriteResponseStats{},
+		},
+		{
+			name: "provided otel_scope_name and otel_scope_version",


I'm not sure if I'm missing something, but I think this test case is covered by the test case duplicated scope name and version, isn't it?

If that correct, could we remove this test case?

ArthurSens · 2025-02-21T21:03:23Z

receiver/prometheusremotewritereceiver/receiver_test.go

+			expectedStats: remote.WriteResponseStats{},
+		},
+		{
+			name: "missing otel_scope_name/version falls back to BuildInfo",


We don't need a fallback :)

But we could certify that all metrics with a missing scope would end up within the same scope with no attributes.

ArthurSens · 2025-02-21T21:04:23Z

receiver/prometheusremotewritereceiver/receiver_test.go

+			buildNameOverride:    "customBuildName",
+			buildVersionOverride: "customBuildVersion",


I don't really understand where this is coming from 🤔 could you explain?

ArthurSens · 2025-02-21T21:04:50Z

receiver/prometheusremotewritereceiver/receiver_test.go

+		buildNameOverride    string
+		buildVersionOverride string


What is this being used for? I don't really understand

Signed-off-by: Vandit Singh <vanditsinghkv@gmail.com>

Co-authored-by: Jonathan <perebaj@gmail.com>

Signed-off-by: Vandit Singh <vanditsinghkv@gmail.com>

Vandit1604 · 2025-02-24T12:12:40Z

I rebased the PR and did following changes

Using settings.BuildInfo to get collector's build details
The updated the tests accordingly.

perebaj · 2025-03-06T22:25:44Z

Hey @Vandit1604 many thanks for your effort here, but this PR is a blocker for us. So we decided to go forward in a simpler way. But the others addXDataPoints should be implement using the extractScopeInfo approach

Vandit1604 · 2025-03-08T16:15:53Z

@perebaj Should I close this PR then?

Vandit1604 requested review from dashpole and a team as code owners February 8, 2025 12:51

github-actions bot assigned andrzej-stencel Feb 8, 2025

github-actions bot added the receiver/prometheusremotewrite label Feb 8, 2025

github-actions bot requested a review from ArthurSens February 8, 2025 12:51

ArthurSens reviewed Feb 10, 2025

View reviewed changes

PiyushRajDev mentioned this pull request Feb 11, 2025

Prometheus Remote-Write receiver Alpha #37277

Open

9 tasks

ArthurSens mentioned this pull request Feb 12, 2025

[reciever/prometheusremotewritereciever] handle otel_scope_name and otel_scope_version #37871

Closed

ArthurSens reviewed Feb 12, 2025

View reviewed changes

perebaj reviewed Feb 12, 2025

View reviewed changes

perebaj reviewed Feb 13, 2025

View reviewed changes

perebaj reviewed Feb 16, 2025

View reviewed changes

receiver/prometheusremotewritereceiver/receiver_test.go Outdated Show resolved Hide resolved

perebaj reviewed Feb 16, 2025

View reviewed changes

receiver/prometheusremotewritereceiver/receiver_test.go Outdated Show resolved Hide resolved

perebaj reviewed Feb 16, 2025

View reviewed changes

receiver/prometheusremotewritereceiver/receiver_test.go Outdated Show resolved Hide resolved

perebaj reviewed Feb 16, 2025

View reviewed changes

receiver/prometheusremotewritereceiver/receiver_test.go Outdated Show resolved Hide resolved

perebaj reviewed Feb 16, 2025

View reviewed changes

receiver/prometheusremotewritereceiver/receiver_test.go Outdated Show resolved Hide resolved

perebaj reviewed Feb 16, 2025

View reviewed changes

perebaj reviewed Feb 17, 2025

View reviewed changes

ArthurSens reviewed Feb 21, 2025

View reviewed changes

Vandit1604 added 4 commits February 23, 2025 21:25

handle otel_scope_name and otel_scope_version in prw reciever

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

897960a

Signed-off-by: Vandit Singh <vanditsinghkv@gmail.com>

remove the fallbacks and use description as otel_scope_name

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

1eea9f4

Signed-off-by: Vandit Singh <vanditsinghkv@gmail.com>

fix up test

12ebb4b

Signed-off-by: Vandit Singh <vanditsinghkv@gmail.com>

add tests

e1501a9

Signed-off-by: Vandit Singh <vanditsinghkv@gmail.com>

Vandit1604 and others added 4 commits February 23, 2025 21:38

implement suggestions and update test

cf56fb3

Signed-off-by: Vandit Singh <vanditsinghkv@gmail.com>

Apply suggestions from code review

0ef01dd

Co-authored-by: Jonathan <perebaj@gmail.com>

fix failing tests

27a5565

Signed-off-by: Vandit Singh <vanditsinghkv@gmail.com>

rebase and simplify tests

Loading
Loading status checks…

a70e789

Signed-off-by: Vandit Singh <vanditsinghkv@gmail.com>

Vandit1604 force-pushed the handle-prw-otel_scope_name-and-version branch from 01615f6 to a70e789 Compare February 23, 2025 17:02

Vandit1604 added 2 commits February 23, 2025 22:33

methods should belong to prw

Loading
Loading status checks…

5d974bd

Signed-off-by: Vandit Singh <vanditsinghkv@gmail.com>

group otel scope related tests

Loading
Loading status checks…

0d27779

Signed-off-by: Vandit Singh <vanditsinghkv@gmail.com>

Vandit1604 closed this Mar 15, 2025

		// For job "service-x/test", no explicit otel_scope is provided.
		// fall back to BuildInfo defaults.

		buildNameOverride: "customBuildName",
		buildVersionOverride: "customBuildVersion",

[reciever/prometheusremotewritereciever] handle otel_scope_name and otel_scope_version in prw reciever #37791

[reciever/prometheusremotewritereciever] handle otel_scope_name and otel_scope_version in prw reciever #37791

Conversation

Vandit1604 commented Feb 8, 2025 • edited Loading

Vandit1604 commented Feb 8, 2025

dashpole commented Feb 10, 2025

ArthurSens left a comment • edited Loading

Choose a reason for hiding this comment

ArthurSens Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArthurSens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vandit1604 commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perebaj Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

perebaj Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArthurSens commented Feb 13, 2025

Vandit1604 commented Feb 13, 2025

Choose a reason for hiding this comment

perebaj left a comment

Choose a reason for hiding this comment

perebaj Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArthurSens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vandit1604 commented Feb 24, 2025

perebaj commented Mar 6, 2025

Vandit1604 commented Mar 8, 2025

Vandit1604 commented Feb 8, 2025 •

edited

Loading

ArthurSens left a comment •

edited

Loading

ArthurSens Feb 10, 2025 •

edited

Loading

perebaj Feb 13, 2025 •

edited

Loading

perebaj Feb 13, 2025 •

edited

Loading

perebaj Feb 17, 2025 •

edited

Loading