receiver/prometheus/internal: add createNodeAndResourcePdata for Prometheus->OTLP Pdata #3139

odeke-em · 2021-05-11T01:38:03Z

Starts the progressive effort to convert directly from
Prometheus->OTLPPdata

directly instead of

Prometheus->OpenCensusProto->OTLPPdata

by adding a converter for node+resource -> pdata.Resource.

Updates #3137

odeke-em · 2021-05-11T01:41:01Z

Kindly cc-ing @alolita @Aneurysm9 @rakyll @bogdandrutu; there is a long chain of PRs coming to progressively make equivalent converters and have full tests and then when ready just switch over entirely when we have complete converters.

alolita · 2021-05-11T14:57:12Z

@anuraaga @Aneurysm9 @rakyll can you please review?

receiver/prometheusreceiver/internal/prom_to_otlp_test.go

bogdandrutu · 2021-05-11T18:48:29Z

receiver/prometheusreceiver/internal/prom_to_otlp_test.go

+	if diff := cmp.Diff(byDirectOTLPResource, fromOCResource, protocmp.Transform()); diff != "" {
+		t.Fatalf("Resource mismatch: got: - want: +\n%s", diff)
+	}


You have sort on the AttributeMap, so you can sort both resources. Also we try to avoid using cmp.Diff and encourage github.com/stretchr/testify to be used.

I don't think we need this comparison test, just write assertions on the PData as you normally would, verifying its structure. The OC code is going to get deleted at which point these tests will need to be rewritten anyways, we should do it now. I believe all the current caveats in the PR would go away if doing so.

bogdandrutu · 2021-05-11T19:01:04Z

receiver/prometheusreceiver/internal/prom_to_otlp_test.go

+func TestCreateNodeAndResourceConversion(t *testing.T) {
+	job, instance, scheme := "converter", "ocmetrics", "http"
+	ocNode, ocResource := createNodeAndResource(job, instance, scheme)
+	mdFromOC := internaldata.OCToMetrics(internaldata.MetricsData{


Please avoid using deprecated methods, and deprecated OC format even in tests

Well I am doing a straight up translation of what exists in the code and I need to produce the exact same result that the current code produces. This PR converts from Prometheus->OTLP instead of Prometheus->OpenCensus->OTLP, and once the entire translation is done and we are sure that we have an absolute 1-to-1 mapping, these tests will be discarded.

Perhaps the test can be renamed in a way that makes it clear it is only intended to validate congruence with the outgoing implementation and will need to be removed.

Sounds good @Aneurysm9, I've renamed it to TestCreateNodeAndResourceEquivalence

anuraaga · 2021-05-12T04:00:27Z

receiver/prometheusreceiver/internal/prom_to_otlp.go

+	}
+	resource := pdata.NewResource()
+	attrs := resource.Attributes()
+	attrs.UpsertString(conventions.AttributeServiceName, job)


@alolita Does anyone own the spec for prometheus mapping of the standard conventions? These seem somewhat arbitrary. If they can be excluded, I'd only add the prometheus specific attributes for now

@anuraaga this is a straight up translation of what already exists in the current converter.

I know - it doesn't block this PR but someone needs to be working on that.

Maybe we need to open an issue to track this.

Maybe we need to open an issue to track this.

Why upsert the job to service.name? Shouldn't it be better to check if service.name has been set?

anuraaga · 2021-05-12T04:03:30Z

receiver/prometheusreceiver/internal/prom_to_otlp_test.go

+	if diff := cmp.Diff(byDirectOTLPResource, fromOCResource, protocmp.Transform()); diff != "" {
+		t.Fatalf("Resource mismatch: got: - want: +\n%s", diff)
+	}


I don't think we need this comparison test, just write assertions on the PData as you normally would, verifying its structure. The OC code is going to get deleted at which point these tests will need to be rewritten anyways, we should do it now. I believe all the current caveats in the PR would go away if doing so.

odeke-em · 2021-05-14T02:00:37Z

@bogdandrutu @tigrannajaryan kind ping!

anuraaga · 2021-05-14T03:19:23Z

My comment about adding normal unit tests, not just parity ones, seems to still not be addressed

odeke-em · 2021-05-14T03:34:47Z

My comment about adding normal unit tests, not just parity ones, seems to still not be addressed

@anuraaga thanks, I hadn't seen your comment, but I've just answered by saying that for the very beginning let's focus on full equivalence which is currently the uphill task.

anuraaga · 2021-05-14T03:54:26Z

@odeke-em Having the parity tests is fine - but this is new code. As with any new code, we add test cases and assertions for them. We'll have to do it anyways to remove the opencensus APIs which we really want to do, so we should do it now.

bogdandrutu · 2021-05-14T21:09:55Z

@odeke-em Having the parity tests is fine - but this is new code. As with any new code, we add test cases and assertions for them. We'll have to do it anyways to remove the opencensus APIs which we really want to do, so we should do it now.

I completely agree, also the parity tests will be removed when we switch to the new code so we will be left without unit-tests.

alolita · 2021-05-14T23:17:51Z

@odeke-em can you please address @anuraaga and @bogdandrutu for adding tests cases and assertions so that this PR can be merged. ty!

The main blocking change was fixed

odeke-em · 2021-05-18T08:07:36Z

I completely agree, also the parity tests will be removed when we switch to the new code so we will be left without unit-tests.

Sure, I am going to add them but I predict that the amount of review time is going to be increase even more, which was the entire point of firstly mailing parity tests, then later on unit tests.

bogdandrutu · 2021-05-19T18:47:56Z

Waiting for the author to add unit-tests.

github-actions · 2021-05-27T04:16:20Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

…etheus->OTLP Pdata Starts the progressive effort to convert directly from Prometheus->OTLPPdata directly instead of Prometheus->OpenCensusProto->OTLPPdata by adding a converter for node+resource -> pdata.Resource. Updates #3137

Addresses feedback pointed out by Bogdan, Tigran, Jaana by using Resource.Attributes().Sort() instead and also avoid using go-cmp/cmp and instead opt to using require.Equal.

odeke-em · 2021-05-27T08:28:15Z

@bogdandrutu @anuraaga please take a look.

odeke-em · 2021-06-02T10:00:07Z

Kind ping @bogdandrutu..

odeke-em requested a review from a team May 11, 2021 01:38

odeke-em force-pushed the receiver-prometheus-createNodeAndResource branch from bbe1e0b to b949055 Compare May 11, 2021 01:39

jmacd approved these changes May 11, 2021

View reviewed changes

alolita added the area:prometheus label May 11, 2021

rakyll approved these changes May 11, 2021

View reviewed changes

receiver/prometheusreceiver/internal/prom_to_otlp_test.go Outdated Show resolved Hide resolved

tigrannajaryan previously requested changes May 11, 2021

View reviewed changes

receiver/prometheusreceiver/internal/prom_to_otlp_test.go Outdated Show resolved Hide resolved

bogdandrutu reviewed May 11, 2021

View reviewed changes

anuraaga reviewed May 12, 2021

View reviewed changes

odeke-em force-pushed the receiver-prometheus-createNodeAndResource branch from b949055 to 9e0df1b Compare May 12, 2021 07:08

odeke-em requested review from tigrannajaryan, bogdandrutu and anuraaga May 12, 2021 07:10

odeke-em mentioned this pull request May 12, 2021

receiver/prometheus/internal: direct metricfamily Prometheus->OTLP + equivalent tests #3145

Merged

Aneurysm9 approved these changes May 12, 2021

View reviewed changes

odeke-em force-pushed the receiver-prometheus-createNodeAndResource branch from 9e0df1b to 33919a8 Compare May 14, 2021 03:31

dashpole approved these changes May 19, 2021

View reviewed changes

bogdandrutu added the waiting-for-author label May 19, 2021

github-actions bot added the Stale label May 27, 2021

odeke-em added 4 commits May 27, 2021 10:37

Use .Attributes to skip unsafe extraction plus use require.Equal

dd55c24

Addresses feedback pointed out by Bogdan, Tigran, Jaana by using Resource.Attributes().Sort() instead and also avoid using go-cmp/cmp and instead opt to using require.Equal.

Add comment and rename parity test check

3977f60

Add createNodeAndResource unit test

6ea4b9e

odeke-em force-pushed the receiver-prometheus-createNodeAndResource branch from 33919a8 to 6ea4b9e Compare May 27, 2021 08:17

odeke-em requested a review from alolita as a code owner May 27, 2021 08:17

Aneurysm9 approved these changes May 27, 2021

View reviewed changes

github-actions bot removed the Stale label May 28, 2021

anuraaga approved these changes May 28, 2021

View reviewed changes

bogdandrutu approved these changes Jun 7, 2021

View reviewed changes

bogdandrutu merged commit 4171b70 into open-telemetry:main Jun 7, 2021

CoderPoet mentioned this pull request Feb 22, 2022

prometheusreceiver upsert job to service.name open-telemetry/opentelemetry-collector-contrib#8060

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

receiver/prometheus/internal: add createNodeAndResourcePdata for Prometheus->OTLP Pdata #3139

receiver/prometheus/internal: add createNodeAndResourcePdata for Prometheus->OTLP Pdata #3139

odeke-em commented May 11, 2021

odeke-em commented May 11, 2021

alolita commented May 11, 2021

bogdandrutu May 11, 2021

anuraaga May 12, 2021 •

edited

Loading

bogdandrutu May 11, 2021

odeke-em May 12, 2021

Aneurysm9 May 12, 2021

odeke-em May 14, 2021

anuraaga May 12, 2021

odeke-em May 12, 2021

anuraaga May 12, 2021

odeke-em May 12, 2021

bogdandrutu May 17, 2021

CoderPoet Feb 22, 2022

anuraaga May 12, 2021 •

edited

Loading

odeke-em commented May 14, 2021

anuraaga commented May 14, 2021

odeke-em commented May 14, 2021

anuraaga commented May 14, 2021

bogdandrutu commented May 14, 2021

alolita commented May 14, 2021

odeke-em commented May 18, 2021

bogdandrutu commented May 19, 2021

github-actions bot commented May 27, 2021

odeke-em commented May 27, 2021

odeke-em commented Jun 2, 2021

receiver/prometheus/internal: add createNodeAndResourcePdata for Prometheus->OTLP Pdata #3139

receiver/prometheus/internal: add createNodeAndResourcePdata for Prometheus->OTLP Pdata #3139

Conversation

odeke-em commented May 11, 2021

odeke-em commented May 11, 2021

alolita commented May 11, 2021

Choose a reason for hiding this comment

anuraaga May 12, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga May 12, 2021 • edited Loading

Choose a reason for hiding this comment

odeke-em commented May 14, 2021

anuraaga commented May 14, 2021

odeke-em commented May 14, 2021

anuraaga commented May 14, 2021

bogdandrutu commented May 14, 2021

alolita commented May 14, 2021

odeke-em commented May 18, 2021

bogdandrutu commented May 19, 2021

github-actions bot commented May 27, 2021

odeke-em commented May 27, 2021

odeke-em commented Jun 2, 2021

anuraaga May 12, 2021 •

edited

Loading

anuraaga May 12, 2021 •

edited

Loading