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.

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

Fix hostmetricsreceiver/processscraper tests to work for darwin #20538

Merged
merged 5 commits into from
Apr 10, 2023

Conversation

8naama
Copy link
Contributor

@8naama 8naama commented Apr 1, 2023

Description:

  • Excluded darwin unsupported metrics from testCases, 'metricsLen', default config
  • Excluded from testCases the 'Parent PID Error' test since it's excluded in the code
  • Added darwin specific test case for Cmdline()

Not sure I took the best approach here (as I also added a function to a DO NOT EDIT file - generated_metrics.go), will love your feedback!

Link to tracking Issue: Issue #19141

Testing:
Tested locally on Mac to make sure metrics are collected and that running go test at the ./hostmetricsreceiver/internal/scraper/processscraper folder for returns PASS

Documentation:
added notes for excluding unsupported metrics

@8naama 8naama requested a review from a team April 1, 2023 13:51
@8naama 8naama requested a review from dmitryax as a code owner April 1, 2023 13:51
@runforesight
Copy link

runforesight bot commented Apr 1, 2023

Foresight Summary

    
Major Impacts

build-and-test-windows duration(6 seconds) has decreased 31 minutes 13 seconds compared to main branch avg(31 minutes 19 seconds).
View More Details

⭕  build-and-test-windows workflow has finished in 6 seconds (31 minutes 28 seconds less than main branch avg.) and finished at 8th Apr, 2023.


Job Failed Steps Tests
windows-unittest -     🔗  N/A See Details
windows-unittest-matrix -     🔗  N/A See Details

✅  check-links workflow has finished in 44 seconds and finished at 8th Apr, 2023.


Job Failed Steps Tests
changed files -     🔗  N/A See Details
check-links -     🔗  N/A See Details

✅  telemetrygen workflow has finished in 1 minute 3 seconds and finished at 8th Apr, 2023.


Job Failed Steps Tests
build-dev -     🔗  N/A See Details
publish-latest -     🔗  N/A See Details
publish-stable -     🔗  N/A See Details

✅  changelog workflow has finished in 2 minutes 19 seconds and finished at 8th Apr, 2023.


Job Failed Steps Tests
changelog -     🔗  N/A See Details

✅  prometheus-compliance-tests workflow has finished in 3 minutes 20 seconds (2 minutes 55 seconds less than main branch avg.) and finished at 8th Apr, 2023.


Job Failed Steps Tests
prometheus-compliance-tests -     🔗  N/A See Details

✅  load-tests workflow has finished in 6 minutes 18 seconds (4 minutes 4 seconds less than main branch avg.) and finished at 8th Apr, 2023.


Job Failed Steps Tests
setup-environment -     🔗  N/A See Details
loadtest (TestIdleMode) -     🔗  N/A See Details
loadtest (TestBallastMemory|TestLog10kDPS) -     🔗  N/A See Details
loadtest (TestMetric10kDPS|TestMetricsFromFile) -     🔗  N/A See Details
loadtest (TestMetricResourceProcessor|TestTrace10kSPS) -     🔗  N/A See Details
loadtest (TestTraceNoBackend10kSPS|TestTrace1kSPSWithAttrs) -     🔗  N/A See Details
loadtest (TestTraceBallast1kSPSWithAttrs|TestTraceBallast1kSPSAddAttrs) -     🔗  N/A See Details
loadtest (TestTraceAttributesProcessor) -     🔗  N/A See Details

✅  e2e-tests workflow has finished in 10 minutes 56 seconds (3 minutes 10 seconds less than main branch avg.) and finished at 8th Apr, 2023.


Job Failed Steps Tests
kubernetes-test (v1.26.0) -     🔗  N/A See Details
kubernetes-test (v1.25.3) -     🔗  N/A See Details
kubernetes-test (v1.23.13) -     🔗  N/A See Details
kubernetes-test (v1.24.7) -     🔗  N/A See Details

✅  build-and-test workflow has finished in 23 minutes 58 seconds (22 minutes 37 seconds less than main branch avg.) and finished at 9th Apr, 2023.


Job Failed Steps Tests
lint-matrix (other) -     🔗  N/A See Details
lint -     🔗  N/A See Details
cross-compile (darwin, amd64) -     🔗  N/A See Details
cross-compile (darwin, arm64) -     🔗  N/A See Details
cross-compile (linux, 386) -     🔗  N/A See Details
cross-compile (linux, amd64) -     🔗  N/A See Details
cross-compile (linux, arm) -     🔗  N/A See Details
cross-compile (linux, arm64) -     🔗  N/A See Details
cross-compile (linux, ppc64le) -     🔗  N/A See Details
cross-compile (windows, 386) -     🔗  N/A See Details
cross-compile (windows, amd64) -     🔗  N/A See Details
build-package (deb) -     🔗  N/A See Details
build-package (rpm) -     🔗  N/A See Details
windows-msi -     🔗  N/A See Details
publish-check -     🔗  N/A See Details
publish-stable -     🔗  N/A See Details
publish-dev -     🔗  N/A See Details
rotate-milestone -     🔗  N/A See Details

🔎 See details on Foresight

*You can configure Foresight comments in your organization settings page.

@8naama 8naama requested a review from mx-psi April 6, 2023 06:20
@andrzej-stencel
Copy link
Member

@8naama, the tests in process_scraper_test.go are still being skipped on MacOS, as described in the issue:

the skipTestOnUnsupportedOS helper disables tests for metrics that are now supported on Darwin.

These tests need to be run on MacOS as well as on Linux and Windows.

@8naama
Copy link
Contributor Author

8naama commented Apr 6, 2023

Thank you @astencel-sumo !! Sorry, I probably had a mistake when working on it and accidentally removed the line when I enabled it and my tests last commit were wrong. 🤦🏻‍♀️

I made sure it's enabled now and re-tested everything

@andrzej-stencel
Copy link
Member

Thanks @8naama, this fixes the tests (just fix the "deafult" typos that the bot has diligently pointed out ;)).

I'm worried that this change introduces even more complexity into this already too complicated test code. Ideally, these tests should be refactored for better maintainability, to have less "if" conditions all over the place. I think this is a task for another PR though.

What do you think @dmitryax?

@dmitryax
Copy link
Member

dmitryax commented Apr 8, 2023

I agree. It would be great to simplify the test code overall, but not required for this PR

@mx-psi mx-psi merged commit ec8176d into open-telemetry:main Apr 10, 2023
@github-actions github-actions bot added this to the next release milestone Apr 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants