Don't drop instance and job labels from the Prometheus receiver #2964

rakyll · 2021-04-20T03:31:37Z

Fixes #575
Fixes #2499
Fixes #2363
Fixes open-telemetry/prometheus-interoperability-spec#37
Fixes open-telemetry/prometheus-interoperability-spec#39
Fixes open-telemetry/prometheus-interoperability-spec#44

Passing compliance tests:

$ go test --tags=compliance -run "TestRemoteWrite/otelcollector/Job.+" -v ./
=== RUN TestRemoteWrite
=== RUN TestRemoteWrite/otelcollector
=== RUN TestRemoteWrite/otelcollector/JobLabel
=== PAUSE TestRemoteWrite/otelcollector/JobLabel
=== CONT TestRemoteWrite/otelcollector/JobLabel
--- PASS: TestRemoteWrite (10.02s)
--- PASS: TestRemoteWrite/otelcollector (0.00s)
--- PASS: TestRemoteWrite/otelcollector/JobLabel (10.02s)
PASS
ok github.com/prometheus/compliance/remote_write 10.382s
$ go test --tags=compliance -run "TestRemoteWrite/otelcollector/Instance.+" -v ./
=== RUN TestRemoteWrite
=== RUN TestRemoteWrite/otelcollector
=== RUN TestRemoteWrite/otelcollector/InstanceLabel
=== PAUSE TestRemoteWrite/otelcollector/InstanceLabel
=== CONT TestRemoteWrite/otelcollector/InstanceLabel
--- PASS: TestRemoteWrite (10.01s)
--- PASS: TestRemoteWrite/otelcollector (0.00s)
--- PASS: TestRemoteWrite/otelcollector/InstanceLabel (10.01s)
PASS
ok github.com/prometheus/compliance/remote_write 10.291s
$ go test --tags=compliance -run "TestRemoteWrite/otelcollector/RepeatedLabels.+" -v ./
=== RUN TestRemoteWrite
=== RUN TestRemoteWrite/otelcollector
--- PASS: TestRemoteWrite (0.00s)
--- PASS: TestRemoteWrite/otelcollector (0.00s)
testing: warning: no tests to run
PASS

Fixes #575 Fixes #2499 Fixes #2363 Fixes open-telemetry/prometheus-interoperability-spec#37 Fixes open-telemetry/prometheus-interoperability-spec#39 Passing compliance tests: $ go test --tags=compliance -run "TestRemoteWrite/otelcollector/Job.+" -v ./ === RUN TestRemoteWrite === RUN TestRemoteWrite/otelcollector === RUN TestRemoteWrite/otelcollector/JobLabel === PAUSE TestRemoteWrite/otelcollector/JobLabel === CONT TestRemoteWrite/otelcollector/JobLabel --- PASS: TestRemoteWrite (10.02s) --- PASS: TestRemoteWrite/otelcollector (0.00s) --- PASS: TestRemoteWrite/otelcollector/JobLabel (10.02s) PASS ok github.com/prometheus/compliance/remote_write 10.382s $ go test --tags=compliance -run "TestRemoteWrite/otelcollector/Instance.+" -v ./ === RUN TestRemoteWrite === RUN TestRemoteWrite/otelcollector === RUN TestRemoteWrite/otelcollector/InstanceLabel === PAUSE TestRemoteWrite/otelcollector/InstanceLabel === CONT TestRemoteWrite/otelcollector/InstanceLabel --- PASS: TestRemoteWrite (10.01s) --- PASS: TestRemoteWrite/otelcollector (0.00s) --- PASS: TestRemoteWrite/otelcollector/InstanceLabel (10.01s) PASS ok github.com/prometheus/compliance/remote_write 10.291s

rakyll · 2021-04-20T03:31:46Z

cc @odeke-em

odeke-em

This is quite the awesome PR!! Thank you @rakyll , LGTM!

rakyll · 2021-04-20T05:05:37Z

Will fix the tests soon.

rakyll · 2021-04-20T05:57:18Z

The failing test (TestEndToEndSummarySupport) has a hard dependency on the receiver and byte to byte comparing the output. Is there a way we can remove this test and add something a bit more self contained @odeke-em?

odeke-em · 2021-04-20T05:58:29Z

Please t.Skipf("@odeke-em please take a look") it and then we'll be gucci to go.

rakyll · 2021-04-20T05:58:31Z

It seems like the pull based Prometheus exporter is not exposing the job and instance labels and it seems like it's a problem itself.

codecov · 2021-04-20T06:08:36Z

Codecov Report

Merging #2964 (339944a) into main (0a2ea1b) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2964      +/-   ##
==========================================
- Coverage   91.63%   91.54%   -0.09%     
==========================================
  Files         312      312              
  Lines       15445    15443       -2     
==========================================
- Hits        14153    14138      -15     
- Misses        883      897      +14     
+ Partials      409      408       -1

