Refactor for supporting external writer by cgilmour · Pull Request #35 · DataDog/dd-opentracing-cpp

cgilmour · 2018-08-01T22:09:15Z

Related to some future proxy integrations, this lets external projects supply a Writer implementation for publishing traces.
This is useful when external projects need to control precisely when and how traces are published to the agent.

The Writer is given an HttpEncoder at initialization, and uses that to correctly publish the HTTP headers and msgpack-encoded payload to the agent. This allows us to retain control over those implementation details and not require the external project to provide an identical implementation.

willgittoes-dd

It might help to see the "other half", since I'm not sure I understand the architecture here.

HTTPEncoder seems like it could be entirely static, and therefore replaced with a function.

willgittoes-dd · 2018-08-01T23:10:59Z

+  HttpEncoder() {}
+  virtual ~HttpEncoder() {}
+
+  virtual std::string path() = 0;


willgittoes-dd · 2018-08-01T23:11:07Z

+class SpanData;
+using Trace = std::unique_ptr<std::vector<std::unique_ptr<SpanData>>>;
+
+class HttpEncoder {


willgittoes-dd · 2018-08-01T23:13:58Z

+  virtual ~HttpEncoder() {}
+
+  virtual std::string path() = 0;
+  virtual std::map<std::string, std::string> headers(std::deque<Trace> &traces) = 0;


And add a comment that describes the shape of this. From looking at the impl it looks idempotent, which is good! If we add a comment saying so, maybe it'll stay that way!

willgittoes-dd · 2018-08-01T23:16:49Z

+std::string AgentHttpEncoder::path() { return agent_api_path; }
+
+std::map<std::string, std::string> AgentHttpEncoder::headers(std::deque<Trace> &traces) {
+  std::map<std::string, std::string> headers(common_headers_);


This method could be static if only we got the 'version' into common_headers_ statically as well. Since version is known at compile time, surely there's some way we can statically reference it?

Yeah. This was to satisfy an existing test which passed in an arbitrary version string and depended on its value later on.
I'll adjust the test and remove some bits.

willgittoes-dd · 2018-08-01T23:22:05Z

+}
+
+std::string AgentHttpEncoder::payload(std::deque<Trace> &traces) {
+  return trace_encoder_(std::move(traces));


Why do we need to move? Why not just leave it owned by Writer (since this all happens synchronously), and let writer continue to explicitly clear() it and re-use the memory.

This should take a const reference to a Trace deque, and so should trace_encoder_.

Yup. This was from rushing to implement. Will fix.

willgittoes-dd · 2018-08-01T23:24:07Z

+namespace opentracing {
+
+// A function that encodes a collection of traces.
+typedef std::function<std::string(std::deque<Trace>)> TraceEncoder;


Will there be other implementations? Why not just call msgpack::pack directly in HttpEncoder::payload?

Then we could make HttpEncoder::payload static, and then we don't even need a class. What does envoy need, can we just give them a function or something?

The things envoy needs are more-or-less the three functions from HttpEncoder.

(We want to still control the implementation details of those, it just needs the data)

I'll tidy this up, make it simpler / flatter.

willgittoes-dd · 2018-08-01T23:24:24Z

+  ~AgentHttpEncoder() override {}
+
+  // Returns the path that is used to submit HTTP requests to the agent.
+  std::string path() override;


const (^___^)

willgittoes-dd · 2018-08-01T23:28:23Z

 namespace datadog {
 namespace opentracing {

+Writer::Writer() : encoder_(std::unique_ptr<HttpEncoder>{new AgentHttpEncoder{}}) {}


Could we assign this in the class def?

This is the one bit I wouldn't want to put in there.
It's the point where we set the bits we control for the writer that is extended by something else.

cgilmour · 2018-08-02T02:09:29Z

The "other half" is under development but depends on this change (that's also under development) 😄

willgittoes-dd

It's looking great! I'd set this to 'approve' with nitpicks, but I really want to see a diff of writer->agent_writer. So I've suggested you do a git mv (and then the writer.cpp as it exists in this PR is a "new file").

willgittoes-dd · 2018-08-29T03:28:29Z

+};
+
 std::shared_ptr<ot::Tracer> makeTracer(const TracerOptions &options);
+std::tuple<std::shared_ptr<ot::Tracer>, std::shared_ptr<TracePublisher>> makeTracerAndPublisher(


Might be good to add comments so anyone looking here would know why they'd want to use one method or the other.

willgittoes-dd · 2018-08-29T03:29:04Z

@@ -0,0 +1,173 @@
+#include "agent_writer.h"
+#include <iostream>


Could we please undo this change, and redo it with git mv. Otherwise the history isn't kept and I can't see changes.

I tried to fix this, but github isn't showing things clearly.
The history correctly shows up in blame, but not in the file's commit logs or in the PR diff

willgittoes-dd · 2018-08-29T03:29:52Z

@@ -0,0 +1,18 @@
+#include <datadog/opentracing.h>


Comment at the top about why we have two versions, and a comment on each about what they're for.

Also remove this and re-add it using git mv opentracing.cpp opentracing_agent.cpp

Comment added.

willgittoes-dd · 2018-08-29T03:32:07Z

  }
-  return std::move(
-      std::unique_ptr<ot::SpanContext>{new SpanContext{parent_id, trace_id, std::move(baggage)}});
+  return std::unique_ptr<ot::SpanContext>{


While we're fixing my silly mistakes, can we use make_unique here?

willgittoes-dd · 2018-08-29T03:33:38Z

+  void clearTraces() override;
+  // Returns the HTTP headers that are required for the collection of traces.
+  const std::map<std::string, std::string> headers() override;
+  // Returns the encoded payload from the collection of traces.


May as well put these comments in the superclass?

willgittoes-dd · 2018-08-29T03:36:50Z

 std::unique_ptr<SpanData> makeSpanData(std::string type, std::string service,
                                       ot::string_view resource, std::string name,
-                                       uint64_t trace_id, int64_t span_id, uint64_t parent_id,
+                                       uint64_t trace_id, uint64_t span_id, uint64_t parent_id,


Oh shiiiiiiii

I think I missed a -Wall somewhere.

Even -Wall -Wextra -pedantic wouldn't have made noise about it.

willgittoes-dd · 2018-08-29T03:39:31Z

@@ -1,180 +1,14 @@
 #include "writer.h"
 #include <iostream>


Yeah I think this diff/history would definitely make more sense if we git mv writer.cpp agent_writer.cpp and then make a new writer.cpp (and some for .h).

willgittoes-dd · 2018-08-29T03:43:27Z

@@ -1,6 +1,7 @@
-#include "../src/writer.h"


Yes, like this! It's been git mv 'd! And make a new writer_test.cpp that tests ExternalWriter (I know there's not much to test there, but still).

willgittoes-dd · 2018-08-30T06:15:56Z


+// TracePublisher exposes the data required to publish traces to the
+// Datadog Agent.
+class TracePublisher {


Thinking about this, I'd want to call it 'TraceEncoder', since that's the piece of AgentWriter that's moved into here. Even the headers stuff makes sense as "encoding", since it's "encoding" the traces as HTTP.

willgittoes-dd · 2018-08-30T06:18:48Z

  std::string operation_name_override = "";
 };

+// TracePublisher exposes the data required to publish traces to the


Needs a comment that it's not thread-safe. The way this is used in AgentWriter will work fine, and you should double-check that it's only going to be used single-threaded in the envoy stuff.

The best way to really make sure we don't fuck this up in the future, is to make the constructor private and have a static method return a unique_ptr only. That way, anyone who gets an instance of this object really has to go out of their way (dangerously get()ing the pointer) to use this in a multi-threaded context.

cgilmour requested a review from willgittoes-dd August 1, 2018 22:09

willgittoes-dd reviewed Aug 1, 2018

View reviewed changes

cgilmour force-pushed the cgilmour/refactor-for-external-writer branch 2 times, most recently from 190cb43 to a24f4c1 Compare August 13, 2018 03:11

cgilmour mentioned this pull request Aug 13, 2018

WIP datadog tracing for envoy DataDog/envoy#1

Closed

cgilmour force-pushed the cgilmour/refactor-for-external-writer branch from a24f4c1 to 6425753 Compare August 13, 2018 04:12

cgilmour force-pushed the cgilmour/refactor-for-external-writer branch from a5cfcec to aa23cbc Compare August 27, 2018 06:50

willgittoes-dd added the enhancement New feature or request label Aug 29, 2018

willgittoes-dd suggested changes Aug 29, 2018

View reviewed changes

cgilmour added 5 commits August 29, 2018 18:35

Move files to retain history

de96939

Refactor to support external publishing of traces.

0c1788b

Change to makeTraceAndPublisher

d814504

Now curl libs not required.

9388e0c

Cleanup and fixes for some warnings

9b61949

cgilmour force-pushed the cgilmour/refactor-for-external-writer branch from aa23cbc to 3c05488 Compare August 30, 2018 05:16

Additional comments on the code additions.

6a5d804

cgilmour force-pushed the cgilmour/refactor-for-external-writer branch from 3c05488 to 6a5d804 Compare August 30, 2018 05:28

Use make_unique

5bd13fc

willgittoes-dd approved these changes Sep 4, 2018

View reviewed changes

Publisher -> Encoder

06a51ae

cgilmour merged commit 7ab99c6 into master Sep 4, 2018

cgilmour deleted the cgilmour/refactor-for-external-writer branch September 4, 2018 05:37

		@@ -0,0 +1,173 @@
		#include "agent_writer.h"
		#include <iostream>

Conversation

cgilmour commented Aug 1, 2018

Uh oh!

willgittoes-dd 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!

cgilmour commented Aug 2, 2018

Uh oh!

willgittoes-dd 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants