Switch Mixer tracing package to use jaeger library. by douglas-reid · Pull Request #1693 · istio/istio

douglas-reid · 2017-11-13T19:53:34Z

What this PR does / why we need it:

This PR switches Mixer from using the openzipkin tracing library to the jaeger tracing library for tracing Mixer functions.

This switch accomplishes a few key items:

Adds Mixer support for Jaeger backends
Removes the need for a custom fork of the OpenZipkin library
Adds better flag support for tracing control
Enables development of Jaeger adapter (library conflict with OpenZipkin library).

It deprecates the --traceOutput flag and replaces it with several additional flags:

zipkinURL,
jaegerURL
logTraceSpans

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Fixes istio/old_mixer_repo#1352

Special notes for your reviewer:

Release note:

Add Jaeger support to Mixer.

douglas-reid · 2017-11-13T22:35:35Z

/retest all

mandarjog

What is the behavior of jaeger http collector on a slow outbound transport? We have to ensure that spans are discarded correctly.

mandarjog · 2017-11-13T22:11:20Z

install/kubernetes/istio-one-namespace.yaml

Why change command line params?
Can we keep traceOutput to mean zipkin and only add a new flag for Jaeger.

A couple of concerns:

traceOutput was badly overloaded before

it implies a default stance towards zipkin, which I don't think we necessarily want

i think traceOutput makes it less obvious what the flag value should be

Is your concern backwards compatibility of YAMLs? If so, one option would be to maintain support for traceOutput, mark it as deprecated, and do the value conversion in server.go.

Yes, backwards compat for yamls and the command line options in general.
Let us make traceOutput an alias to zipkin then and mark as deprecated.

Added alias and deprecation.

mandarjog · 2017-11-13T22:15:22Z

mixer/pkg/pool/goroutine.go

implements io.Closer

added to comment.

mandarjog · 2017-11-13T23:45:48Z

mixer/cmd/server/cmd/server.go

We should use standard url format instead.
zipkin://user:pass@host

fwiw, this is not how the backend libraries take the args. So this would mean additional parsing on our part. Another potential consideration: USER and PASS mounted as secrets. Thoughts?

removed basic auth support altogether -- it was kinda ugly (prefer Istio for securing access). we can add it back if needed later.

mandarjog · 2017-11-13T23:46:21Z

mixer/cmd/server/cmd/server.go

standard url for jaeger.

douglas-reid · 2017-11-14T20:03:51Z

PTAL.

mandarjog · 2017-11-17T03:59:28Z

@jmuk can you ensure that this branch has the good memory characteristics that we fixed for openzipkin-go ? If that checks out this is great.

jmuk · 2017-11-17T07:24:54Z

Tried a bit, I don't see any memory issues like the ones in openzipkin-go.

mandarjog · 2017-11-17T11:38:43Z

Note that with this PR we are using a an actively developed library that support both Jaeger and zipkin. This is great!

/lgtm

douglas-reid · 2017-11-18T00:02:54Z

/retest all

douglas-reid · 2017-11-18T17:46:25Z

/retest

douglas-reid · 2017-11-18T21:51:12Z

/retest

douglas-reid · 2017-11-27T18:00:53Z

/retest

istio-merge-robot · 2017-11-27T18:42:11Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @douglas-reid @mandarjog

codecov · 2017-11-27T19:27:10Z

Codecov Report

Merging #1693 into master will decrease coverage by 0.06%.
The diff coverage is 48.93%.

@@            Coverage Diff             @@
##           master    #1693      +/-   ##
==========================================
- Coverage   81.23%   81.17%   -0.07%     
==========================================
  Files         191      185       -6     
  Lines       19344    18804     -540     
==========================================
- Hits        15715    15264     -451     
+ Misses       3173     3113      -60     
+ Partials      456      427      -29

Flag	Coverage Δ
#broker	`44.44% <ø> (-1.07%)`	⬇️
#mixer	`82.46% <48.93%> (-0.08%)`	⬇️
#pilot	`80.36% <ø> (-0.11%)`	⬇️
#security	`90.39% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
mixer/cmd/server/cmd/test_server.go	`0% <ø> (ø)`	⬆️
mixer/cmd/client/cmd/util.go	`61.92% <0%> (ø)`	⬆️
mixer/cmd/server/cmd/server.go	`0% <0%> (ø)`	⬆️
mixer/pkg/pool/goroutine.go	`100% <100%> (ø)`	⬆️
mixer/pkg/tracing/tracing.go	`78.57% <78.57%> (ø)`
mixer/adapter/prometheus/server.go	`89.83% <0%> (-5.09%)`	⬇️
pilot/platform/kube/inject/http.go
pilot/platform/kube/inject/configmap.go
broker/pkg/version/version.go
pilot/platform/kube/inject/initializer.go
... and 2 more

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 85983c2...a41b7f9. Read the comment docs.

