exporter/prometheusremotewriteexporter: import and add official compliance tests. #5014

kirbyquerby · 2021-08-30T19:21:49Z

Description: exporter/prometheusremotewrite: import and add official compliance tests.
This change imports and runs the official Prometheus Remote Write compliance tests from https://github.com/prometheus/compliance against a live collector end-to-end. Due to data races in the official tests, this test is currently skipped.

Link to tracking Issue: Fixes open-telemetry/prometheus-interoperability-spec/issues/34

Testing: This change itself is a test :)

kirbyquerby · 2021-08-30T19:33:42Z

/cc @odeke-em

odeke-em

LGTM, thank you @kirbyquerby! Minor nit, please examine the 10s artificial sleep and also add a function to properly shut down lest we'll encounter race conditions with the OpenTelemetry service as I've encountered before.

exporter/prometheusremotewriteexporter/compliance/compliance_test.go

odeke-em · 2021-08-30T20:22:45Z

exporter/prometheusremotewriteexporter/compliance/compliance_test.go

+	app := startCollector(t, l.Addr())
+	defer app.Shutdown()
+
+	time.Sleep(10 * time.Second)


Why this artificial sleep? Could we perhaps reduce this as much.

The 10s sleep comes from the original tests: https://github.com/prometheus/compliance/blob/main/remote_write/main_test.go#L102
The collector process is interrupted after TIMEOUT seconds: https://github.com/prometheus/compliance/blob/main/remote_write/targets/common.go#L276

We can potentially lower timeout by some amount without causing flakiness. Once we fix the go modules issues, I can try playing around with lower values.

No worries, we can keep them as they are.

exporter/prometheusremotewriteexporter/compliance/compliance_test.go

kirbyquerby

Thanks for the review Emmanuel!

exporter/prometheusremotewriteexporter/compliance/compliance_test.go

kirbyquerby · 2021-08-30T21:58:10Z

exporter/prometheusremotewriteexporter/compliance/compliance_test.go

+	app := startCollector(t, l.Addr())
+	defer app.Shutdown()
+
+	time.Sleep(10 * time.Second)


The 10s sleep comes from the original tests: https://github.com/prometheus/compliance/blob/main/remote_write/main_test.go#L102
The collector process is interrupted after TIMEOUT seconds: https://github.com/prometheus/compliance/blob/main/remote_write/targets/common.go#L276

We can potentially lower timeout by some amount without causing flakiness. Once we fix the go modules issues, I can try playing around with lower values.

exporter/prometheusremotewriteexporter/compliance/compliance_test.go

odeke-em · 2021-09-03T00:58:32Z

Kindly requesting y'all eyes @Aneurysm9 @bogdandrutu @tigrannajaryan.

odeke-em · 2021-09-10T22:29:50Z

Kindly pinging y'all, it's been 11 days and the PR is getting stale. Kindly cc-ing @dashpole

codeboten

Code looks good, please address the conflicts. It looks like lint and the tests are failing as well:

exporter/prometheusremotewriteexporter/compliance/compliance_test.go: Expected no more than 3 groups, got 4

dashpole

LGTM!

kirbyquerby · 2021-09-13T19:29:40Z

There are currently data races in the compliance tests (and in the collector) that cause this test to fail with -race, so for now it's skipped. I've created prometheus/compliance#40 for the compliance races and can work on the races in the collector after the compliance test races are fixed).

kirbyquerby · 2021-09-13T21:39:56Z

The CI check failures appear to be caused elsewhere. See this clean PR which also fails checks: #5228

kirbyquerby · 2021-09-14T01:27:18Z

