Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[prometheusremotewriteexporter] Correctly run all prw unit tests, and skip intentional WAL failure. #8401

Merged
merged 3 commits into from May 10, 2022
Merged

[prometheusremotewriteexporter] Correctly run all prw unit tests, and skip intentional WAL failure. #8401

merged 3 commits into from May 10, 2022

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Mar 14, 2022 •
edited

Description:

While working on a different PR, I found that PRW tests are not actually running each case. It runs the last case a whole bunch of times instead. Declaring the test case as a local var fixes this, but uncovers a WAL test failure. This PR skips that test only for the WAL case.

The failure for the WAL case:

--- FAIL: Test_PushMetrics (8.22s)
    --- FAIL: Test_PushMetrics/WAL (0.00s)
        --- FAIL: Test_PushMetrics/WAL/5xx_case (4.11s)
            exporter_test.go:667:
                	Error Trace:	exporter_test.go:667
                	Error:      	An error is expected but got nil.
                	Test:       	Test_PushMetrics/WAL/5xx_case
FAIL
FAIL	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/prometheusremotewriteexporter	12.401s
FAIL

Follow-up from #4751.

@dashpole dashpole requested review from a team and Aneurysm9 as code owners March 14, 2022 16:03
@dashpole dashpole assigned Aneurysm9 and unassigned mx-psi Mar 14, 2022
@dashpole dashpole added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Mar 14, 2022
@Aneurysm9
Copy link
Member

Unfortunately, I'm not sure it's a simple fix. The issue seems to be the expectation of an error returned from PushMetrics() when a 5xx response is provided, but the data flow is changed when a WAL is in use and a non-error return happens once the data is persisted to the WAL.

I think it might make sense to separate this out so that it is only tested for the non-WAL case and then separately test for correct handling of server errors when a WAL is in use.

@dashpole
Copy link
Contributor Author

Sounds good. I'll go ahead and add that onto this PR when I get the chance

@dashpole
Copy link
Contributor Author

Updated to skip the test when the WAL is in use. I also cleaned up the cases a bit so all tests don't have to set skipForWal: false

@github-actions
Copy link
Contributor

github-actions bot commented Apr 1, 2022

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

@github-actions github-actions bot added the Stale label Apr 1, 2022
@dashpole dashpole removed the Stale label Apr 1, 2022
@dashpole dashpole mentioned this pull request Apr 13, 2022
Merged
@dashpole dashpole force-pushed the fix_prom_tests branch 2 times, most recently from 6de32e3 to 4808b71 Compare April 13, 2022 20:06
@dashpole
Copy link
Contributor Author

dashpole commented Apr 14, 2022 •
edited

Failing because of #9255

Edit: no longer failing.

@dashpole dashpole changed the title [prometheusremotewriteexporter] Correctly run all prw unit tests (found a WAL failure) [prometheusremotewriteexporter] Correctly run all prw unit tests, and skip intentional WAL failure. Apr 14, 2022
@dashpole
Copy link
Contributor Author

This is still waiting on review, and fixes important Prometheus unit tests.

@dashpole dashpole added comp:prometheus Prometheus related issues bug Something isn't working labels Apr 25, 2022
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label May 10, 2022
djaglowski
djaglowski approved these changes May 10, 2022
View reviewed changes
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@djaglowski djaglowski merged commit 10b308e into open-telemetry:main May 10, 2022
djaglowski pushed a commit to djaglowski/opentelemetry-collector-contrib that referenced this pull request May 10, 2022
@dashpole @djaglowski
[prometheusremotewriteexporter] Correctly run all prw unit tests, and…
b413547 
… skip intentional WAL failure. (open-telemetry#8401)

* correctly run all prw unit tests

* reformat test cases, and omit zero values

* skip unsupported test when using WAL
@ahayworth ahayworth mentioned this pull request May 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djaglowski djaglowski djaglowski approved these changes

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9 Aneurysm9 is a code owner

Assignees

@Aneurysm9 Aneurysm9

Labels
bug Something isn't working comp:prometheus Prometheus related issues Skip Changelog PRs that do not require a CHANGELOG.md entry Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dashpole @Aneurysm9 @djaglowski @mx-psi