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

Fixed ballast test cases by using cross-platform lib #61

Merged
merged 2 commits into from
Jun 27, 2019
Merged

Fixed ballast test cases by using cross-platform lib #61

merged 2 commits into from
Jun 27, 2019

Conversation

owais
Copy link
Contributor

@owais owais commented Jun 26, 2019

This should make the test work on the following platforms:

  • FreeBSD i386/amd64/arm
  • Linux i386/amd64/arm
  • Windows/amd64
  • Darwin i386/amd64
  • OpenBSD amd64

@codecov-io
Copy link

codecov-io commented Jun 26, 2019

Codecov Report

Merging #61 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
- Coverage   69.67%   69.58%   -0.09%     
==========================================
  Files          86       86              
  Lines        5606     5606              
==========================================
- Hits         3906     3901       -5     
- Misses       1481     1485       +4     
- Partials      219      220       +1
Impacted Files Coverage Δ
cmd/occollector/app/collector/collector.go 79.26% <0%> (-3.05%) ⬇️

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 2ad0e1d...7c5664b. Read the comment docs.

@tigrannajaryan
Copy link
Member

tigrannajaryan commented Jun 26, 2019

Strange that it shows reduction in codecov. Also the build result does not seem to be reported from Travis to Github properly.

@owais
Copy link
Contributor Author

owais commented Jun 26, 2019

@tigrannajaryan it had a typo linu/amd64 instead of linux so the test case never ran :)

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

Change itself looks good, need to check if the counter for Darwin is correct per offline conversation.

This should make the test work on the following platforms:

- FreeBSD i386/amd64/arm
- Linux i386/amd64/arm
- Windows/amd64
- Darwin i386/amd64
- OpenBSD amd64

Fixes #58
@owais
Copy link
Contributor Author

owais commented Jun 26, 2019

Update

The test was working across platforms but not so reliably on Travis CI. It seems the testsuite and the Go compiler toolchain consuming very different amount of memory across different environments. We could get it to work but I don't think it would be worth the effort especially considering it would likely break in new environments and we would have to maintain it. I think it's better to have this as a standalone test that spins up a production collector instance and monitors it's memory usage. @pjanotti suggested we use the perf testbed for this and I think that makes a lot of sense. As a result this PR is just disabling the ballast test for now. We'll remove it once it is implemented as a perf test.

@tigrannajaryan @songy23

The ballast test case unreliable across platforms so had to remove it.
It'll be replaced with an external test case as leveraging the
performance testbed.

The latest version was working on Linux and macOS laptops but had a hard
time on Travis CI. It seems the only reliable way to test ballast is to
spin up an external collector process and monitor it's memory usage.
This will be implemented as part one of the perf testbed cases.
@owais
Copy link
Contributor Author

owais commented Jun 26, 2019

I actually removed it completely to get rid of the dependency as there was no point in carrying it for nothing.

@tigrannajaryan
Copy link
Member

I think it's better to have this as a standalone test that spins up a production collector instance and monitor's it's memory usage. @pjanotti suggested we use the perf testbed for this and I think that makes a lot of sense.

I agree. This makes a lot of sense as an E2E test.

@pjanotti pjanotti merged commit d05ec08 into open-telemetry:master Jun 27, 2019
@owais owais deleted the fix-ballast-test branch June 27, 2019 01:38
pjanotti referenced this pull request in pjanotti/opentelemetry-service Jun 27, 2019
@flands flands added this to the 0.1.0 milestone Jun 28, 2019
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
* Add function_ref

* Add function_ref
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.

6 participants