I was mistaken :)
After running a bunch of go mod tidys, everything seems to be in order (other than loadtest >:( ). No idea why #5228 is failing. Flaky tests?

odeke-em · 2021-09-14T14:51:53Z

Code looks good, please address the conflicts. It looks like lint and the tests are failing as well:

exporter/prometheusremotewriteexporter/compliance/compliance_test.go: Expected no more than 3 groups, got 4

@codeboten please take a look again. The Prometheus Remote Writer test code has data races they'll need to fix. On our end though we are good to go.

odeke-em · 2021-09-14T23:12:07Z

Kindly cc-ing @alolita @bogdandrutu @codeboten @Aneurysm9 to take a look before this PR gets stale again, thank you!

codeboten

i suspect another make gotidy is needed here to address the conflicts.

kirbyquerby · 2021-09-15T23:09:35Z

Rebased + gotidied. Thanks for the review Alex!

…iance tests. run make gotidy run make gotidy again ???????? run go mod tidy manually in prometheusexporter run make gotidy after rebasing make gotidy manually run go mod tidy in prometheusexporter

Aneurysm9 · 2021-09-20T23:46:08Z

exporter/prometheusremotewriteexporter/compliance/compliance_test.go

+
+var (
+	logger = log.NewLogfmtLogger(log.NewSyncWriter(os.Stderr))
+	tests  = []func() cases.Test{


I'm not sure that duplicating the test harness here is the best approach. Can we instead have an action that runs on PR that pulls the compliance test repo and runs the tests there against a build artifact?

Sure, I've made #5739

github-actions · 2021-09-28T05:15:19Z

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

tigrannajaryan · 2021-10-04T13:21:39Z

Please resolve the conflicts and address/resolve remaining open comments.

github-actions · 2021-10-13T05:15:24Z

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

codeboten

@kirbyquerby can this be closed as superseeded by #5739?

kirbyquerby · 2021-10-21T00:24:57Z

@codeboten Yeah :D

kirbyquerby force-pushed the wg-issue-34/prom-wr-compliance branch 2 times, most recently from 96d2db3 to 768db57 Compare August 30, 2021 19:29

kirbyquerby changed the title ~~exporter/prometheusremotewriteexporter: add prometheus remote write compatibility tests.~~ exporter/prometheusremotewriteexporter: import and add official compliance tests. Aug 30, 2021

kirbyquerby force-pushed the wg-issue-34/prom-wr-compliance branch from 768db57 to 834d67c Compare August 30, 2021 19:32

kirbyquerby marked this pull request as ready for review August 30, 2021 19:32

kirbyquerby requested review from a team and codeboten August 30, 2021 19:32

github-actions bot assigned mx-psi Aug 30, 2021

odeke-em approved these changes Aug 30, 2021

View reviewed changes

kirbyquerby commented Aug 30, 2021

View reviewed changes

kirbyquerby force-pushed the wg-issue-34/prom-wr-compliance branch 2 times, most recently from 4472318 to bfbf0ca Compare September 7, 2021 22:06

codeboten reviewed Sep 10, 2021

View reviewed changes

kirbyquerby force-pushed the wg-issue-34/prom-wr-compliance branch from bfbf0ca to 3f08474 Compare September 10, 2021 23:27

dashpole approved these changes Sep 13, 2021

View reviewed changes

kirbyquerby mentioned this pull request Sep 13, 2021

otel remote_write fails when run with -race prometheus/compliance#40

Open

kirbyquerby force-pushed the wg-issue-34/prom-wr-compliance branch from 3f08474 to b6d628c Compare September 13, 2021 19:26

kirbyquerby requested review from alolita and anuraaga as code owners September 13, 2021 20:13

kirbyquerby force-pushed the wg-issue-34/prom-wr-compliance branch from 96c17df to 95ce1cc Compare September 13, 2021 22:03

kirbyquerby force-pushed the wg-issue-34/prom-wr-compliance branch from 7ea9db7 to 8bf4140 Compare September 14, 2021 22:08

codeboten approved these changes Sep 15, 2021

View reviewed changes

kirbyquerby force-pushed the wg-issue-34/prom-wr-compliance branch 2 times, most recently from 055d1b2 to 24bcded Compare September 15, 2021 23:07

anuraaga approved these changes Sep 16, 2021

View reviewed changes

kirbyquerby force-pushed the wg-issue-34/prom-wr-compliance branch from e5803c7 to 9751f72 Compare September 16, 2021 19:20

kirbyquerby added 4 commits September 20, 2021 13:46

exporter/prometheusremotewriteexporter: import and add official compl…

1163190

…iance tests. run make gotidy run make gotidy again ???????? run go mod tidy manually in prometheusexporter run make gotidy after rebasing make gotidy manually run go mod tidy in prometheusexporter

make gotidy after rebase

34f3bdc

manual go mod tidy in prometheusexporter

9553b7a

add replace directive + add context to Run() call

cf48dff

kirbyquerby force-pushed the wg-issue-34/prom-wr-compliance branch from 9751f72 to cf48dff Compare September 20, 2021 19:18

Aneurysm9 reviewed Sep 20, 2021

View reviewed changes

github-actions bot added the Stale label Sep 28, 2021

bogdandrutu removed the Stale label Sep 29, 2021

github-actions bot added the Stale label Oct 13, 2021

bogdandrutu removed the Stale label Oct 13, 2021

codeboten reviewed Oct 20, 2021

View reviewed changes

kirbyquerby closed this Oct 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exporter/prometheusremotewriteexporter: import and add official compliance tests. #5014

exporter/prometheusremotewriteexporter: import and add official compliance tests. #5014

kirbyquerby commented Aug 30, 2021 •

edited

Loading

kirbyquerby commented Aug 30, 2021

odeke-em left a comment

odeke-em Aug 30, 2021

kirbyquerby Aug 30, 2021

odeke-em Sep 14, 2021

kirbyquerby left a comment

kirbyquerby Aug 30, 2021

odeke-em commented Sep 3, 2021

odeke-em commented Sep 10, 2021

codeboten left a comment

dashpole left a comment

kirbyquerby commented Sep 13, 2021

kirbyquerby commented Sep 13, 2021

kirbyquerby commented Sep 14, 2021

odeke-em commented Sep 14, 2021

odeke-em commented Sep 14, 2021

codeboten left a comment •

edited

Loading

kirbyquerby commented Sep 15, 2021

Aneurysm9 Sep 20, 2021

kirbyquerby Oct 13, 2021

github-actions bot commented Sep 28, 2021

tigrannajaryan commented Oct 4, 2021

github-actions bot commented Oct 13, 2021

codeboten left a comment

kirbyquerby commented Oct 21, 2021

exporter/prometheusremotewriteexporter: import and add official compliance tests. #5014

exporter/prometheusremotewriteexporter: import and add official compliance tests. #5014

Conversation

kirbyquerby commented Aug 30, 2021 • edited Loading

kirbyquerby commented Aug 30, 2021

odeke-em left a comment

Choose a reason for hiding this comment

odeke-em Aug 30, 2021

Choose a reason for hiding this comment

kirbyquerby Aug 30, 2021

Choose a reason for hiding this comment

odeke-em Sep 14, 2021

Choose a reason for hiding this comment

kirbyquerby left a comment

Choose a reason for hiding this comment

kirbyquerby Aug 30, 2021

Choose a reason for hiding this comment

odeke-em commented Sep 3, 2021

odeke-em commented Sep 10, 2021

codeboten left a comment

Choose a reason for hiding this comment

dashpole left a comment

Choose a reason for hiding this comment

kirbyquerby commented Sep 13, 2021

kirbyquerby commented Sep 13, 2021

kirbyquerby commented Sep 14, 2021

odeke-em commented Sep 14, 2021

odeke-em commented Sep 14, 2021

codeboten left a comment • edited Loading

Choose a reason for hiding this comment

kirbyquerby commented Sep 15, 2021

Aneurysm9 Sep 20, 2021

Choose a reason for hiding this comment

kirbyquerby Oct 13, 2021

Choose a reason for hiding this comment

github-actions bot commented Sep 28, 2021

tigrannajaryan commented Oct 4, 2021

github-actions bot commented Oct 13, 2021

codeboten left a comment

Choose a reason for hiding this comment

kirbyquerby commented Oct 21, 2021

kirbyquerby commented Aug 30, 2021 •

edited

Loading

codeboten left a comment •

edited

Loading