Zipkin end to end example by rshriram · Pull Request #993 · envoyproxy/envoy

rshriram · 2017-05-18T19:49:59Z

This PR describes how to setup Zipkin tracing in Envoy with a simple end to end example (reuses the front proxy sandbox). PR #962 and the current one address #628

rshriram · 2017-05-18T19:57:49Z

cc @adriancole @timperrett @fabolive

mattklein123 · 2017-05-18T19:59:46Z

💯 thanks @rshriram for doing this. @RomanDzhabarov @junr03 @ccaraman can you also take a look.

ccaraman · 2017-05-18T21:32:07Z

+needed to correlate the span with other related spans (e.g., the trace ID).
+
+One of the most important benefits of tracing from Envoy is that it will take care of
+propagating the traces to to the Zipkin service cluster. However, in order to fully take advantage


nit: duplicate to

ccaraman · 2017-05-18T21:35:36Z

+        for header in TRACE_HEADERS_TO_PROPAGATE:
+            if header in request.headers:
+                headers[header] = request.headers[header]
+        ret = requests.get("http://localhost:9000/trace/2", headers=headers)


do you want to use service_number from within the get instead of 2?

You mean service_number + 1 ? Because the entire code is specialized to service version == 1

ccaraman

LGTM modulo a few nits

mattklein123 · 2017-05-18T22:25:58Z

 fulfilled by them, information about http ingress, and a plethora of other useful
 stats.

+Zipkin Tracing


nit: This page is absolutely huge. Can we potentially break this into multiple pages, one for each example? You didn't cause this problem but would be great to fix now.

rshriram · 2017-05-18T22:29:08Z

Yeah sure.

On Thu, May 18, 2017 at 6:26 PM Matt Klein ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In docs/install/sandboxes.rst <#993 (comment)>: > @@ -234,6 +234,87 @@ Notice that we can get the number of members of upstream clusters, number of req fulfilled by them, information about http ingress, and a plethora of other useful stats. +Zipkin Tracing nit: This page is absolutely huge. Can we potentially break this into multiple pages, one for each example? You didn't cause this problem but would be great to fix now. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#993 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH0qdzMpb4vepURsziC8nmIYebXiEMWWks5r7MV6gaJpZM4NftoQ> .

-- ~shriram

RomanDzhabarov · 2017-05-18T20:59:23Z

+--------------
+
+The Zipkin tracing demonstrates Envoy's :ref:`request tracing <arch_overview_tracing>`
+capabilities using `Zipkin <http://zipkin.io/>`_ as the trace provider. This sandbox


nit: tracing provider.

RomanDzhabarov · 2017-05-18T22:53:40Z

+    'X-B3-ParentSpanId',
+    'X-B3-Sampled',
+    'X-B3-Flags',
+    'X-B3-Envoy'


is X-B3-Envoy required?

RomanDzhabarov · 2017-05-18T22:54:18Z

 app = Flask(__name__)

+TRACE_HEADERS_TO_PROPAGATE = [
+    'X-Client-Trace-Id',


that's not required to propagate, essentially it's only used on the first hop so that client traces can be merged with the backend traces

codefromthecrypt · 2017-05-18T23:30:45Z

Excited but bad personal timing as headed for a flight w/o laptop. I pinged some folks to have a look. Thanks!

RomanDzhabarov · 2017-05-18T23:37:47Z

+Zipkin Tracing
+--------------
+
+The Zipkin tracing demonstrates Envoy's :ref:`request tracing <arch_overview_tracing>`


can you reference this as an example of zipkin tracing configuration from https://lyft.github.io/envoy/docs/operations/faq/overview.html ?

devinsba · 2017-05-19T02:38:27Z

+    if int(os.environ['SERVICE_NAME']) == 1 :
+        for header in TRACE_HEADERS_TO_PROPAGATE:
+            if header in request.headers:
+                headers[header] = request.headers[header]


Normally in zipkin apps, the client creates a new span that is a child of the current span and changes the headers appropriately before making requests to another service.

That is true in the normal case. The idea behind Envoy doing the tracing for you is that the application does not have to do any tracing nor does it have to spend resources propagating the traces to a collector server. It eliminates the complexity of adding additional libraries for the purpose of tracing and dealing with all the additional code.

Secondly, the idea is that Envoy's presence is transparent to the application. The amount of time spent in Envoy should be minuscule (if its sizeable, then there is something wrong with Envoy design :) ).

This is the Envoy model btw. I am just adding examples to go with the model. :).

