istio / old_mixer_repo

Deprecated home of Istio's Mixer and its adapters, now in istio/istio's mixer dir
https://github.com/istio/istio/tree/master/mixer
Apache License 2.0
67 stars 93 forks source link

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

Closed mandarjog closed 6 years ago

mandarjog commented 6 years ago

This PR fixes goroutine proliferation issue with openzipkin-go. https://github.com/istio/issues/issues/100#issuecomment-339468537

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

https://github.com/mandarjog/zipkin-go-opentracing/pull/1

Release note:

NONE

This change is Reviewable

jmuk commented 6 years ago

collected some data points for this PR -- in short, this seems solving the reported problem (istio/issues#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 commented 6 years ago

/lgtm

douglas-reid commented 6 years ago

/lgtm

douglas-reid commented 6 years ago

/approve

istio-merge-robot commented 6 years ago

[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: - ~~[OWNERS](https://github.com/istio/mixer/blob/master/OWNERS)~~ [douglas-reid] 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 commented 6 years ago

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

istio-merge-robot commented 6 years ago

Automatic merge from submit-queue.

codecov[bot] commented 6 years ago

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.