Skip to content
This repository has been archived by the owner on Feb 9, 2022. It is now read-only.

Update dependency to fix goroutine proliferation in zipkin-go #1515

Merged
merged 1 commit into from
Oct 26, 2017

Conversation

mandarjog
Copy link
Contributor

@mandarjog mandarjog commented Oct 26, 2017

This PR fixes goroutine proliferation issue with openzipkin-go.
istio/old_issues_repo#100 (comment)

After the PR is upstreamed, we will update the dependency.

mandarjog/zipkin-go-opentracing#1

Release note:

NONE

This change is Reviewable

@jmuk
Copy link
Contributor

jmuk commented Oct 26, 2017

collected some data points for this PR -- in short, this seems solving the reported problem (istio/old_issues_repo#100).

My setup is to simply deploy the istio-auth (without zipkin pod), bookinfo sample, and run wrk to the productpage for 5 minutes.

The red line in the following graph shows the heap alloc bytes data (tracked by prometheus), with this PR (green is proxy -- unrelated, same in the following graphs):
screenshot 2017-10-26 at 14 17 39

Without this PR, typically heap allocation looks like
screenshot 2017-10-26 at 14 18 00

note that the spiky pattern happens after wrk run finishes -- still using memory for zipkin, we guess.

The number of goroutines,
screenshot 2017-10-26 at 14 06 38

Still have some increases since gRPC creates a goroutine for every request, but actually, the goroutine count looks like as follows without this PR
screenshot 2017-10-26 at 14 18 56

and note that, with the scale of the latter graph, the former goroutine "increase" looks like a flat line.

@jmuk
Copy link
Contributor

jmuk commented Oct 26, 2017

/lgtm

@douglas-reid
Copy link
Contributor

/lgtm

@douglas-reid
Copy link
Contributor

/approve

@istio-merge-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: douglas-reid, jmuk

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@istio-merge-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@istio-merge-robot
Copy link

Automatic merge from submit-queue.

@istio-merge-robot istio-merge-robot merged commit e495f82 into istio:master Oct 26, 2017
@codecov
Copy link

codecov bot commented Oct 26, 2017

Codecov Report

Merging #1515 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1515   +/-   ##
=======================================
  Coverage   84.58%   84.58%           
=======================================
  Files         122      122           
  Lines       11999    11999           
=======================================
  Hits        10149    10149           
  Misses       1648     1648           
  Partials      202      202

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 bbd5e27...6e2fca0. Read the comment docs.

mandarjog added a commit to mandarjog/mixer that referenced this pull request Oct 26, 2017
…1515)

Automatic merge from submit-queue.

Update dependency to fix goroutine proliferation in zipkin-go

This PR fixes goroutine proliferation issue with openzipkin-go.
istio/old_issues_repo#100 (comment)

After the PR is upstreamed, we will update the dependency.

mandarjog/zipkin-go-opentracing#1

**Release note**:

```release-note
NONE
```
mandarjog added a commit that referenced this pull request Oct 31, 2017
…#1516)

Automatic merge from submit-queue.

Update dependency to fix goroutine proliferation in zipkin-go

This PR fixes goroutine proliferation issue with openzipkin-go.
istio/old_issues_repo#100 (comment)

After the PR is upstreamed, we will update the dependency.

mandarjog/zipkin-go-opentracing#1

**Release note**:

```release-note
NONE
```
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants