Zipkin library by fabolive · Pull Request #692 · envoyproxy/envoy

fabolive · 2017-04-04T19:35:25Z

This PR adds a Zipkin library to the Envoy code base. Changes made to Envoy to use this library to support Zipkin tracing are in PR #700. This needs to be merged before PR #700.

cc: @RomanDzhabarov, @adriancole, @mattklein123

mattklein123 · 2017-04-04T19:39:08Z

💥 very excited for this! Will let @RomanDzhabarov take first pass.

rshriram · 2017-04-04T19:39:35Z

@htuch @rlazarus there is a source/zipkin folder. The code there is supposed to be a generic library. I am not sure whats the best way to bazel it up.

RomanDzhabarov · 2017-04-04T19:45:01Z

@fabolive this is very exciting. Is there any chance to split this into multiple self-contained PRs?

rshriram · 2017-04-04T19:47:37Z

@RomanDzhabarov one thing that we were discussing: the source/zipkin folder is a separate library. That could go in as its own PR. The rest is additions to http tracer. How does that sound?

rshriram · 2017-04-04T19:52:49Z

cross referencing #628

htuch · 2017-04-04T19:55:45Z

I'd follow other examples of using envoy_cc_library and writing BUILD rules to Bazelify, see https://github.com/lyft/envoy/blob/master/bazel/DEVELOPER.md

RomanDzhabarov · 2017-04-04T19:57:38Z

Yup, that high level separation sounds good.

fabolive · 2017-04-04T20:43:25Z

@RomanDzhabarov I will then re-purpose this PR.

rshriram · 2017-04-04T22:53:47Z

Just FYI, here is the front-proxy example with zipkin integration. https://gist.github.com/rshriram/15f05f530894eff3cebf527ca1ba5857

codefromthecrypt · 2017-04-05T01:50:29Z

prefer two XX

..
just kidding :P

codefromthecrypt

very good. I've only identified a few edge cases, which is pretty amazing considering this is the first pass!

codefromthecrypt · 2017-04-05T02:27:57Z

is this comment out-of-date? I don't see x-b3-envoy listed in the code.

It is out of date, indeed.

codefromthecrypt · 2017-04-05T02:34:19Z

