tracing: add datadog extension by cgilmour · Pull Request #4699 · envoyproxy/envoy

cgilmour · 2018-10-12T03:41:34Z

Description:
This PR adds a tracer extension so envoy can produce HTTP traces and submit them to Datadog via an agent.
It has similar behavior to the existing LightStep and Zipkin tracers, and their implementations were used as a reference.

Risk Level:
Low.

Testing:
Unit tests ("borrowed" from Lightstep)
Integration tests using docker-compose. (An example is not submitted yet, but can be provided)
End-to-End tests from browser HTTP request to traces appearing in Datadog's system, internally and by external users in a staging environment.

Docs Changes:
Please suggest where doc changes need to be made.

Release Notes:
Please suggest about this also.

Fixes #3861 (already closed)

CC: @mattklein123 (offered to sponsor this submission) and other tracing experts @rnburn and @objectiser

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

alyssawilk · 2018-10-12T16:32:25Z

Sorry, can you do a master merge to pick up #4701?

mattklein123 · 2018-10-12T21:23:17Z

@cgilmour unfortunately I'm out for the next 2 weeks, and I'm not going to have a chance to look at this before I go. I'm going to mark this as "no stalebot" in the interim. If anyone else wants to take a look including @rnburn and/or @objectiser that would be great.

rnburn · 2018-10-15T19:26:08Z

+  // The cluster to use for submitting traces to the Datadog agent.
+  string collector_cluster = 1 [(validate.rules).string.min_bytes = 1];
+  string service_name = 2 [(validate.rules).string.min_bytes = 1];
+  bool priority_sampling = 3;


What's the priority_sampling option?

Also, does datadog's tracer support the sampling.priority tag? Envoy uses it to disable sampling for some of the traffic that would be noise.

@rnburn Support for that tag is being added in DataDog/dd-opentracing-cpp#59 should be ready today.

@cgilmour We should copy/have similar comments as https://github.com/DataDog/dd-opentracing-cpp/blob/master/src/tracer_factory.cpp#L21 We should also default priority sampling to on here (whereas normally it's off by default), since envoy uses it.

rnburn · 2018-10-15T19:38:26Z

+    message->headers().insertPath().value(encoder_->path());
+    message->headers().insertHost().value(driver_.cluster()->name());
+    for (auto& h : encoder_->headers()) {
+      ENVOY_LOG(debug, "Adding header {}: {}", h.first, h.second);


Is there enough context in this log statement for it to be useful?

rnburn · 2018-10-15T19:45:07Z

+  cluster_ = cluster->info();
+
+  tracer_options_.operation_name_override = "envoy.proxy";
+  if (datadog_config.service_name().size() > 0) {


!datadog_config.service_name().empty()?

rnburn · 2018-10-15T19:48:43Z

+  Upstream::ClusterInfoConstSharedPtr cluster() { return cluster_; }
+  Runtime::Loader& runtime() { return runtime_; }
+  DatadogTracerStats& tracerStats() { return tracer_stats_; }
+  const datadog::opentracing::TracerOptions& tracerOptions() { return tracer_options_; }


I think there's an Envoy convention for putting non-overridden methods like this before the overridden ones.

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

cgilmour · 2018-10-16T09:48:47Z

I'll sort out an update to fix the failing tests.
Seems like a minor detail from some recent refactoring in our code.

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

mattklein123

Thanks for working on this and sorry for the delay. Some comments to get started.

mattklein123 · 2018-10-29T18:41:31Z

  google.protobuf.Struct config = 2;
 }

+// Configuration for the Datadog tracer.


Can we add a release note for this? Are there any other docs that need updating? Perhaps https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/tracing?

Happy to do this once the main code changes are acceptable.

mattklein123 · 2018-10-29T18:47:21Z

+
+class TraceReporter;
+typedef std::unique_ptr<TraceReporter> TraceReporterPtr;
+typedef std::shared_ptr<datadog::opentracing::TraceEncoder> TraceEncoderPtr;


nit: TraceEncoderSharedPtr

mattklein123 · 2018-10-29T18:51:55Z

+                                POOL_COUNTER_PREFIX(stats, "tracing.datadog."))},
+      tls_(tls.allocateSlot()), runtime_(runtime) {
+
+  Upstream::ThreadLocalCluster* cluster = cm_.get(datadog_config.collector_cluster());


You can use Config::Utility::checkCluster here I think.

Yup, it was done this way for consistency (and not knowing about checkCluster).
Would you like the other tracers updated as well?

Sure please clean up.

Done for all three cases.

mattklein123 · 2018-10-29T18:53:20Z

+
+void TraceReporter::enableTimer() {
+  const uint64_t flush_interval =
+      driver_.runtime().snapshot().getInteger("tracing.datadog.flush_interval_ms", 1000U);


Please make sure these runtime keys are documented somewhere. Don't recall of the top of my head where the other keys you based this on are documented.

It was "inspired" by settings in other tracers, but can probably be replaced with a sensible default.
The impact of it is on the tracer mechanics more than the user.

Yup that's fine, you don't need to have things be configurable if you don't want.

Replaced with a sensible default.

mattklein123 · 2018-10-29T18:54:16Z

+  flush_timer_->enableTimer(std::chrono::milliseconds(flush_interval));
+}
+
+void TraceReporter::flushTraces() {


I'm guessing that almost all this code was basically copied from the LS implementation? How much of it is different? Is it worth sharing the code in any way? cc @rnburn

Correct, they were used as a reference, and where possible, used verbatim so that the result would be consistent.
If there's room to refactor things, that's great.
Flush timers and flush mechanisms are probably a good case for that.
Not sure about other bits.

It's up to you but any de-dup would be appreciated. This is going to come up again when someone comes in and adds the OpenCensus tracer.

Not done, but OK doing a followup change to de-dup things, and any suggestions from @rnburn

mattklein123 · 2018-10-29T18:54:54Z

+
+    Http::MessagePtr message(new Http::RequestMessageImpl());
+    message->headers().insertMethod().value().setReference(Http::Headers::get().MethodValues.Post);
+    message->headers().insertPath().value(encoder_->path());


This line and next line can be references I think, FWIW

OK. I'll see I can make this one return a reference. The next one is internal to envoy so easier to update.

mattklein123 · 2018-10-29T18:55:31Z

+    message->headers().insertMethod().value().setReference(Http::Headers::get().MethodValues.Post);
+    message->headers().insertPath().value(encoder_->path());
+    message->headers().insertHost().value(driver_.cluster()->name());
+    for (auto& h : encoder_->headers()) {


for perf reasons, it would be better to pre-construct the lower case headers. Then you can just send them by reference.

Alright, I'll see what I can do for that.

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

cgilmour · 2018-10-30T09:32:04Z

Thanks @mattklein123, added some responses to the comments.
I'll work on the changes and push them when they're ready for another look.

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

mattklein123

This in general LGTM. Can we do a docs/release notes pass and then I can take a final pass? Thank you!

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

mattklein123 · 2018-11-03T23:44:40Z

Marking is waiting.

/wait

mattklein123

Very nice, thank you!

lizan · 2018-11-11T01:18:00Z

CODEOWNERS entry is missing, @cgilmour @mattklein123?

cgilmour · 2018-11-11T03:28:12Z

Ok. I can submit a followup PR for that.

mattklein123 · 2018-11-11T19:12:51Z

Thank you @cgilmour!

tracing: add datadog extension

2b5c3e6

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

cgilmour force-pushed the cgilmour/datadog-tracer branch from 3decae8 to 2b5c3e6 Compare October 12, 2018 04:23

alyssawilk assigned mattklein123 Oct 12, 2018

Merge branch 'master' into cgilmour/datadog-tracer

5bc8aa8

cgilmour mentioned this pull request Oct 12, 2018

WIP datadog tracing for envoy DataDog/envoy#1

Closed

mattklein123 added the no stalebot Disables stalebot from closing an issue label Oct 12, 2018

rnburn reviewed Oct 15, 2018

View reviewed changes

Updates from review comments.

569bbc2

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

cgilmour force-pushed the cgilmour/datadog-tracer branch from 9a27059 to 569bbc2 Compare October 16, 2018 04:48

cgilmour added 2 commits October 17, 2018 07:54

Update dd-opentracing-cpp to v0.3.3

35b8db6

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

Minor fix and bump dd-opentracing-cpp to v0.3.4

7e20b68

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

mattklein123 removed the no stalebot Disables stalebot from closing an issue label Oct 29, 2018

mattklein123 requested changes Oct 29, 2018

View reviewed changes

cgilmour added 3 commits October 30, 2018 07:18

Use correct type name TraceEncoderSharedPtr

b837332

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

Use references instead of copies.

864db99

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

Remove priority_sampling option.

08bbe72

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

cgilmour added 7 commits October 31, 2018 03:26

Revert to addCopy for encoder headers.

cb479b2

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

Use sensible default, remove config items. Fix test.

cf8c511

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

Change tracers to use checkCluster

9b25a32

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

Convert headers to LowerCaseString once. Pass refs

160ff4e

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

Update dd-opentracing-cpp -> v0.3.5

3ed7daf

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

Merge branch 'master' into cgilmour/datadog-tracer

eb99d70

Update repository locations

8c1fe84

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

Update for clang_tidy errors

5a243ab

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

mattklein123 reviewed Nov 2, 2018

View reviewed changes

cgilmour added 2 commits November 2, 2018 22:52

Merge branch 'master' into cgilmour/datadog-tracer

fd5a4a3

Update release notes and tracer docs.

cf4dfb2

Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>

repokitteh-read-only Bot added waiting and removed waiting labels Nov 3, 2018

mattklein123 approved these changes Nov 5, 2018

View reviewed changes

mattklein123 merged commit 46c0693 into envoyproxy:master Nov 5, 2018

cgilmour mentioned this pull request Nov 14, 2018

Add code owners entry for datadog extension #5030

Merged

cknowles mentioned this pull request Nov 19, 2018

Datadog tracing APM support istio/istio#6314

Closed

cgilmour deleted the cgilmour/datadog-tracer branch January 30, 2019 22:13

Conversation

cgilmour commented Oct 12, 2018

Uh oh!

alyssawilk commented Oct 12, 2018

Uh oh!

mattklein123 commented Oct 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnburn Oct 15, 2018 • 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!

rnburn Oct 15, 2018 • 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!

cgilmour commented Oct 16, 2018

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!

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!

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!

cgilmour commented Oct 30, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Nov 3, 2018

Uh oh!

mattklein123 left a comment

rnburn Oct 15, 2018 •

edited

Loading

rnburn Oct 15, 2018 •

edited

Loading