Checkpoint1 by rnburn · Pull Request #1 · rnburn/opentracing-cpp

rnburn · 2017-06-08T01:13:56Z

Sets up an OpenTracing interface based off of LightStep's tracer. A few modifications from the original LightStep code I made include:

Removed the reference-counting wrapper classes so as to simplify the library.
Created intermediate StartSpanOptions and FinishSpanOptions classes and changed the Apply functions to take these for their arguments instead of a Span.

Other than that, I tried to stay close to the original implementation. I image there will be some other changes we want to make before proposing it as an interface. Here are some things I would consider:

Should we use noexcept to specify which of the API functions are guaranteed not to throw?
Do we want to use something like the StringRef class from the C++ Redux PR to avoid unnecessary string copying.
Would adding inline namespaces help to manage versioning?

I also wrote some other more specific suggests inline.

rnburn · 2017-06-08T01:25:02Z

+// Basic foundation for OpenTracing basictracer-compatible carrier writers.
+class BasicCarrierWriter : public CarrierWriter {
+ public:
+  virtual void Set(const std::string& key, const std::string& value) const = 0;


Should Set return an error-code that allows a user to bail out of the writer if the key-value pair can't be written? I found the lack of such a return code inconvenient when writing the CarrierWriter for Nginx's instrumentation, since setting a header value can fail and you have to otherwise manage another error code separately or use exceptions to break out of the writer.

Yes, sounds like a good idea to return an error here. I'll let you pick the best style :)

rnburn · 2017-06-08T01:28:01Z

+  //
+  // Returns true on success.
+  virtual bool Inject(const SpanContext& sc, CarrierFormat format,
+                      const CarrierWriter& writer) = 0;


Should Inject have a more descriptive return code? Perhaps something similar to this?

I don't like returning values using reference variables like that, but I think it should be permitted to return some useful information. I just wish C++ had a better style for error types, but I'm fine if there's a std::string pointer for returning error values (same goes for the question about CarrierWriter::Set).

How about something like Expected<void, std::string> from this library? It also mirrors what's being proposed for a future C++ standard.

That looks appropriate. I generally support moving in the direction of the C++ language, so yes.

rnburn · 2017-06-08T01:36:10Z

+class StartTimestamp : public StartSpanOption {
+ public:
+  StartTimestamp(SystemTime system_when, SteadyTime steady_when)
+      : system_when_(system_when), steady_when_(steady_when) {}


Should we also allow you to construct the StartTimestamp from a single point in time that then uses that time point to set both the system and steady timestamps?

That sounds nice. Want to create such a type?

Not sure we need a new type. I was thinking more of just an additional constructor that allows you to pass in a singe time_since_epoch of std::chrono::duration. I updated with what I have in mind.

rnburn · 2017-06-08T01:37:32Z

+  // may ignore the tag, but shall not panic.
+  //
+  // SetTag may be called prior to Finish.
+  virtual void SetTag(const std::string& key, const Value& value) = 0;


Do we want a more restrictive type for a tag's value?

I'm not sure. On the one hand, I think it's useful to allow anything goes here, but probably difficult for people to agree on the semantics / interpretation of nested structure other than scalar values. I'd suggest leaving our options open and getting this around for review.

I.e., restrict values to scalars for now. I think OT needs to put some work into structured data representation but here and now is not the time to hash it out.

rnburn · 2017-06-08T01:42:16Z

+  //
+  // With the exception of calls to context() (which are always allowed),
+  // Finish() must be the last call made to any span instance, and to do
+  // otherwise leads to undefined behavior.


Following the discussion here, should we allow Span's methods to be called after finish?

I think it's fine for certain operations to have undefined behavior after finish, but not OK to crash. I believe based on other OT discussions that the SpanContext should be accessible after Finish(), especially, to allow starting a FollowsFrom span after the parent completes.

How about the contract is that SetTag, SetOperation, Log (when added), etc leave the span in a valid but unspecified state, and calling Finish more than once does nothing. That way they'd be safe to use from a multithreaded context without forcing a user to do additional synchronization.

Yes, sounds good.

rnburn · 2017-06-08T01:44:08Z

+  //
+  // Returns a `SpanContext` that is `non-null` on success.
+  virtual std::unique_ptr<SpanContext> Extract(CarrierFormat format,
+                                               const CarrierReader& reader) = 0;


Any reason Inject and Extract aren't const?

Not that I can remember. Let's say they should be and see if there was a reason. :)

rnburn · 2017-06-08T22:07:24Z

+    options.start_steady_timestamp = SteadyClock::now();
+    for (const auto& option : option_list) option.get().Apply(options);
+    return StartSpanWithOptions(operation_name, options);
+  }


Should something like StartSpan be made noexcept and specify that a nullptr is returned upon an error?

Not sure the best way to handle something like a std::bad_alloc error that might occur. You could either do nothing (with noexcept the program would terminate) or catch it and return a nullptr.

I was at Google for a hundred years and (because of their style guide) have very little experience programming with C++ exceptions. I'm not sure I have an opinion, so I recommend doing what you think is best.

rnburn · 2017-06-19T20:59:27Z

+};
+
+// Carrier format values.
+enum class CarrierFormat {


Is this the right type to use to specify the carrier format? It's not clear to me how something like LightStep's custom binary format or other custom carriers would fit into this.

rnburn added 13 commits June 6, 2017 13:13

Added core classes.

ade8fb6

Added propagation functions.

7b96e57

Copied documentation over from Go.

72f3f31

Added noop tracer.

f9c1cb6

Added CMakeLists.txt.

286ec27

Added tracer method.

d59a940

Added CMakeLists.txt file for c++11 subdirectory.

3bf1874

Added global tracer.

d906d80

Added compiler warnings.

e91a6ae

Refactoring option classes.

315b677

Reformatting.

f423f3b

Specify to build both static and shared library.

c047dbf

Add README.md to c++98 directory.

739fb20

rnburn commented Jun 8, 2017

View reviewed changes

rnburn added 3 commits June 8, 2017 16:39

Added noexcept specifiers.

bc3043b

Support specification of times using time_since_epoch.

997b0eb

Specify Inject and Extract as const.

53d5951

rnburn commented Jun 8, 2017

View reviewed changes

rnburn added 7 commits June 8, 2017 20:55

Use system include for 3rd_party files.

56ad28b

Added Expected type for errors.

aee9d08

Add inline namespace for versioning.

b6dec43

Merge branch 'master' of github.com:rnburn/opentracing-cpp

3d4e03e

Add the StringRef optimizations from opentracing#7.

d528d51

Update style of StringRef class.

128b71e

Changed contracts on span methods.

dd3df3f

rnburn added 4 commits June 9, 2017 16:29

Added in stringref tests.

6929d1f

Updated README.md.

7f6d892

Corrections to README.md.

543c220

Run clang-format.

8bbbc91

jmacd approved these changes Jun 9, 2017

View reviewed changes

rnburn commented Jun 19, 2017

View reviewed changes

rnburn closed this Oct 13, 2017

Conversation

rnburn commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnburn Jun 8, 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!

rnburn Jun 8, 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!

rnburn Jun 8, 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!

rnburn Jun 8, 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!

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

rnburn commented Jun 8, 2017 •

edited

Loading

rnburn Jun 8, 2017 •

edited

Loading

rnburn Jun 8, 2017 •

edited

Loading

rnburn Jun 8, 2017 •

edited

Loading

rnburn Jun 8, 2017 •

edited

Loading