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

Enable circleci to run tests on windows #976

Merged
merged 1 commit into from
May 21, 2020

Conversation

bogdandrutu
Copy link
Member

No description provided.

@codecov-io
Copy link

codecov-io commented May 15, 2020

Codecov Report

Merging #976 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #976      +/-   ##
==========================================
+ Coverage   85.65%   85.67%   +0.01%     
==========================================
  Files         188      188              
  Lines       13149    13149              
==========================================
+ Hits        11263    11265       +2     
+ Misses       1435     1434       -1     
+ Partials      451      450       -1     
Impacted Files Coverage Δ
receiver/opencensusreceiver/octrace/opencensus.go 93.33% <0.00%> (+3.33%) ⬆️

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 1076b51...b0f8a1f. Read the comment docs.

Copy link
Member

@james-bebbington james-bebbington left a comment

Choose a reason for hiding this comment

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

Note that when running the go test command successfully on Windows, we still have a number of tests that don't run as per #854.

In addition to that, note that the testbed processes don't get terminated properly on Windows so you will encounter a stall until that's fixed (I wrote a fix/hack for that locally that I can upload when I get a chance).

.circleci/config.yml Outdated Show resolved Hide resolved
@james-bebbington
Copy link
Member

Thanks for looking into this btw!

@james-bebbington
Copy link
Member

james-bebbington commented May 18, 2020

Note that when running the go test command successfully on Windows, we still have a number of tests that don't run as per #854.

In addition to that, note that the testbed processes don't get terminated properly on Windows so you will encounter a stall until that's fixed (I wrote a fix/hack for that locally that I can upload when I get a chance).

I created a PR to fix the unit tests that are failing on Windows: #983


There is still an issue with running the testbed on Windows though. SIGTERM is not supported on Windows, so the agent process will get terminated via SIGKILL (the code currently waits 10s to perform a SIGKILL if SIGTERM fails - it would be straightforward to clean this code up so that it calls SIGKILL straight away on Windows).

If the test succeeds, everything works okay. But in the case that a test fails due to CPU/Mem Usage, the agent is forcefully killed and the load generator receives an "Unavailable" gRPC response. It then goes into an exponential backoff retry strategy with a 15m max timeout, before checking if a signal has been sent to stop generating data. This is what I thought was a "stall" as mentioned earlier. I guess the SIGTERM must result in a nicer shutdown of the gRPC channel that prevents similar issues on other OSs (I didn't look into the details of how this works).

This could probably be fixed by just running the generateMetrics function in a separate go routine. I can come back to this later if we need a fix, but I imagine its not necessary to run the testbed tests on Windows anyway?

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@bogdandrutu
Copy link
Member Author

@james-bebbington for the moment I am not running testbed on windows but this should be good enough.

@bogdandrutu
Copy link
Member Author

@dmitryax PTAL

- run:
name: Upgrade golang
shell: powershell.exe
command: choco upgrade golang --version=1.14.3
Copy link
Member Author

Choose a reason for hiding this comment

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

Would be good if I can cache this operation since it takes a while. If anyone knows how, I would be happy to review a followup PR :)

Copy link
Member

@james-bebbington james-bebbington May 21, 2020

Choose a reason for hiding this comment

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

A few of the tests are abnormally slow on Windows as well, I'll try to take a look into that if I get a chance later.

Edit: This PR should shave off ~1 min: #1009

@bogdandrutu bogdandrutu merged commit 6c27ad7 into open-telemetry:master May 21, 2020
@bogdandrutu bogdandrutu deleted the windows branch May 21, 2020 14:16
wyTrivail pushed a commit to mxiamxia/opentelemetry-collector that referenced this pull request Jul 13, 2020
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
* Remove grpctrace package

This is being moved to the contrib repo with
open-telemetry/opentelemetry-go-contrib#189
as part of open-telemetry#976.

* Update Changelog

* Remove the grpc example

Moved to contrib repo

* Fix spelling error

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

Co-authored-by: aunshc <aunshc@splunk.com>
Co-authored-by: Dmitrii Anoshin <anoshindx@gmail.com>
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
swiatekm pushed a commit to swiatekm/opentelemetry-collector that referenced this pull request Oct 9, 2024
* make the service port for the webhook configurable

* service port

* version bump

* version bump the examples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants