Add a CI job that builds dd-opentracing-cpp using bazel

.circleci/config.yml

@@ -3,7 +3,7 @@ version: 2
 defaults: &defaults
   working_directory: ~/dd-opentracing-cpp
   docker:
-    - image: datadog/docker-library:dd_opentracing_cpp_build_0_3_2
+    - image: datadog/docker-library:dd_opentracing_cpp_build_0_3_3
 
 jobs:
   build:
@@ -17,12 +17,12 @@ jobs:
       - run:
           name: Run clang-format
           command: |
-            find ./ -iname *.h -o -iname *.cpp | while read fname; do 
+            find include src test -iname '*.h' -o -iname '*.cpp' | while read fname; do 
               changes=$(clang-format-5.0 -output-replacements-xml $fname | grep -c "<replacement " || true)
               if [ $changes != 0 ]
               then
                 clang-format-5.0 -output-replacements-xml $fname
-                echo "$fname did not pass clang-format, consider running: find ./ -iname *.h -o -iname *.cpp | xargs clang-format -i"
+                echo "$fname did not pass clang-format, consider running: find include src test -iname '*.h' -o -iname '*.cpp' | xargs clang-format-5.0 -i"
                 exit 1
               fi
             done
@@ -31,7 +31,7 @@ jobs:
           command: |
             ./scripts/install_dependencies.sh
       - run:
-          name: Build
+          name: Build (with cmake)
           command: |
             rm -rf .build
             mkdir -p .build
@@ -40,6 +40,10 @@ jobs:
             cmake $CMAKE_ARGS ..
             make
             cp libdd_opentracing_plugin.so /tmp/build/libdd_opentracing_plugin.so
+      - run:
+          name: Build (with bazel)
+          command: |
+            bazel build //:dd_opentracing_cpp
       - run:
           name: Build opentracing-nginx
           command: |

BUILD.bazel

@@ -31,7 +31,15 @@ cc_library(
         "@io_opentracing_cpp//:opentracing",
         "@com_github_msgpack_msgpack_c//:msgpack",
     ],
