Added end to end test for mem ballast #80

owais · 2019-07-01T17:13:51Z

This commit adds an end to end test to verify performance gain acheived
with memory ballast

It mainly checks the CPU usage and does not test against memory usage.
I'll be submitting another PR that will specifically test memory
behaviour and ensure the ballast does not actually consume memory.

TestNoBackend10kSPS test was very reliably consuming >20 CPU on my
machine so I increased the accepted values to 30 for this test.

songy23

LGTM

tigrannajaryan · 2019-07-01T17:19:20Z

testbed/tests/perf_test.go

@@ -188,3 +158,81 @@ func Test1000SPSWithAttributes(t *testing.T) {
 		})
 	}
 }
+
+
+func Test1000SPSWithAttributes(t *testing.T) {


The test with ballast specifies the same expectedMaxCPU as the one without ballast. However test results that you posted show that with ballast it actually uses less CPU. So, perhaps tighten a bit expectedMaxCPU in this test? I suggest you leave about 30% of slack in CPU (that was my rule of thumb when writing other perf tests).

I think I did reduce the numbers before creating the PR. Probably I missed something in the first push. In any case, updated the numbers and reduced them a further.

tigrannajaryan · 2019-07-01T17:19:40Z

testbed/tests/results/TESTRESULTS.md

+TestBallast1000SPSWithAttributes/0*0bytes|PASS  |     12s|    21.9|    25.3|         73|        114|     10000|         10000
+TestBallast1000SPSWithAttributes/100*50bytes|PASS  |     12s|    60.2|    68.3|        498|        847|     10000|         10000
+TestBallast1000SPSWithAttributes/10*1000bytes|PASS  |     12s|    41.0|    45.3|        354|        576|      9990|          9990
+TestBallast1000SPSWithAttributes/20*5000bytes|PASS  |     12s|    64.4|    74.0|        780|       1079|      9990|          9990


This is a nice CPU improvement.

codecov-io · 2019-07-01T17:25:47Z

Codecov Report

❗ No coverage uploaded for pull request base (master@e0e3e0b). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #80   +/-   ##
=========================================
  Coverage          ?   69.77%           
=========================================
  Files             ?      101           
  Lines             ?     6392           
  Branches          ?        0           
=========================================
  Hits              ?     4460           
  Misses            ?     1698           
  Partials          ?      234

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 e0e3e0b...f2dac10. Read the comment docs.

* Update the Agent architecture graph. * Fix a typo.

tigrannajaryan · 2019-07-01T22:48:09Z

@owais Build status is not reported from Travis. You may want to restart.

owais · 2019-07-01T22:58:19Z

Looks like a bug. The build has passed: https://travis-ci.org/open-telemetry/opentelemetry-service/builds/552958842?utm_source=github_status&utm_medium=notification

I don't have the option to restart builds for this repo and it's PRs.

tigrannajaryan · 2019-07-01T23:03:52Z

It will restart automatically after you merged the other PR and rebase this one.

tigrannajaryan · 2019-07-02T00:00:37Z

Build failed because of #43

owais · 2019-07-02T00:03:55Z

Yea, I've been seeing quite a few random failures. Will rebase after we merge #85

testbed/tests/results/TESTRESULTS.md

tigrannajaryan · 2019-07-03T00:08:49Z

@owais did you make changes or just rebased? If you need to rebase it is better to use "Update branch" button in Github. It reruns CI, but does not invalidate reviews (unlike rebasing and pushing the branch manually, which requires re-approval). So you don't need to ask for reapproval in that case.

owais · 2019-07-03T00:10:53Z

@tigrannajaryan I rebased. Update branch features ends up creating a merge commit as it merges master into the branch. Is that acceptable?

This commit adds an end to end test to verify performance gain acheived with memory ballast It mainly checks the CPU usage and does not test against memory usage. I'll be submitting another PR that will specifically test memory behaviour and ensure the ballast does not actually consume memory. `TestNoBackend10kSPS` test was very reliably consuming >20 CPU on my machine so I increased the accepted values to 30 for this test.

tigrannajaryan · 2019-07-03T00:21:51Z

Update branch features ends up creating a merge commit as it merges master into the branch. Is that acceptable?

@owais I don't think this is true. If I remember correctly when you merge the PR, the commit is squashed and rebased, no merge commit is created.

pjanotti · 2019-07-03T16:01:55Z

CI hit #59

owais · 2019-07-03T16:06:18Z

@owais I don't think this is true. If I remember correctly when you merge the PR, the commit is squashed and rebased, no merge commit is created.

I see. I meant it creates a merge commit in the branch when updating from master but that wouldn't matter if the repo has only "squash and merge" available.

Update again. Thanks.

* add support for Link. * add AddLink to mockSpan * update api documentation.

owais requested review from bogdandrutu, flands, pjanotti, songy23 and tigrannajaryan as code owners July 1, 2019 17:13

songy23 previously approved these changes Jul 1, 2019

View reviewed changes

tigrannajaryan reviewed Jul 1, 2019

View reviewed changes

owais dismissed songy23’s stale review via d72250c July 1, 2019 17:27

tigrannajaryan previously approved these changes Jul 1, 2019

View reviewed changes

pjanotti previously approved these changes Jul 1, 2019

View reviewed changes

owais dismissed stale reviews from pjanotti and tigrannajaryan via 7106783 July 1, 2019 17:41

pjanotti referenced this pull request in pjanotti/opentelemetry-service Jul 1, 2019

Update the Agent architecture graph. (#80)

38173a3

* Update the Agent architecture graph. * Fix a typo.

tigrannajaryan previously approved these changes Jul 1, 2019

View reviewed changes

owais dismissed tigrannajaryan’s stale review via 2adb37c July 1, 2019 23:42

flands assigned owais Jul 1, 2019

flands added this to the 0.1.0 milestone Jul 1, 2019

tigrannajaryan previously approved these changes Jul 2, 2019

View reviewed changes

owais dismissed tigrannajaryan’s stale review via d7d3398 July 2, 2019 20:42

tigrannajaryan reviewed Jul 2, 2019

View reviewed changes

testbed/tests/results/TESTRESULTS.md Outdated Show resolved Hide resolved

tigrannajaryan previously approved these changes Jul 3, 2019

View reviewed changes

owais dismissed tigrannajaryan’s stale review via 541c6f3 July 3, 2019 00:15

Merge branch 'master' into e2e-ballast-test

0bc36a4

Merge branch 'master' into e2e-ballast-test

ac5dec6

tigrannajaryan approved these changes Jul 3, 2019

View reviewed changes

Merge branch 'master' into e2e-ballast-test

f2dac10

pjanotti approved these changes Jul 3, 2019

View reviewed changes

pjanotti merged commit 127ea97 into open-telemetry:master Jul 3, 2019

owais deleted the e2e-ballast-test branch July 3, 2019 17:39

owais mentioned this pull request Jul 3, 2019

REQUEST: New membership for owais open-telemetry/community#146

Closed

6 tasks

owais mentioned this pull request Sep 7, 2019

Nominating @owais as approver for Agent and Collector open-telemetry/community#172

Merged

MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021

add support for Link. (open-telemetry#80)

c70cb29

* add support for Link. * add AddLink to mockSpan * update api documentation.

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

Update linux env file for SPLUNK_MEMORY_TOTAL_MIB (open-telemetry#80)

cad9178

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added end to end test for mem ballast #80

Added end to end test for mem ballast #80

owais commented Jul 1, 2019

songy23 left a comment

tigrannajaryan Jul 1, 2019 •

edited

Loading

owais Jul 1, 2019

tigrannajaryan Jul 1, 2019

codecov-io commented Jul 1, 2019 •

edited

Loading

tigrannajaryan commented Jul 1, 2019

owais commented Jul 1, 2019

tigrannajaryan commented Jul 1, 2019

tigrannajaryan commented Jul 2, 2019 •

edited

Loading

owais commented Jul 2, 2019

tigrannajaryan commented Jul 3, 2019

owais commented Jul 3, 2019

tigrannajaryan commented Jul 3, 2019

pjanotti commented Jul 3, 2019

owais commented Jul 3, 2019

Added end to end test for mem ballast #80

Added end to end test for mem ballast #80

Conversation

owais commented Jul 1, 2019

songy23 left a comment

Choose a reason for hiding this comment

tigrannajaryan Jul 1, 2019 • edited Loading

Choose a reason for hiding this comment

owais Jul 1, 2019

Choose a reason for hiding this comment

tigrannajaryan Jul 1, 2019

Choose a reason for hiding this comment

codecov-io commented Jul 1, 2019 • edited Loading

Codecov Report

tigrannajaryan commented Jul 1, 2019

owais commented Jul 1, 2019

tigrannajaryan commented Jul 1, 2019

tigrannajaryan commented Jul 2, 2019 • edited Loading

owais commented Jul 2, 2019

tigrannajaryan commented Jul 3, 2019

owais commented Jul 3, 2019

tigrannajaryan commented Jul 3, 2019

pjanotti commented Jul 3, 2019

owais commented Jul 3, 2019

tigrannajaryan Jul 1, 2019 •

edited

Loading

codecov-io commented Jul 1, 2019 •

edited

Loading

tigrannajaryan commented Jul 2, 2019 •

edited

Loading