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

[chore][cmd/otelcontribcol] Re-enable exporters and receivers tests on Windows #29532

Merged
merged 11 commits into from
Dec 1, 2023

Conversation

pjanotti
Copy link
Contributor

@pjanotti pjanotti commented Nov 28, 2023

Description:
Last part to re-enable all Windows tests disabled due to issue #11451.

Notes:

  • The default test timeout is not enough for the cmd GROUP on CI and it was increased to 1200s just for this group on Windows, the default stays at 300s (which was also enough for my local run).
  • The fileexporter life cycle test indicates an issue that shouldn't affect most usages of the collector: if the same configuration is used to export multiple signals it opens one file handle for each signal but just closes the one of them at shutdown (the helper only executes the shutdown for the one signal). This is an issue for the test because during cleanup the testing attempts to delete the test folder and there are still handles open to the file that lives inside it. This shouldn't be a problem for collector users. That said the exporter should close all file handles on shutdown. I will take look at what can be done to correct this behavior.
  • Fixed a minor typo on the receivers test that affected many lines.

Link to tracking Issue:
Fix #28679 - last part.

Testing:
Local run and GH CI on my fork with Run Windows label.

Documentation:
N/A

@pjanotti pjanotti requested review from a team and mx-psi November 28, 2023 14:29
@github-actions github-actions bot added the cmd/otelcontribcol otelcontribcol command label Nov 28, 2023
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks @pjanotti, any idea why the tests are 4x slower on windows?

Please take a look at the failing check:

Generated code is out of date, please run "make genotelcontribcol" and commit the changes in this PR.

@pjanotti pjanotti force-pushed the part-04-of-issue-28679 branch from e741250 to af1f1bc Compare November 28, 2023 17:57
@pjanotti
Copy link
Contributor Author

@codeboten

any idea why the tests are 4x slower on windows?

I'm not sure at this point, but, I suspect the memory restrictions that are needed to run these tests on Windows GH runners:

# Limit memory usage via GC environment variables to avoid OOM on GH runners, especially for `cmd/otelcontribcol`,
# see https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/28682#issuecomment-1802296776
GOGC: 50
GOMEMLIMIT: 2GiB

I'm planning to verify that.

@pjanotti
Copy link
Contributor Author

I missed a required change in receiver\windowseventlogreceiver: its config type is different between the platforms. This PR should be broken until I fix that - doing as a separate PR to keep the history clean. Changing the current PR to draft until that one is fixed.

@pjanotti pjanotti marked this pull request as draft November 28, 2023 19:11
@mx-psi mx-psi added the Run Windows Enable running windows test on a PR label Nov 29, 2023
@pjanotti
Copy link
Contributor Author

@codeboten my theory about GOGC=50 GOMEMLIMIT=2GiB GOMAXPROCS=2 didn't pan out in my box: it did take more than 300s but didn't need the the much larger one needed on GH. I will investigate that separately.

@pjanotti
Copy link
Contributor Author

The extra long duration is due to the exporters test under cmd/otelcontribcol, in my box the exporter tests take around 70% of the time to run all tests in cmd/otelcontribcol.

djaglowski pushed a commit that referenced this pull request Nov 30, 2023
…ig on all platforms (#29553)

**Description:**
Discovered via
#29532 (comment).
The receiver configuration is not the same between Windows and the other
OSes. This is not the practice for other components and make hard to
write utilities dealing with the configuration on multiple platforms.

The change moves some types to sources that are built for all OSes while
preserving the behavior of the receiver.

**Link to tracking Issue:**
N/A

**Testing:**
Local Windows and Linux, plus CI on my fork.

**Documentation:**
N/A
@pjanotti pjanotti marked this pull request as ready for review November 30, 2023 19:05
@djaglowski djaglowski merged commit 5343a85 into open-telemetry:main Dec 1, 2023
91 checks passed
@github-actions github-actions bot added this to the next release milestone Dec 1, 2023
@pjanotti pjanotti deleted the part-04-of-issue-28679 branch December 1, 2023 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd/otelcontribcol otelcontribcol command Run Windows Enable running windows test on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-enable Windows tests disabled due to #11451
4 participants