Enable Zipkin tracing by fabolive · Pull Request #905 · envoyproxy/envoy

fabolive · 2017-05-05T20:47:48Z

This PR makes Envoy use the code added by PR #692 (Zipkin library).

This PR replaces the old PR #700.

cc: @mattklein123, @htuch, @RomanDzhabarov

…yproxy#430)

… master. Make Zipkin Tracer use Envoy's own random-number generator (envoyproxy#430)

fabolive · 2017-05-05T22:04:53Z

@moderation Fixed!

ccaraman · 2017-05-05T23:16:24Z

+   * annotation then this Envoy instance initiated the span. Only the Envoy that initiates a Zipkin
+   * span should add binary annotations to it; otherwise, duplicate binary annotations
+   * would be created.
+


nit: missing *

ccaraman · 2017-05-05T23:27:14Z

+    http_tracer_.reset(
+        new Tracing::HttpTracerImpl(std::move(lightstep_driver), server_.localInfo()));
+  } else if (type == "zipkin") {
+    if (server_.localInfo().clusterName().empty()) {


nit: remove else statement and add ASSERT checking for type is zipkin. ex https://github.com/lyft/envoy/blob/master/source/common/router/router_ratelimit.cc#L102

goaway · 2017-05-06T01:21:09Z

+        "properties" : {
+          "type" : {
+            "type" : "string",
+            "enum" : ["zipkin"]


I might have missed some prior discussion, but my reading of this enum is that tracing drivers were originally envisioned to all be specified under a single "driver" schema, with a type ("zipkin", "lightstep") specified via this field.

Most of the other fields are identical across "lightstep_driver" and "zipkin_driver."

My original code had both "lightstep" and "zipkin" as valid values of the same enum, and I had made access_token_file an optional parameter, as it has no use for Zipkin. However, under PR #700 (the old PR replaced by this one), @RomanDzhabarov suggested the following: "this should be done in a way that there are separate schemas for different driver types, otherwise schema will not have much value."

fabolive · 2017-05-07T14:48:21Z

@goaway I addressed two of your comments above. Please, see my reply to your comment on driver schema.

…nvoyproxy#430)

)

…ctor_endpoint optional. (envoyproxy#430)

…eaned up code. (envoyproxy#430)

…oyproxy#430)

fabolive · 2017-05-12T05:14:00Z

@mattklein123 Please, see my new commits addressing your comments.

mattklein123

looks good, a few small things.

mattklein123 · 2017-05-12T16:22:04Z


+void Annotation::changeEndpointServiceName(const std::string& service_name) {
+  if (endpoint_.valid()) {
+    Endpoint endpoint(endpoint_.value());


You can avoid copying here by just adding a non-const value() function here: https://github.com/lyft/envoy/blob/master/include/envoy/common/optional.h#L37

The method signature should be: T& value() and the body will be the same as the other one.

Sorry just to clarify, when you add the new non-const version, have the const version call the non-const version to avoid code duplication. :)

I cannot have one call the other without using const_cast. Are you okay with this use of const_cast?

The const version can call the non-cost version. Basically never use const_cast.

That seems to be intuitive, indeed. That's what I did.
The problem here is different, though. Irrespective of whether or not const_cast is used, the compiler seems to detect an infinite recursion. Perhaps because of optimization? (I am compiling the code with the CI docker image, the one I pulled today).
So, the compiler is not happy with this:

T& value() { if (!valid_) { throw EnvoyException("fetching invalid Optional value"); } return value_; } const T& value() const { return value(); }

I see, sorry, yes. I'm not sure off the top of my head the right way to deal with this. I would just copy the code and then make sure you have test coverage for both variants in optional_test (just declare a const Optional for testing).

ok. I will duplicate the code. I believe avoiding code duplication requires an ugly combination of const_cast and static_cast...

mattklein123 · 2017-05-12T16:24:22Z

+  return false;
+}
+
+ZipkinDriver::TlsZipkinTracer::TlsZipkinTracer(TracerPtr tracer, ZipkinDriver& driver)


nit: for clarify making this take TracerPtr&& which will make it clear at the call site that you are moving.

mattklein123 · 2017-05-12T16:25:20Z

+
+namespace Zipkin {
+
+ZipkinSpan::ZipkinSpan(Zipkin::Span& span) : span_(span) {}


nit: it would be optimal if you remove Zipkin from all of the class names now, since you are inside the Zipkin namespace. You can veto this but I would appreciate it. 😉

I only left Zipkin:: in the ZipkinSpan class. It turns out that, because ZipkinSpan inherits from Tracing::Span, any reference to Span is considered to be a reference to Tracing::Span and not to Zipkin::Span. I explicitly refer to Zipkin::Span to disambiguate. I was surprised by this behavior. Any suggestion?

No that's fine.

mattklein123 · 2017-05-12T16:27:20Z

-  Tracer tracer("my_service_name", addr);
-  MockMonotonicTimeSource mock_start_time;
-  MonotonicTime start_time = mock_start_time.currentTime();
+  Runtime::RandomGeneratorImpl random_generator;


Use MockRandomGenerator

I can certainly do that; no problem. The reason I used the real RandomGenerator in some cases was that MockRandomGenerator::random() always returns 0; therefore it renders some of my checks useless. It would be nice if we could control the value to be returned by MockRandomGenerator::random() to some extent, e.g., zero or non-zero.

You can. It's a mock, have it return whatever you want in your tests.

Yes, please remove commented out code. The point of the mock is so that you can return whatever value want, and can exercise all of your code paths completely and reproducibly. So make sure you have full coverage when using all mocks, and remove all commented out checks.

mattklein123 · 2017-05-12T16:30:06Z

+
+  Span span2(std::move(span));
+
+  EXPECT_EQ(id, span2.id());


If you are going to bother writing a move constructor test, you should sanity check that span1 has actually been moved from, and a copy was not done.

According to the test coverage reports with and without this test, the code above is indeed exercising the move constructor.
Given that a "move" in the pure sense of the word does not happen when std::move is invoked, I don't think there is a safe/good way to determine whether or not an object was moved from. One (ugly) approach to automatically verify if the move constructor was actually called would be to change the class implementation to set a member flag to a value in the case of a copy constructor execution, and to another value when the move constructor is exercised. I don't think we want that. Any ideas?

You can check span1 to see if it still contains data members. The move should literally move them, so string should be empty, etc.

I don't think that is the case. My point is: there is no move in the literal sense. I believe what actually happens is some sort of "optimized copy". In fact, the data members of the original span seem to be intact.

Then you aren't actually moving anything. In the interest of moving this forward, I would kill the move constructor and just do the copy, and deal with the perf issues in a follow up.

mattklein123 · 2017-05-12T16:30:33Z

+
+  void setupValidDriver() {
+    EXPECT_CALL(cm_, get("fake_cluster")).WillRepeatedly(Return(&cm_.thread_local_cluster_));
+    ON_CALL(*cm_.thread_local_cluster_.cluster_.info_, features())


IIRC this is the default so this line can be removed.

mattklein123 · 2017-05-12T16:30:57Z

+  NiceMock<Runtime::MockLoader> runtime_;
+  NiceMock<ThreadLocal::MockInstance> tls_;
+  NiceMock<LocalInfo::MockLocalInfo> local_info_;
+  Runtime::RandomGeneratorImpl random_;


(Just search all the test code for non-mock versions to make sure you get them all). Thanks.

mattklein123 · 2017-05-12T16:38:59Z

P.S., @rshriram @fabolive I would strongly recommend setting up a perf run with zipkin enabled and getting some (perf) traces. I think there is going to be some super low hanging fruit. Can obviously be done in a follow up.

fabolive · 2017-05-12T20:33:58Z

@mattklein123 Please, see my latest commit and my replies to your comments. Do you have any suggestions based on all my comments above?

mattklein123 · 2017-05-12T20:34:33Z

@fabolive yes, I responded.

mattklein123 · 2017-05-12T22:53:52Z

 void Annotation::changeEndpointServiceName(const std::string& service_name) {
  if (endpoint_.valid()) {
-    Endpoint endpoint(endpoint_.value());
+    Endpoint& endpoint = endpoint_.value();


nit: delete this line and just do endpoint_.value().setServiceName(service_name)

fabolive · 2017-05-13T00:58:15Z

@mattklein123 Let me know if there are any further changes that are needed.

mattklein123 · 2017-05-13T01:03:36Z

@fabolive I still see most of the classes have Zipkin in them in the Zipkin namespace. Are we skipping that for now? (I thought your comment meant you had changed all of them besides for ZipkinSpan).

fabolive · 2017-05-13T01:10:07Z

@mattklein123 I actually meant something else. I might have misunderstood your original comment. Never mind; let me rename the classes.

fabolive · 2017-05-13T01:29:33Z

@mattklein123 Classes renamed (except for ZipkinSpan).

mattklein123

Few tiny things and then ready to ship.

mattklein123 · 2017-05-13T16:02:58Z

 namespace Zipkin {

-bool SpanBuffer::addSpan(SpanPtr&& span) {
+bool SpanBuffer::addSpan(Span&& span) {


To make it clear that we are currently copying here, please make this method take a const Span& instead. I would also add a TODO that this needs to be looked at for perf.

mattklein123 · 2017-05-13T16:03:30Z

 * span will be sent to out to Zipkin, or buffered so that it can be sent out later.
 */
-class Reporter {
+class ReporterInterface {


To match the rest of the code, please leave this as Reporter, and make the implementation class ReporterImpl

mattklein123 · 2017-05-13T16:04:01Z

+#include "common/tracing/http_tracer_impl.h"
+#include "common/tracing/zipkin/zipkin_core_constants.h"
+
+#include "spdlog/spdlog.h"


I think @RomanDzhabarov already mentioned this, but this include should not be needed here.

If I remove that include statement the code does not compile because of this:

throw EnvoyException(fmt::format("{} collector cluster is not defined on cluster manager level", config.getString("collector_cluster")));

As you can see, I am actually using fmt::format(). Do you prefer that I do not use fmt::format in favor of plain string concatenation?

Ah OK, that's fine. (Didn't seeing logging so was confused). No problem.

fabolive · 2017-05-13T20:07:03Z

@mattklein123 Code updated.

mattklein123

Nice!

rshriram · 2017-05-13T22:26:37Z

@kyessenov time to update Envoy SHA in our code

fabolive and others added 14 commits April 17, 2017 14:35

Reapplied changes reverted from PR 692. Use the Zipkin library. (envo…

83c00b9

…yproxy#430)

removing printfs and cleanup json schema

9b129bf

refactor zipkin tracer code into separate files

11dd597

disallow http2 collector cluster for zipkin

ee362a5

fix buggy stats recording

abba6cc

Zipkin tracer implementation - tests: first pass

4052a88

Merged master (envoyproxy#430)

9929ce5

Merged master (envoyproxy#430)

ad84cc2

Merged master (envoyproxy#430)

48993c8

Fixed interfaces when integrating the code on this PR with the latest…

75f2640

… master. Make Zipkin Tracer use Envoy's own random-number generator (envoyproxy#430)

Merge branch 'master' into use-zipkin-library

35a67b4

Added Zipkin-related documentation. (envoyproxy#430)

c6addb1

Updated code comments (envoyproxy#430)

0025d37

Fixed Zipkin documentation typo (envoyproxy#430)

86c1a2f

ccaraman reviewed May 5, 2017

View reviewed changes

moderation mentioned this pull request May 6, 2017

Support multiple tracing drivers #909

Closed

goaway reviewed May 6, 2017

View reviewed changes

fabolive added 2 commits May 7, 2017 10:27

Added missing * to comment block. (envoyproxy#430)

8c6d335

Using ASSERT to check for zipkin driver type (envoyproxy#430).

b6654b0

fabolive added 4 commits May 8, 2017 09:44

Add method to get a Span::Zipkin reference from a ZipkinSpan object. (e…

f087130

…nvoyproxy#430)

Merge branch 'master' into use-zipkin-library

e77dddc

Merged with master. Fixed asan/tsan errors. (envoyproxy#430)

b814fcd

Fixed clang format. (envoyproxy#430)

c89a99e

mattklein123 requested review from RomanDzhabarov and goaway May 8, 2017 23:01

fabolive added 2 commits May 8, 2017 22:41

Support changing span's source/destination service names (envoyproxy#430

6e45ef5

)

Merge branch 'master' into use-zipkin-library

91c71d9

Shriram Rajagopalan and others added 8 commits May 11, 2017 14:56

remove :ref:

1d79da7

Added move constructor to Zipkin::Span (envoyproxy#430)

ea13629

Merge branch 'master' into use-zipkin-library

84fa474

Fixed file format. Merged docs (envoyproxy#430)

cb52302

Remove Tracer move constructor. Use unique_ptr for Tracer. Make colle…

2398424

…ctor_endpoint optional. (envoyproxy#430)

Removed const_cast (envoyproxy#430)

4762a32

Removed HTTP2 check. Made tls_slot_ and collector_endpoint_ const. Cl…

5c0bc00

…eaned up code. (envoyproxy#430)

Moved zipkin_tracer_impl.* to the zipkin directory and namespace (env…

1de938d

…oyproxy#430)

mattklein123 reviewed May 12, 2017

View reviewed changes

fabolive added 2 commits May 12, 2017 13:25

Merge branch 'master' into use-zipkin-library

7bfa342

Addressed latest code-review comments (envoyproxy#430)

d5e96de

Fixed usage of Mock (envoyproxy#430)

839bf50

mattklein123 reviewed May 12, 2017

View reviewed changes

Removed Span move constructor. More clean-up (envoyproxy#430)

fc5d961

Renamed classes (envoyproxy#430)

7981b56

mattklein123 reviewed May 13, 2017

View reviewed changes

Feedback from code review (envoyproxy#430)

320a2a8

mattklein123 approved these changes May 13, 2017

View reviewed changes

mattklein123 merged commit 14e1f10 into envoyproxy:master May 13, 2017

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


		namespace Zipkin {

		ZipkinSpan::ZipkinSpan(Zipkin::Span& span) : span_(span) {}

Conversation

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

Uh oh!

Uh oh!

fabolive commented May 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabolive May 7, 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!

fabolive commented May 7, 2017

Uh oh!

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

Uh oh!

Uh oh!

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

fabolive May 12, 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!

fabolive May 12, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabolive May 12, 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!

fabolive May 12, 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 commented May 5, 2017 •

edited

Loading

fabolive May 7, 2017 •

edited

Loading

fabolive commented May 12, 2017 •

edited

Loading

fabolive May 12, 2017 •

edited

Loading

fabolive May 12, 2017 •

edited

Loading

fabolive May 12, 2017 •

edited

Loading

fabolive May 12, 2017 •

edited

Loading

fabolive May 13, 2017 •

edited

Loading