-    copts = ["-std=c++14"],
+    copts = [
+        "-Wall",
+        "-Wextra",
+        "-Werror",
+        "-Wnon-virtual-dtor",
+        "-Woverloaded-virtual",
+        "-Wold-style-cast",
+        "-std=c++14",
+    ],
 )
 
 genrule(

CMakeLists.txt

@@ -67,6 +67,8 @@ include_directories(include)
 # Code
 install(DIRECTORY include/datadog DESTINATION include)
 file(GLOB DD_OPENTRACING_SOURCES "src/*.cpp")
+add_compile_options(-Wall -Wextra -Werror -Wnon-virtual-dtor -Woverloaded-virtual -Wold-style-cast -std=c++14)
+
 
 # Outputs
 ## Shared lib

src/noopspan.cpp

@@ -7,7 +7,8 @@ namespace datadog {
 namespace opentracing {
 
 NoopSpan::NoopSpan(std::shared_ptr<const Tracer> tracer, uint64_t span_id, uint64_t trace_id,
-                   uint64_t parent_id, SpanContext context, const ot::StartSpanOptions &options)
+                   uint64_t parent_id, SpanContext context,
+                   const ot::StartSpanOptions & /* options */)
     : tracer_(std::move(tracer)),
       span_id_(span_id),
       trace_id_(trace_id),
@@ -21,19 +22,22 @@ NoopSpan::NoopSpan(NoopSpan &&other)
       parent_id_(other.parent_id_),
       context_(std::move(other.context_)) {}
 
-void NoopSpan::FinishWithOptions(const ot::FinishSpanOptions &finish_span_options) noexcept {}
+void NoopSpan::FinishWithOptions(
+    const ot::FinishSpanOptions & /* finish_span_options */) noexcept {}
 
-void NoopSpan::SetOperationName(ot::string_view operation_name) noexcept {}
+void NoopSpan::SetOperationName(ot::string_view /* operation_name */) noexcept {}
 
-void NoopSpan::SetTag(ot::string_view key, const ot::Value &value) noexcept {}
+void NoopSpan::SetTag(ot::string_view /* key */, const ot::Value & /* value */) noexcept {}
 
-void NoopSpan::SetBaggageItem(ot::string_view restricted_key, ot::string_view value) noexcept {}
+void NoopSpan::SetBaggageItem(ot::string_view /* restricted_key */,
+                              ot::string_view /* value */) noexcept {}
 
 std::string NoopSpan::BaggageItem(ot::string_view restricted_key) const noexcept {
   return context_.baggageItem(restricted_key);
 }
 
-void NoopSpan::Log(std::initializer_list<std::pair<ot::string_view, ot::Value>> fields) noexcept {}
+void NoopSpan::Log(
+    std::initializer_list<std::pair<ot::string_view, ot::Value>> /* fields */) noexcept {}
 
 const ot::SpanContext &NoopSpan::context() const noexcept { return context_; }
 
@@ -43,7 +47,7 @@ uint64_t NoopSpan::traceId() const { return trace_id_; }
 uint64_t NoopSpan::spanId() const { return span_id_; }
 
 OptionalSamplingPriority NoopSpan::setSamplingPriority(
-    std::unique_ptr<UserSamplingPriority> priority) {
+    std::unique_ptr<UserSamplingPriority> /* priority */) {
   return nullptr;
 };
 OptionalSamplingPriority NoopSpan::getSamplingPriority() const { return nullptr; };

src/propagation.cpp

@@ -48,7 +48,8 @@ bool has_prefix(const std::string &str, const std::string &prefix) {
 }  // namespace
 
 OptionalSamplingPriority asSamplingPriority(int i) {
-  if (i < (int)SamplingPriority::MinimumValue || i > (int)SamplingPriority::MaximumValue) {
+  if (i < static_cast<int>(SamplingPriority::MinimumValue) ||
+      i > static_cast<int>(SamplingPriority::MaximumValue)) {
     return nullptr;
   }
   return std::make_unique<SamplingPriority>(static_cast<SamplingPriority>(i));

src/sample.cpp

@@ -41,13 +41,13 @@ bool DiscardRateSampler::discard(const SpanContext& context) const {
   return true;
 }
 
-OptionalSamplingPriority DiscardRateSampler::sample(const std::string& environment,
-                                                    const std::string& service,
-                                                    uint64_t trace_id) const {
+OptionalSamplingPriority DiscardRateSampler::sample(const std::string& /* environment */,
+                                                    const std::string& /* service */,
+                                                    uint64_t /* trace_id */) const {
   return nullptr;
 }
 
-bool PrioritySampler::discard(const SpanContext& context) const { return false; }
+bool PrioritySampler::discard(const SpanContext& /* context */) const { return false; }
 
 OptionalSamplingPriority PrioritySampler::sample(const std::string& environment,
                                                  const std::string& service,

src/span.cpp

@@ -112,7 +112,8 @@ void audit(SpanData *span) {
   }
 }
 
-void Span::FinishWithOptions(const ot::FinishSpanOptions &finish_span_options) noexcept try {
+void Span::FinishWithOptions(
+    const ot::FinishSpanOptions & /* finish_span_options */) noexcept try {
   if (is_finished_.exchange(true)) {
     return;
   }
@@ -309,7 +310,8 @@ std::string Span::BaggageItem(ot::string_view restricted_key) const noexcept {
   return context_.baggageItem(restricted_key);
 }
 
-void Span::Log(std::initializer_list<std::pair<ot::string_view, ot::Value>> fields) noexcept {}
+void Span::Log(
+    std::initializer_list<std::pair<ot::string_view, ot::Value>> /* fields */) noexcept {}
 
 OptionalSamplingPriority Span::setSamplingPriority(
     std::unique_ptr<UserSamplingPriority> user_priority) {

src/span_buffer.cpp

@@ -11,7 +11,7 @@ namespace {
 const std::string sampling_priority_metric = "_sampling_priority_v1";
 }  // namespace
 
-void PendingTrace::finish(const std::shared_ptr<SampleProvider>& sampler) {
+void PendingTrace::finish() {
   if (finished_spans->size() == 0) {
     std::cerr << "finish called on trace with no spans" << std::endl;
     return;  // I don't know why this would ever happen.
@@ -58,13 +58,12 @@ void WritingSpanBuffer::finishSpan(std::unique_ptr<SpanData> span,
   trace.finished_spans->push_back(std::move(span));
   if (trace.finished_spans->size() == trace.all_spans.size()) {
     assignSamplingPriorityImpl(sampler, trace.finished_spans->back().get());
-    trace.finish(sampler);
-    unbufferAndWriteTrace(trace_id, sampler);
+    trace.finish();
+    unbufferAndWriteTrace(trace_id);
   }
 }
 
-void WritingSpanBuffer::unbufferAndWriteTrace(uint64_t trace_id,
-                                              const std::shared_ptr<SampleProvider>& sampler) {
+void WritingSpanBuffer::unbufferAndWriteTrace(uint64_t trace_id) {
   auto trace_iter = traces_.find(trace_id);
   if (trace_iter == traces_.end()) {
     return;

src/span_buffer.h

@@ -20,7 +20,7 @@ struct PendingTrace {
   PendingTrace()
       : finished_spans(Trace{new std::vector<std::unique_ptr<SpanData>>()}), all_spans() {}
 
-  void finish(const std::shared_ptr<SampleProvider>& sampler);
+  void finish();
 
   Trace finished_spans;
   std::unordered_set<uint64_t> all_spans;
@@ -72,8 +72,7 @@ class WritingSpanBuffer : public SpanBuffer {
  protected:
   // Exists to make it easy for a subclass (ie, our testing mock) to override on-trace-finish
   // behaviour.
-  virtual void unbufferAndWriteTrace(uint64_t trace_id,
-                                     const std::shared_ptr<SampleProvider>& sampler);
+  virtual void unbufferAndWriteTrace(uint64_t trace_id);
 
   std::unordered_map<uint64_t, PendingTrace> traces_;
 };

src/tracer.h

@@ -43,6 +43,10 @@ class Tracer : public ot::Tracer, public std::enable_shared_from_this<Tracer> {
                                                  const ot::StartSpanOptions &options) const
       noexcept override;
 
+  // Ensure we aren't hiding methods from ot::Tracer due to the overloaded overrides.
+  using ot::Tracer::Extract;
+  using ot::Tracer::Inject;
+
   ot::expected<void> Inject(const ot::SpanContext &sc, std::ostream &writer) const override;
 
   ot::expected<void> Inject(const ot::SpanContext &sc,

src/transport.cpp

@@ -7,7 +7,7 @@ namespace datadog {
 namespace opentracing {
 
 size_t write_callback(char* ptr, size_t size, size_t nmemb, void* userdata) {
-  CurlHandle* handle = (CurlHandle*)userdata;
+  CurlHandle* handle = static_cast<CurlHandle*>(userdata);
   handle->response_buffer_.write(ptr, size * nmemb);
 
   if (!handle->response_buffer_) {
@@ -40,7 +40,7 @@ CurlHandle::CurlHandle() {
     throw std::runtime_error(std::string("Unable to set curl write callback: ") +
                              curl_easy_strerror(rcode));
   }
-  rcode = curl_easy_setopt(handle_, CURLOPT_WRITEDATA, (void*)this);
+  rcode = curl_easy_setopt(handle_, CURLOPT_WRITEDATA, static_cast<void*>(this));
   if (rcode != CURLE_OK) {
     tearDownHandle();
     throw std::runtime_error(std::string("Unable to set curl write callback userdata: ") +

test/mocks.h

@@ -35,9 +35,10 @@ struct TestSpanData : public SpanData {
 struct MockSampler : public PrioritySampler {
   MockSampler() {}
 
-  bool discard(const SpanContext& context) const override { return discard_spans; }
-  OptionalSamplingPriority sample(const std::string& environment, const std::string& service,
-                                  uint64_t trace_id) const override {
+  bool discard(const SpanContext& /* context */) const override { return discard_spans; }
+  OptionalSamplingPriority sample(const std::string& /* environment */,
+                                  const std::string& /* service */,
+                                  uint64_t /* trace_id */) const override {
     if (sampling_priority == nullptr) {
       return nullptr;
     }
@@ -73,8 +74,7 @@ struct MockBuffer : public WritingSpanBuffer {
   MockBuffer()
       : WritingSpanBuffer(std::make_shared<MockWriter>(std::make_shared<MockSampler>())){};
 
-  void unbufferAndWriteTrace(uint64_t trace_id,
-                             const std::shared_ptr<SampleProvider>& sampler) override {
+  void unbufferAndWriteTrace(uint64_t /* trace_id */) {
     // Haha NOPE.
     // Leave the trace inside the traces map instead of deleting it.
   }
@@ -215,7 +215,7 @@ struct MockTextMapCarrier : ot::TextMapReader, ot::TextMapWriter {
     return {};
   }
 
-  ot::expected<ot::string_view> LookupKey(ot::string_view key) const override {
+  ot::expected<ot::string_view> LookupKey(ot::string_view /* key */) const override {
     return ot::make_unexpected(ot::lookup_key_not_supported_error);
   }
 

test/sample_test.cpp

@@ -11,8 +11,12 @@
 using namespace datadog::opentracing;
 
 TEST_CASE("sample") {
-  int id = 100;                        // Starting span id.
-  std::tm start{0, 0, 0, 12, 2, 107};  // Starting calendar time 2007-03-12 00:00:00
+  int id = 100;  // Starting span id.
+  // Starting calendar time 2007-03-12 00:00:00
+  std::tm start{};
+  start.tm_mday = 12;
+  start.tm_mon = 2;
+  start.tm_year = 107;
   TimePoint time{std::chrono::system_clock::from_time_t(timegm(&start)),
                  std::chrono::steady_clock::time_point{}};
   auto buffer = std::make_shared<MockBuffer>();
@@ -133,7 +137,7 @@ TEST_CASE("priority sampler unit test") {
         REQUIRE(((*p == SamplingPriority::SamplerKeep) || (*p == SamplingPriority::SamplerDrop)));
         count_sampled += *p == SamplingPriority::SamplerKeep ? 1 : 0;
       }
-      double sample_rate = count_sampled / (double)total;
+      double sample_rate = count_sampled / static_cast<double>(total);
       REQUIRE((sample_rate < 0.85 && sample_rate > 0.75));
       // Case 2, service:nginx,env:prod => 0.2
       count_sampled = 0;
@@ -144,7 +148,7 @@ TEST_CASE("priority sampler unit test") {
         REQUIRE(((*p == SamplingPriority::SamplerKeep) || (*p == SamplingPriority::SamplerDrop)));
         count_sampled += *p == SamplingPriority::SamplerKeep ? 1 : 0;
       }
-      sample_rate = count_sampled / (double)total;
+      sample_rate = count_sampled / static_cast<double>(total);
       REQUIRE((sample_rate < 0.85 && sample_rate > 0.75));
     }
   }
@@ -225,7 +229,7 @@ TEST_CASE("priority sampler \"integration\" test") {
       REQUIRE(((*p == SamplingPriority::SamplerKeep) || (*p == SamplingPriority::SamplerDrop)));
       count_sampled += *p == SamplingPriority::SamplerKeep ? 1 : 0;
     }
-    double sample_rate = count_sampled / (double)total;
+    double sample_rate = count_sampled / static_cast<double>(total);
     REQUIRE((sample_rate < 0.35 && sample_rate > 0.25));
   }
 }

test/span_test.cpp

@@ -11,8 +11,12 @@ using namespace datadog::opentracing;
 using json = nlohmann::json;
 
 TEST_CASE("span") {
-  int id = 100;                        // Starting span id.
-  std::tm start{0, 0, 0, 12, 2, 107};  // Starting calendar time 2007-03-12 00:00:00
+  int id = 100;  // Starting span id.
+  // Starting calendar time 2007-03-12 00:00:00
+  std::tm start{};
+  start.tm_mday = 12;
+  start.tm_mon = 2;
+  start.tm_year = 107;
   TimePoint time{std::chrono::system_clock::from_time_t(timegm(&start)),
                  std::chrono::steady_clock::time_point{}};
   auto sampler = std::make_shared<KeepAllSampler>();

test/tracer_factory_test.cpp

@@ -12,41 +12,47 @@ using namespace datadog::opentracing;
 struct MockTracer : public ot::Tracer {
   TracerOptions opts;
 
-  MockTracer(TracerOptions opts_, std::shared_ptr<Writer> &writer,
-             std::shared_ptr<SampleProvider> sampler)
+  MockTracer(TracerOptions opts_, std::shared_ptr<Writer> & /* writer */,
+             std::shared_ptr<SampleProvider> /* sampler */)
       : opts(opts_) {}
 
-  std::unique_ptr<ot::Span> StartSpanWithOptions(ot::string_view operation_name,
-                                                 const ot::StartSpanOptions &options) const
+  std::unique_ptr<ot::Span> StartSpanWithOptions(ot::string_view /* operation_name */,
+                                                 const ot::StartSpanOptions & /* options */) const
       noexcept override {
     return nullptr;
   }
 
-  ot::expected<void> Inject(const ot::SpanContext &sc, std::ostream &writer) const override {
+  // This is here to avoid a warning about hidden overloaded-virtual methods.
+  using ot::Tracer::Extract;
+  using ot::Tracer::Inject;
+
+  ot::expected<void> Inject(const ot::SpanContext & /* sc */,
+                            std::ostream & /* writer */) const override {
     return ot::make_unexpected(ot::invalid_carrier_error);
   }
 
-  ot::expected<void> Inject(const ot::SpanContext &sc,
-                            const ot::TextMapWriter &writer) const override {
+  ot::expected<void> Inject(const ot::SpanContext & /* sc */,
+                            const ot::TextMapWriter & /* writer */) const override {
     return ot::make_unexpected(ot::invalid_carrier_error);
   }
 
-  ot::expected<void> Inject(const ot::SpanContext &sc,
-                            const ot::HTTPHeadersWriter &writer) const override {
+  ot::expected<void> Inject(const ot::SpanContext & /* sc */,
+                            const ot::HTTPHeadersWriter & /* writer */) const override {
     return ot::make_unexpected(ot::invalid_carrier_error);
   }
 
-  ot::expected<std::unique_ptr<ot::SpanContext>> Extract(std::istream &reader) const override {
+  ot::expected<std::unique_ptr<ot::SpanContext>> Extract(
+      std::istream & /* reader */) const override {
     return ot::make_unexpected(ot::invalid_carrier_error);
   }
 
   ot::expected<std::unique_ptr<ot::SpanContext>> Extract(
-      const ot::TextMapReader &reader) const override {
+      const ot::TextMapReader & /* reader */) const override {
     return ot::make_unexpected(ot::invalid_carrier_error);
   }
 
   ot::expected<std::unique_ptr<ot::SpanContext>> Extract(
-      const ot::HTTPHeadersReader &reader) const override {
+      const ot::HTTPHeadersReader & /* reader */) const override {
     return ot::make_unexpected(ot::invalid_carrier_error);
   }