Impacted Files	Coverage Δ
...iver/prometheusreceiver/internal/metricsbuilder.go	`100.00% <100.00%> (ø)`
exporter/prometheusexporter/accumulator.go	`92.53% <0.00%> (-6.47%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a2ea1b...339944a. Read the comment docs.

rakyll · 2021-04-20T06:13:26Z

I can't see why build-and-test is failed and can't reproduce it myself locally. Can someone paste the log?

alolita · 2021-04-20T06:45:46Z

Nicely done @rakyll ty!

rakyll · 2021-04-20T17:18:56Z

cc @bogdandrutu @tigrannajaryan Can you review this? It's required for the stability.

Aneurysm9 · 2021-04-20T21:01:18Z

exporter/prometheusexporter/end_to_end_test.go

@@ -40,6 +40,10 @@ func TestEndToEndSummarySupport(t *testing.T) {
 		t.Skip("This test can take a couple of seconds")
 	}

+	// TODO(odeke-em): Fix this, see https://github.com/open-telemetry/opentelemetry-collector/pull/2964
+	// for context.
+	t.Skipf("Not supporting instances and jobs, skipping for now. @odeke-em to take a look.")


Suggested change

t.Skipf("Not supporting instances and jobs, skipping for now. @odeke-em to take a look.")

t.Skip("Not supporting instances and jobs, skipping for now. @odeke-em to take a look.")

This doesn't appear to be using the formatting template to inject values, so t.Skip() should suffice.

tigrannajaryan · 2021-04-21T13:34:26Z

I don't know enough about this receiver to understand the change fully. @bogdandrutu @jrcamp maybe you can help review?

dashpole · 2021-04-21T15:01:24Z

receiver/prometheusreceiver/metrics_receiver_test.go

@@ -263,12 +270,17 @@ func verifyTarget1(t *testing.T, td *testData, mds []internaldata.MetricsData) {
 			Name:        "go_threads",
 			Description: "Number of OS threads created",
 			Type:        metricspb.MetricDescriptor_GAUGE_DOUBLE,
+			LabelKeys:   []*metricspb.LabelKey{{Key: "instance"}, {Key: "job"}},


Should instance and job be part of the resource instead of labels?

You are correct, working on it and will send another version.

Aneurysm9 · 2021-04-21T16:33:39Z

How does this relate to #2683?

rakyll · 2021-04-21T19:15:36Z

Moving this PR to #2979. This issue requires an exporter change which populates the "instance" and "job" from resource attributes.

rakyll · 2021-04-21T19:25:52Z

@Aneurysm9, just seeing your last comment, #2683 is related to this but it seems like #2897 was doing the right thing to turn these labels into resource attributes. What was missing was to take them from the resource attributes and produce Prometheus labels in the exporters which #2979 is doing.

This change was a part of #2964 but it's closed now. Having typed arguments make it easier to massage the arguments before comparing. Contributing this change for future maintainers.

This change was a part of open-telemetry#2964 but it's closed now. Having typed arguments make it easier to massage the arguments before comparing. Contributing this change for future maintainers.

rakyll requested a review from a team April 20, 2021 03:31

odeke-em approved these changes Apr 20, 2021

View reviewed changes

rakyll added 6 commits April 19, 2021 22:05

Fix tests

115f1e7

Sort labels and values before comparing

4c08c30

Fix tests

632d788

Improve readability

fa26478

Fix lint

4c0af72

Fix lint

ac7edab

Skip broken test for now

339944a

rakyll mentioned this pull request Apr 20, 2021

Add job and instance as well-known labels in prometheus receiver #2897

Merged

Aneurysm9 approved these changes Apr 20, 2021

View reviewed changes

dashpole reviewed Apr 21, 2021

View reviewed changes

bogdandrutu added the waiting-for-author label Apr 21, 2021

rakyll closed this Apr 21, 2021

rakyll mentioned this pull request Apr 21, 2021

Minor change to doCompare #2982

Closed

Aneurysm9 mentioned this pull request May 3, 2021

Track @Aneurysm9 interest and contributions for approver #2820

Closed

rakyll mentioned this pull request May 7, 2021

Minor change to doCompare #3130

Merged

hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023

log endpoint id with discovery status (open-telemetry#2964)

7b7cb53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't drop instance and job labels from the Prometheus receiver #2964

Don't drop instance and job labels from the Prometheus receiver #2964

rakyll commented Apr 20, 2021 •

edited

Loading

rakyll commented Apr 20, 2021

odeke-em left a comment

rakyll commented Apr 20, 2021

rakyll commented Apr 20, 2021

odeke-em commented Apr 20, 2021

rakyll commented Apr 20, 2021

codecov bot commented Apr 20, 2021

rakyll commented Apr 20, 2021

alolita commented Apr 20, 2021

rakyll commented Apr 20, 2021

Aneurysm9 Apr 20, 2021

tigrannajaryan commented Apr 21, 2021

dashpole Apr 21, 2021

rakyll Apr 21, 2021

Aneurysm9 commented Apr 21, 2021

rakyll commented Apr 21, 2021

rakyll commented Apr 21, 2021

	t.Skipf("Not supporting instances and jobs, skipping for now. @odeke-em to take a look.")
	t.Skip("Not supporting instances and jobs, skipping for now. @odeke-em to take a look.")

Don't drop instance and job labels from the Prometheus receiver #2964

Don't drop instance and job labels from the Prometheus receiver #2964

Conversation

rakyll commented Apr 20, 2021 • edited Loading

rakyll commented Apr 20, 2021

odeke-em left a comment

Choose a reason for hiding this comment

rakyll commented Apr 20, 2021

rakyll commented Apr 20, 2021

odeke-em commented Apr 20, 2021

rakyll commented Apr 20, 2021

codecov bot commented Apr 20, 2021

Codecov Report

rakyll commented Apr 20, 2021

alolita commented Apr 20, 2021

rakyll commented Apr 20, 2021

Aneurysm9 Apr 20, 2021

Choose a reason for hiding this comment

tigrannajaryan commented Apr 21, 2021

dashpole Apr 21, 2021

Choose a reason for hiding this comment

rakyll Apr 21, 2021

Choose a reason for hiding this comment

Aneurysm9 commented Apr 21, 2021

rakyll commented Apr 21, 2021

rakyll commented Apr 21, 2021

rakyll commented Apr 20, 2021 •

edited

Loading