devinsba · 2017-05-19T02:58:57Z

From a zipkin devs point of view, the traces this example creates are not what I would expect to see.

I would expect to see server spans for the envoy parts of these requests, maybe I'm in the minority on this though. I would want to see how much of the request time could be attributed to envoy and how much of it is the apps themselves

EDIT: To clarify, in my opinoin hitting the example endpoint should produce 4 spans in the trace that ends up in zipkin, not the 2 it currently does

RomanDzhabarov · 2017-05-22T18:53:27Z

@rshriram there are some pending comments on this one
I'll do another pass once it's addressed and let's merge it.

rshriram · 2017-05-22T21:48:32Z

@RomanDzhabarov @goaway the last two commits address your nits. I also split the sandbox file into smaller files as per @mattklein123 's suggestion.

RomanDzhabarov · 2017-05-22T22:01:01Z

that's awesome, thanks @rshriram

@htuch ready for final pass

htuch

Thanks @rshriram, this reads well. One thing I did wonder about is whether the setup steps have been independently carried out by someone to ensure that there were no local environment dependencies that crept in. Otherwise, this looks good to ship after the last few nits are taken care of.

htuch · 2017-05-23T05:18:48Z

+
+The gRPC bridge sandbox is an example usage of Envoy's
+:ref:`gRPC bridge filter <config_http_filters_grpc_bridge>`.
+Included in the sandbox is a gRPC in memory Key/Value store with a Python HTTP


Nit: in-memory

htuch · 2017-05-23T05:19:57Z

+Sending requests to the Key/Value store
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+To use the python service and sent gRPC requests::


Nit: "To use the Python service and send"

htuch · 2017-05-23T05:20:10Z

+  $ docker-compose exec python /client/client.py get foo
+  bar
+
+Locally building a docker image with an envoy binary


Nit: Docker, Envoy

htuch · 2017-05-23T05:20:49Z

+Locally building a docker image with an envoy binary
+----------------------------------------------------
+
+The following steps guide you through building your own envoy binary, and


Nit: prefer Envoy and Ubuntu everywhere, proper nouns and all.

Docker everywhere too, unless referring to the literal docker command.

htuch · 2017-05-23T05:22:11Z

+
+  $ pwd
+  src/envoy
+  $ ./ci/run_envoy_docker.sh './ci/do_ci.sh bazel.dev'


Do you really want to use bazel.dev here? I think this is generally only useful for developer iteration productivity. In general you want either bazel.release or bazel.debug, depending on whether performance matters. That way you get symbols for sure.

htuch · 2017-05-23T05:23:44Z

+
+  $ pwd
+  src/envoy/
+  $ docker build -f ci/Dockerfile-envoy-image -t envoy .


This seems to be dupe of the above file suffix, can you de-dupe?

Oh shux.. good catch.. removed that stuff from grpc_bridge.rst

rshriram · 2017-05-23T10:52:06Z

The only thing I wrote was the zipkin stuff. The rest was already there and all I did was to split the large file into smaller docs. I have verified the zipkin steps. The front proxy works for sure (that's how I started with Envoy in the first place). Not sure about gRPC though. That said, I don't mind fixing the nits in current docs :). Will take another pass.

On Tue, May 23, 2017 at 1:30 AM htuch ***@***.***> wrote: ***@***.**** commented on this pull request. In docs/install/sandboxes/grpc_bridge.rst <#993 (comment)>: > +To use the python service and sent gRPC requests:: + + $ pwd + ~/src/envoy/examples/grpc-bridge + # set a key + $ docker-compose exec python /client/client.py set foo bar + setf foo to bar + + # get a key + $ docker-compose exec python /client/client.py get foo + bar + +Locally building a docker image with an envoy binary +---------------------------------------------------- + +The following steps guide you through building your own envoy binary, and Docker everywhere too, unless referring to the literal docker command. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#993 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH0qd0QUYGb03SJ6Atl-f67TI7M_qm1-ks5r8m8FgaJpZM4NftoQ> .