douglas-reid · 2017-11-27T19:54:43Z

/test e2e-smoke

googlebot · 2017-11-27T23:14:38Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

mandarjog · 2017-11-28T01:38:03Z

/lgtm

istio-merge-robot · 2017-11-28T01:38:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mandarjog

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

Details

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

~~OWNERS~~ [mandarjog]

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 · 2017-11-28T04:21:48Z

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

istio-merge-robot · 2017-11-28T04:53:45Z

Automatic merge from submit-queue.

Automatic merge from submit-queue. Switch Mixer tracing package to use jaeger library. **What this PR does / why we need it**: This PR switches Mixer from using the openzipkin tracing library to the jaeger tracing library for tracing Mixer functions. This switch accomplishes a few key items: - Adds Mixer support for Jaeger backends - Removes the need for a custom fork of the OpenZipkin library - Adds better flag support for tracing control - Enables development of Jaeger adapter (library conflict with OpenZipkin library). It deprecates the `--traceOutput` flag and replaces it with several additional flags: - `zipkinURL`, - `jaegerURL` - `logTraceSpans` **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # Fixes istio/old_mixer_repo#1352 **Special notes for your reviewer**: **Release note**: ```release-note Add Jaeger support to Mixer. ```

istio-testing added the release-note label Nov 13, 2017

googlebot added the cla: yes label Nov 13, 2017

douglas-reid changed the title ~~WIP: Switch Mixer tracing package to use jaeger library.~~ Switch Mixer tracing package to use jaeger library. Nov 13, 2017

douglas-reid requested a review from mandarjog November 13, 2017 20:00

douglas-reid added the mixer label Nov 13, 2017

mandarjog reviewed Nov 13, 2017

View reviewed changes

douglas-reid force-pushed the switch-opentracing-lib branch 5 times, most recently from 7aea417 to 2024105 Compare November 14, 2017 19:59

istio-testing assigned mandarjog Nov 17, 2017

istio-testing added the lgtm label Nov 17, 2017

istio-merge-robot added the approved label Nov 17, 2017

yutongz added do-not-merge/post-submit and removed do-not-merge/post-submit labels Nov 17, 2017

yutongz added do-not-merge/post-submit and removed do-not-merge/post-submit labels Nov 22, 2017

douglas-reid force-pushed the switch-opentracing-lib branch from 2024105 to 9ae7567 Compare November 27, 2017 18:41

istio-merge-robot removed the lgtm label Nov 27, 2017

douglas-reid force-pushed the switch-opentracing-lib branch 2 times, most recently from e9e14a2 to 2384adc Compare November 27, 2017 18:44

googlebot added cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. and removed cla: yes labels Nov 27, 2017

douglas-reid and others added 6 commits November 27, 2017 15:15

Refactor tracing package to use jaeger library.

728d2e3

Update yamls

2e17ee3

update go dep files

fd8819a

address linter issues

7aa1e20

Refactor tracing package to use jaeger library.

76135bb

Address review comments

df9b289

douglas-reid force-pushed the switch-opentracing-lib branch from 9878a64 to df9b289 Compare November 27, 2017 23:21

Updating SHA logic for lulz

a41b7f9

istio-testing added the lgtm label Nov 28, 2017

douglas-reid added cla: human-approved and removed cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. labels Nov 28, 2017

istio-merge-robot merged commit 8743921 into istio:master Nov 28, 2017

douglas-reid deleted the switch-opentracing-lib branch February 3, 2018 07:25

Conversation

douglas-reid commented Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

douglas-reid commented Nov 13, 2017

Uh oh!

mandarjog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

douglas-reid commented Nov 14, 2017

Uh oh!

mandarjog commented Nov 17, 2017

Uh oh!

jmuk commented Nov 17, 2017

Uh oh!

mandarjog commented Nov 17, 2017

Uh oh!

douglas-reid commented Nov 18, 2017

Uh oh!

douglas-reid commented Nov 18, 2017

Uh oh!

douglas-reid commented Nov 18, 2017

Uh oh!

douglas-reid commented Nov 27, 2017

Uh oh!

istio-merge-robot commented Nov 27, 2017

Uh oh!

codecov bot commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

douglas-reid commented Nov 27, 2017

Uh oh!

googlebot commented Nov 27, 2017

Uh oh!

mandarjog commented Nov 28, 2017

Uh oh!

istio-merge-robot commented Nov 28, 2017

Uh oh!

istio-merge-robot commented Nov 28, 2017

Uh oh!

istio-merge-robot commented Nov 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

douglas-reid commented Nov 13, 2017 •

edited

Loading

codecov bot commented Nov 27, 2017 •

edited

Loading