timestamp and duration should only be set on server spans which are not client-originated (ex root spans which weren't resumed from headers)

openzipkin/openzipkin.github.io#49

In brave, we set a flag in the in-process struct representing the context called "shared" which is used to differentiate a client-originated root span from a server-originated one. (the former is shared, the latter is not). When a trace context is parsed from headers, shared is set to true. If it wasn't it defaults to false.

/** * True if we are contributing to a span started by another tracer (ex on a different host). * Defaults to false. * * <p>When an RPC trace is client-originated, it will be sampled and the same span ID is used for * the server side. However, the server shouldn't set span.timestamp or duration since it didn't * start the span. */ public abstract boolean shared();

We use this flag when creating the span (like it is here)

// don't report server-side timestamp on shared or incomplete spans if (shared && (flags & FLAG_SR) != 0) { span.timestamp(null).duration(null); }

@adriancole The code you highlighted above is setting the timestamp of the annotation, not the span's. I make sure that only the span initiator sets the timestamp and duration of a span, as explained here: http://zipkin.io/pages/instrumenting.html

lol yes.. you are right! eyes blurred

codefromthecrypt · 2017-04-05T02:36:51Z

endpoint is the proper name for this field (we haven't renamed the field in thrift, but all docs refer to it as endpoint iirc)

Fair enough.

codefromthecrypt · 2017-04-05T02:38:38Z

you can simplify this by only supporting BOOL and STRING as others are not used in practice in zipkin.

BOOL is a special case for identifying the peer address. for example, "ca", true, host=client address, not the local address.

https://github.com/openzipkin/zipkin-api/blob/master/thrift/zipkinCore.thrift#L226

codefromthecrypt · 2017-04-05T02:41:37Z

on all the core annotations "cs" "sr" "ss" "cr" as well normal binary annotations (string value), the host will be constant for the tracer, as it is identifying the local process. I can't see clearly if that's the case in the current variant of this code, so just sayin..

There are special annotations "ca" and "sa" which identify the peer (incoming client or downstream server), which are optional but useful if you know it. I made a comment elsewhere on that.

The current code guarantees that the same endpoint (identifying the envoy in question) is used for the annotations originating from the same envoy process.
Indeed, ca/sa might be useful.

fabolive · 2017-04-05T15:36:58Z

@RomanDzhabarov We have 2 PRs now. This one has been re-purposed to contain only the addition of a Zipkin library to the Envoy code base. PR #700, in turn, contains the changes to make Envoy use this library.

cc: @adriancole, @rshriram, @mattklein123

htuch · 2017-04-06T12:11:20Z

You can just skip the deps if empty.

htuch · 2017-04-06T12:11:44Z

If any of these are purely internal to the library and don't need exporting for the use of other libraries, you can move them into srcs.

codefromthecrypt · 2017-04-06T14:14:30Z

when an operation abends, is finish still called? if so, we'd usually put a string binary annotation of key "error" and value of the error message in the span

In Envoy, when a connection is recycled the connection manager checks for errors. If an error is caught, it calls span.setTag(error, true), before calling span.finish(), to indicate that an error has occurred. In my code, span.setTag(key, value) essentially adds a binary annotation to the span. So, the code behaves just like you were expecting.
setTag: https://github.com/amalgam8/envoy/blob/zipkin/source/zipkin/zipkin_core_types.cc#L231

codefromthecrypt · 2017-04-11T05:36:08Z

not sure if here or not, but should test that 128bit variant doesn't crash. All new instrumentation support 128-bit trace ids, so if you accept an inbound trace id, the choices are to truncate by dropping high bits, or better.. use all of them.

I have added a test to verify this behavior. Note that the Span class has a member named traceIdHigh_, plus a flag indicating whether or not it has been set. That's how a Span with a 128-bit trace id can be created. Currently, the code ignores the higher bits.
Here is the test I added:
https://github.com/amalgam8/envoy/blob/zipkin/test/zipkin/span_context_test.cc#L117

codefromthecrypt · 2017-04-11T05:37:08Z

presumably for json encoding bc thrift it is packed into a 32bit unsigned

Correct. Since the current implementation supports only JSON over HTTP, we are storing IP addresses as strings. When we support other transports, we will revisit that (and possibly a few other aspects of the core data structures).

codefromthecrypt · 2017-04-11T05:38:10Z

this assumes the only sort of child span is a client span. That's ok if that's the intent

Correct. In Envoy, that happens to be the case.

codefromthecrypt

good progress!

rshriram · 2017-04-11T12:04:27Z

I suggest using full names instead of shorthands. E.g. endpoint instead of ep. Annotation instead of ann.

rshriram · 2017-04-11T12:08:34Z

Micro -> US or if it's not too lengthy, might as well use Microseconds

rshriram · 2017-04-11T12:38:24Z

Do we need this copy constructor if it's just a one to one assignment of ints and pointers to const objects ?

fabolive · 2017-04-11T17:46:18Z

@adriancole: Have I answered your questions to your satisfaction? Have I addressed properly your concerns?
@RomanDzhabarov This is no longer a work-in-progress PR. I consider it ready, unless other issues are raised or the reviewers are not satisfied with my answers and latest commits that attempt to address their concerns.

cc: @rshriram

mattklein123

Some high level comments to get started. Will review in more detail once these are cleaned up.

mattklein123 · 2017-04-11T19:40:06Z

I don't think I would add this as a top level directory. Please move to source/common/tracing/zipkin. There are other things we may eventually want to compile out via various build options that are already in common. We can deal with that at that point.

mattklein123 · 2017-04-11T19:40:37Z

mattklein123 · 2017-04-11T19:40:59Z

All variables should use snake case. I won't comment on this further but please fix everywhere.

mattklein123 · 2017-04-11T19:41:32Z

Remove all standard headers that are already in precompiled.h (I won't comment on this in other places).

mattklein123 · 2017-04-11T19:41:57Z

SpanSharedPtr (there might be other instances of this, won't comment further).

mattklein123 · 2017-04-11T19:42:47Z

nit: put inside Zipkin namespace to avoid extra typing.

mattklein123 · 2017-04-11T19:43:57Z

To make the code easier to read/review, please have at minimum class level comments for each class/struct. Optimally, every public function should have proper doc comments to make it easier to review. (This applies everywhere, won't comment futher).

mattklein123 · 2017-04-11T19:44:55Z

Please use TODO(your GH name): style for TODOs. (Applies elsewhere)

mattklein123 · 2017-04-11T19:45:34Z

You should have a stat here. If there are similar TODOs please also make sure there are error stats.

mattklein123 · 2017-04-11T19:46:46Z

This is very slow, use common/hex.h instead.

codefromthecrypt · 2017-04-11T23:56:31Z

@adriancole <https://github.com/adriancole>: Have I answered your questions to your satisfaction? Have I addressed properly your concerns?

Yep! should this change include a readme update on how to test the code locally? or any other description of this feature?

RomanDzhabarov · 2017-04-12T01:46:46Z

need comments on the methods

RomanDzhabarov · 2017-04-12T01:47:45Z

full name for 'ann'

const Annotation& annotation

RomanDzhabarov · 2017-04-12T01:50:08Z

s/s/result or something

RomanDzhabarov · 2017-04-12T01:50:28Z

const std::string&

fabolive · 2017-04-28T01:53:30Z

@mattklein123 One more comment. The Zipkin library uses MonotonicTime to compute the span duration, but uses SystemTime for timestamps. Timestamps are rendered by the Zipkin UI as human-readable date/time.

mattklein123 · 2017-04-28T02:51:16Z

@fabolive cool will look at this in the morning and let's see if we can get this merged tomorrow. I think we are pretty close.

mattklein123 · 2017-04-28T02:53:39Z

Also, in the future, please don't rebase your change once you first push the PR. Just merge master and add commits. It makes it much easier to review the deltas.

mattklein123

This is looking great. I suspect there are quite a few opportunities for performance improvements, but we can sort that out once the code goes into production somewhere and we do some profiling.

A few small comments and then I think we are good to go. @htuch chime in if you feel like going through this at all.

mattklein123 · 2017-04-28T17:45:54Z

 }
+
+std::string Hex::uint64ToHex(uint64_t value) {
+  std::vector<uint8_t> data(8);


Use std::array here to avoid heap allocation.

mattklein123 · 2017-04-28T17:48:51Z

Please fix all the POD stuff per @htuch comments.

mattklein123 · 2017-04-28T17:57:56Z

+  virtual const std::string& toJson() PURE;
+
+protected:
+  std::string json_string_;


It's pretty odd to have this be a member variable and basically use it as a temporary. I would get rid of this, and then have all the functions return a temporary. The compiler will optimize it.

fabolive · 2017-04-28T22:25:05Z

@mattklein123 Please, see my latest commit to address your comments.
Note that, after I merged with master, check_format started failing for the files:
source/common/tracing/zipkin/BUILD
test/common/tracing/zipkin/BUILD
However, buildifier does not find anything wrong with the files. Any clue on how to fix this?

mattklein123 · 2017-04-28T22:46:35Z

@fabolive I think you are missing license() directive. Just run envoy/tools/check_format.py fix. Anyway this looks good to me. @htuch wanted to take a look and we can merge on Monday.

htuch

I did a style and code structure pass, I'm not a Zipkin expert so I'll defer to those who know more about its implementation.

htuch · 2017-04-30T21:48:29Z

+  data[1] = (value & 0x00FF000000000000) >> 48;
+  data[0] = (value & 0xFF00000000000000) >> 56;
+
+  return encode(&data[0], data.size());


This could also be done with std::hex and std::stringstream in 2-3 lines of code. Is there a reason you opted to do the manual unrolling? If so, please add a comment to the code.

The reason is performance.

htuch · 2017-04-30T21:55:33Z

+    for (uint64_t i = 1; i < size; i++) {
+      stringified_json_array += ",";
+      stringified_json_array += span_buffer_[i]->toJson();
+    }


You could consider using https://github.com/lyft/envoy/blob/master/source/common/json/json_loader.h#L22, which does basically what's being done here.

Well, Json::Factory::listAsJsonString() does not do exactly the same thing. As a result of that method, the elements of the stringified json array are stringified as well, which means that an extra level of double-quote escaping would be present. That was not my intention.
I would, therefore, prefer to keep this code unchanged.

htuch · 2017-04-30T21:57:14Z

+   *
+   * @param size The desired buffer size.
+   */
+  SpanBuffer(uint64_t size) { span_buffer_.reserve(size); }


Probably better to have this call allocateBuffer() to reserve the space to put the logic in a single location.

htuch · 2017-04-30T21:59:10Z

+   * Constructor that creates an empty buffer. Space needs to be allocated by invoking
+   * the method allocateBuffer(size).
+   */
+  SpanBuffer() {}


Does anyone use the default constructor? If not I'd suggest removing it.

Yes, this is used by the code on PR #700. PR #700, which is supposed to be merged after this, makes use of the classes on this PR to perform the actual Zipkin tracing.

htuch · 2017-04-30T22:01:31Z

+
+namespace Zipkin {
+
+// The static functions below are needed due to C++ inability to safely concatenate static strings


You can just say "due to the C++ static initialization order fiasco.". Most C++ programmers should know what that means (or Google it).

htuch · 2017-04-30T23:14:56Z

+void Span::finish() {
+  // Assumption: Span will have only one annotation when this method is called
+  SpanContext context(*this);
+  if ((context.isSetAnnotation().sr_) && (!context.isSetAnnotation().ss_)) {


Nit: unnecessary parentheses.

htuch · 2017-04-30T23:15:56Z

+  }
+
+  auto t = tracer();
+  if (t) {


Nit: if (auto t = tracer()) {

htuch · 2017-04-30T23:16:09Z

+}
+
+void Span::setTag(const std::string& name, const std::string& value) {
+  if ((name.size() > 0) && (value.size() > 0)) {


Nit: unnecessary parentheses.

htuch · 2017-04-30T23:38:43Z

+
+    // Set the timestamp globally for the span
+    span_ptr->setTimestamp(timestamp_micro);
+  } else if ((previous_context.isSetAnnotation().cs_) &&


Nit: unnecessary parentheses (and all other places).

htuch · 2017-04-30T23:39:54Z

+  /**
+   * Returns a randomly-generated 64-bit integer number.
+   */
+  static uint64_t generateRandom64();


Why not always use RandomGenerator()? Is there a good reason to do random number generation differently here?

Originally, my intention was to write a Zipkin library independent from Envoy. The code on PR #700 was supposed to be the only glue between Envoy and the Zipkin library. That's why my original code (which is long gone after several code-review rounds) did not use any of the classes and utilities from the Envoy code base. That includes random-number generation.

I would like to keep this, at least for now.

…nvoyproxy#430)

fabolive · 2017-05-01T22:48:15Z

@htuch Please, see my commits above, as well as my replies to your comments.
cc: @mattklein123, @rshriram

mattklein123

LGTM. @htuch any final comments?

htuch · 2017-05-02T14:31:39Z

I'll make another pass this arvo.

fabolive · 2017-05-02T19:31:23Z

@htuch I will make one more commit. I have applied the singleton constant template to zipkin_json_field_names.h, but forgot to apply it to zipkin_core_constants.h. I will commit shortly.
cc: @mattklein123

fabolive · 2017-05-02T21:15:50Z

@htuch, @mattklein123. I have just pushed one more commit. This should be the last planned one.

mattklein123

@htuch can you merge this if you are OK at this point.

htuch

Thanks for the contribution, @fabolive, this is huge.

One small request for the future - I'd really be interested in being involved in the review process early on where I can provide rapid feedback on small PRs. I feel I only have the bandwidth in this PR to pick up on some surface level stuff due to its size, having smaller PRs would be helpful to reviewers in focussing on the important aspects of the PR.

htuch · 2017-05-03T02:21:41Z

+          annotation_values_.sr_ = true;
+        } else if (annotation_value == ZipkinCoreConstants::get().SERVER_SEND) {
+          annotation_values_.ss_ = true;
+        }


There is some commonality in this if-else block with the constructor block that does the same. Please consider refactoring in the future.

@htuch, @mattklein123, @RomanDzhabarov, @adriancole : Thanks for your reviews!
@htuch: I will make sure the other PRs are smaller and will pull you early on in the process. I will consider the code refactoring suggested above.
I will resume the work on PR #700 and let you guys know when you can take a look at it. The code in that branch is outdated now.

codefromthecrypt · 2017-07-22T01:11:57Z

Hey folks Fyi #1196 aims to remove this work, pushing it to a separate repository making changes to support zipkin indirect. I'm concerned as it seems to be discussed without many of you providing input. I just want to make sure you are cool or voices are heard otherwise. If you are cool, cool with me.

fabolive changed the title ~~[WIP] Zipkin~~ [WIP] Add request tracing with Zipkin Apr 4, 2017

codefromthecrypt reviewed Apr 5, 2017

View reviewed changes

Comment thread source/common/http/conn_manager_impl.cc Outdated

Copy link
Copy Markdown

Contributor

codefromthecrypt Apr 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer two XX

..
just kidding :P

codefromthecrypt reviewed Apr 5, 2017

View reviewed changes

fabolive changed the title ~~[WIP] Add request tracing with Zipkin~~ [WIP] Zipkin library Apr 5, 2017

fabolive mentioned this pull request Apr 5, 2017

Use the Zipkin library to support Zipkin tracing #700

Closed

htuch reviewed Apr 6, 2017

View reviewed changes

codefromthecrypt reviewed Apr 6, 2017

View reviewed changes

codefromthecrypt reviewed Apr 11, 2017

View reviewed changes

rshriram reviewed Apr 11, 2017

View reviewed changes

fabolive changed the title ~~[WIP] Zipkin library~~ Zipkin library Apr 11, 2017

mattklein123 reviewed Apr 11, 2017

View reviewed changes

RomanDzhabarov reviewed Apr 12, 2017

View reviewed changes

Comment thread source/zipkin/span_buffer.h Outdated

Copy link
Copy Markdown

Member

RomanDzhabarov Apr 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need comments on the methods

RomanDzhabarov reviewed Apr 12, 2017

View reviewed changes

Comment thread source/zipkin/span_context.cc Outdated

Copy link
Copy Markdown

Member

RomanDzhabarov Apr 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/s/result or something

RomanDzhabarov reviewed Apr 12, 2017

View reviewed changes

Comment thread source/zipkin/span_context.cc Outdated

Copy link
Copy Markdown

Member

RomanDzhabarov Apr 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const std::string&

Fixed format of BUILD file (envoyproxy#430)

cff6194

mattklein123 reviewed Apr 28, 2017

View reviewed changes

fabolive added 2 commits April 28, 2017 17:52

Minor code improvements (envoyproxy#430)

d236ceb

Merge branch 'master' into zipkin

7c5dfae

Fixed BUILD files (envoyproxy#430)

f0fda03

htuch reviewed Apr 30, 2017

View reviewed changes

fabolive added 4 commits May 1, 2017 17:39

Minor code improvements (envoyproxy#430)

4d424c9

Merge branch 'master' into zipkin

8b0d96a

Use MockMonotonicTimeSource and MockRandomGenerator (envoyproxy#430)

11318bb

Use the Singleton template for string constants. Minor improvements. (e…

e27d597

…nvoyproxy#430)

mattklein123 previously approved these changes May 2, 2017

View reviewed changes

Merge branch 'master' into zipkin

55e8dbf

fabolive dismissed mattklein123’s stale review via a157ad0 May 2, 2017 21:14

Use the Singleton template for string constants (envoyproxy#430)

a157ad0

mattklein123 approved these changes May 2, 2017

View reviewed changes

htuch approved these changes May 3, 2017

View reviewed changes

htuch merged commit b6af9c4 into envoyproxy:master May 3, 2017

fabolive mentioned this pull request May 5, 2017

Enable Zipkin tracing #905

Merged

fabolive deleted the zipkin branch May 22, 2017 20:01

mathetake pushed a commit that referenced this pull request Mar 3, 2026

build(deps): bump github.com/openai/openai-go from 1.2.0 to 1.3.0 (#692)

fbf3067


		namespace Zipkin {

		// The static functions below are needed due to C++ inability to safely concatenate static strings

Conversation

fabolive commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Apr 4, 2017

Uh oh!

rshriram commented Apr 4, 2017

Uh oh!

RomanDzhabarov commented Apr 4, 2017

Uh oh!

rshriram commented Apr 4, 2017

Uh oh!

rshriram commented Apr 4, 2017

Uh oh!

htuch commented Apr 4, 2017

Uh oh!

RomanDzhabarov commented Apr 4, 2017

Uh oh!

fabolive commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rshriram commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

fabolive commented Apr 5, 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!

fabolive Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

fabolive Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

fabolive commented Apr 4, 2017 •

edited

Loading

fabolive commented Apr 4, 2017 •

edited

Loading

fabolive Apr 11, 2017 •

edited

Loading

fabolive Apr 11, 2017 •

edited

Loading