-- ~shriram

rshriram · 2017-05-23T17:38:18Z

@htuch addressed nits. With regard to validating docker build/gRPC/front-proxy, these examples were already present. I just added the zipkin_tracing.rst.

Automatic merge from submit-queue. make mixer cluster name configurable Please see istio/api#358 for context. Essentially remove hardcoded "mixer_server" and allow users to override the name, in addition to allowing separate clusters for check vs reports so that it makes it easier to load balance/manage the system.

**Description** according to the openai openai spec, there should be an index in the chat completions chunk data. This fixes it and avoids serializing nested empty structs. See https://github.com/openai/openai-openapi/tree/manual_spec --------- Signed-off-by: Adrian Cole <adrian@tetrate.io> Signed-off-by: Adrian Cole <64215+codefromthecrypt@users.noreply.github.com> Co-authored-by: Adrian Cole <adrian@tetrate.io>

Shriram Rajagopalan added 5 commits May 14, 2017 18:33

document zipkin headers that services need to propagate

4bdd92a

generalize tracing architecture discussion

fe114f4

fix typo

75e8773

Zipkin sandbox - example

6acbcfe

Merge remote-tracking branch 'upstream/master' into zipkin_example

2d088df

increase number of config files in config_test

153005e

ccaraman reviewed May 18, 2017

View reviewed changes

ccaraman previously approved these changes May 18, 2017

View reviewed changes

typos

7b63edd

rshriram dismissed ccaraman’s stale review via 7b63edd May 18, 2017 22:20

Merge remote-tracking branch 'upstream/master' into zipkin_example

6143a1d

mattklein123 reviewed May 18, 2017

View reviewed changes

RomanDzhabarov reviewed May 18, 2017

View reviewed changes

devinsba reviewed May 19, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into zipkin_example

ad244c5

RomanDzhabarov assigned RomanDzhabarov and goaway May 22, 2017

Shriram Rajagopalan added 5 commits May 22, 2017 17:10

split sandbox into smaller files

23a81f7

Merge remote-tracking branch 'upstream/master' into zipkin_example

fa63435

toc depth to 1

11f5792

address nits

057e2c9

add zipkin faq

b3743a2

RomanDzhabarov previously approved these changes May 22, 2017

View reviewed changes

htuch reviewed May 23, 2017

View reviewed changes

Shriram Rajagopalan added 2 commits May 23, 2017 13:36

nits from htuch

8910af7

Merge remote-tracking branch 'upstream/master' into zipkin_example

f1ad8b8

rshriram dismissed RomanDzhabarov’s stale review via f1ad8b8 May 23, 2017 17:37

htuch approved these changes May 23, 2017

View reviewed changes

htuch merged commit 115434a into envoyproxy:master May 23, 2017

rshriram mentioned this pull request Jun 8, 2017

Zipkin follow ups #960

Closed

3 tasks

rshriram deleted the zipkin_example branch August 14, 2017 18:11

mattklein123 unassigned goaway and RomanDzhabarov Sep 14, 2017

Conversation

rshriram commented May 18, 2017

Uh oh!

rshriram commented May 18, 2017

Uh oh!

mattklein123 commented May 18, 2017

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!

ccaraman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshriram commented May 18, 2017 via email

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!

codefromthecrypt commented May 18, 2017 via email

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!

devinsba commented May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RomanDzhabarov commented May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rshriram commented May 22, 2017

Uh oh!

RomanDzhabarov commented May 22, 2017

Uh oh!

htuch 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!

rshriram commented May 23, 2017 via email

Uh oh!

rshriram commented May 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

devinsba commented May 19, 2017 •

edited

Loading

RomanDzhabarov commented May 22, 2017 •

edited

Loading