chore: enforce codestyle (#60)

Co-authored-by: David Goffredo <[email protected]>

Author: dmehala

.circleci/config.yml

@@ -117,6 +117,23 @@ jobs:
         path: test/logs/test.log
         destination: test.log
 
+  format:
+    docker:
+    - image: "datadog/docker-library:dd-trace-cpp-ci"
+    resource_class: small
+    steps:
+    - checkout
+    - run:
+        name: Install Python dependencies
+        command: |
+          pip install yapf
+          update-alternatives --install /usr/local/bin/yapf3 yapf3 /usr/local/bin/yapf 100
+    - run:
+        name: Lint
+        command: |
+          touch nginx-version-info
+          make lint
+
   shellcheck:
     docker:
     - image: koalaman/shellcheck-alpine:stable
@@ -128,6 +145,7 @@ jobs:
 workflows:
   build-and-test:
     jobs:
+    - format
     - shellcheck:
         name: "run shellcheck on shell scripts"
         filters:

.clang-format

@@ -0,0 +1 @@
+dd-trace-cpp/.clang-format

.gitignore

@@ -10,9 +10,6 @@
 .*.sw?
 .vscode/
 
-# clang-format config copied from dd-opentracing-cpp (see `make format`)
-/.clang-format
-
 # file used as part of the build configuration
 /nginx-version-info
 

Makefile

@@ -44,13 +44,14 @@ nginx-version-info:
 dd-trace-cpp/.clang-format: dd-trace-cpp/.git
 
 .clang-format: dd-trace-cpp/.clang-format
-	cp $< $@
 
 .PHONY: format
 format: .clang-format
-	find src/ -type f \( -name '*.h' -o -name '*.cpp' \) -print0 | xargs -0 clang-format-14 -i --style=file
-	find bin/ -type f -name '*.py' -print0 | xargs -0 yapf3 -i
-	test/bin/format
+	bin/format.sh
+
+.PHONY: lint
+lint: .clang-format
+	bin/lint.sh
 
 .PHONY: clean
 clean:

bin/format.sh

@@ -0,0 +1,12 @@
+#!/bin/sh
+# Format the codebase to follow this project's style.
+
+if ! [ -e .clang-format ]; then
+    >&2 echo '.clang-format file is missing. Run "make format".'
+    exit 1
+fi
+
+find src/ -type f \( -name '*.h' -o -name '*.cpp' \) -print0 | xargs -0 clang-format-14 -i --style=file
+find bin/ -type f -name '*.py' -print0 | xargs -0 yapf3 -i
+
+yapf3 --recursive --in-place "$@" "test/"

bin/lint.sh

@@ -0,0 +1,33 @@
+#!/bin/sh
+# Print any discrepancies between the formatting of the code and the expected
+# style.
+
+if ! [ -e .clang-format ]; then
+    >&2 echo '.clang-format file is missing. Run "make lint".'
+    exit 1
+fi
+
+error_messages=''
+
+find src/ -type f \( -name '*.h' -o -name '*.cpp' \) -print0 | xargs -0 clang-format-14 --Werror --dry-run --style=file
+rc=$?
+if [ "$rc" -ne 0 ]; then
+    error_messages=$(printf '%s\nC++ formatter reported formatting differences in src/ and returned error status %d.\n' "$error_messages" "$rc")
+fi
+
+find bin/ -type f -name '*.py' -print0 | xargs -0 yapf3 --diff
+rc=$?
+if [ "$rc" -ne 0 ]; then
+    error_messages=$(printf '%s\nPython formatter reported formatting differences in bin/ and returned error status %d.\n' "$error_messages" "$rc")
+fi
+
+yapf3 --recursive --diff "$@" "test/"
+rc=$?
+if [ "$rc" -ne 0 ]; then
+    error_messages=$(printf '%s\nPython formatter reported formatting differences in test/ and returned error status %d.\n' "$error_messages" "$rc")
+fi
+
+if [ -n "$error_messages" ]; then
+    >&2 echo "$error_messages"
+    exit 1
+fi

src/datadog_conf.cpp

@@ -7,7 +7,8 @@ namespace nginx {
 
 bool operator==(const conf_directive_source_location_t& left,
                 const conf_directive_source_location_t& right) {
-  return str(left.file_name) == str(right.file_name) && left.line == right.line &&
+  return str(left.file_name) == str(right.file_name) &&
+         left.line == right.line &&
          str(left.directive_name) == str(right.directive_name);
 }
 

src/datadog_conf.h

@@ -213,7 +213,8 @@ struct datadog_loc_conf_t {
   // `allow_sampling_delegation_in_subrequests_directive` is the source location
   // of the `datadog_allow_sampling_delegation_in_subrequests` directive that
   // applies this location, if any.
-  conf_directive_source_location_t allow_sampling_delegation_in_subrequests_directive;
+  conf_directive_source_location_t
+      allow_sampling_delegation_in_subrequests_directive;
 };
 
 }  // namespace nginx

src/datadog_conf_handler.cpp

@@ -5,9 +5,9 @@
 namespace datadog {
 namespace nginx {
 /* The eight fixed arguments */
-static ngx_uint_t argument_number[] = {NGX_CONF_NOARGS, NGX_CONF_TAKE1, NGX_CONF_TAKE2,
-                                       NGX_CONF_TAKE3,  NGX_CONF_TAKE4, NGX_CONF_TAKE5,
-                                       NGX_CONF_TAKE6,  NGX_CONF_TAKE7};
+static ngx_uint_t argument_number[] = {
+    NGX_CONF_NOARGS, NGX_CONF_TAKE1, NGX_CONF_TAKE2, NGX_CONF_TAKE3,
+    NGX_CONF_TAKE4,  NGX_CONF_TAKE5, NGX_CONF_TAKE6, NGX_CONF_TAKE7};
 
 ngx_int_t datadog_conf_handler(const DatadogConfHandlerConfig &args) noexcept {
   ngx_conf_t *const cf = args.conf;
@@ -30,7 +30,8 @@ ngx_int_t datadog_conf_handler(const DatadogConfHandlerConfig &args) noexcept {
       continue;
     }
 
-    if (args.skip_this_module && cf->cycle->modules[i] == &ngx_http_datadog_module) {
+    if (args.skip_this_module &&
+        cf->cycle->modules[i] == &ngx_http_datadog_module) {
       continue;
     }
 
@@ -57,14 +58,15 @@ ngx_int_t datadog_conf_handler(const DatadogConfHandlerConfig &args) noexcept {
       }
 
       if (!(cmd->type & NGX_CONF_BLOCK) && last != NGX_OK) {
-        ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, "directive \"%s\" is not terminated by \";\"",
+        ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
+                           "directive \"%s\" is not terminated by \";\"",
                            name->data);
         return NGX_ERROR;
       }
 
       if ((cmd->type & NGX_CONF_BLOCK) && last != NGX_CONF_BLOCK_START) {
-        ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, "directive \"%s\" has no opening \"{\"",
-                           name->data);
+        ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
+                           "directive \"%s\" has no opening \"{\"", name->data);
         return NGX_ERROR;
       }
 
@@ -122,25 +124,29 @@ ngx_int_t datadog_conf_handler(const DatadogConfHandlerConfig &args) noexcept {
         return NGX_ERROR;
       }
 
-      ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, "\"%s\" directive %s", name->data, rv);
+      ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, "\"%s\" directive %s",
+                         name->data, rv);
 
       return NGX_ERROR;
     }
   }
 
   if (found) {
-    ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, "\"%s\" directive is not allowed here", name->data);
+    ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
+                       "\"%s\" directive is not allowed here", name->data);
 
     return NGX_ERROR;
   }
 
-  ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, "unknown directive \"%s\"", name->data);
+  ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, "unknown directive \"%s\"",
+                     name->data);
 
   return NGX_ERROR;
 
 invalid:
 
-  ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, "invalid number of arguments in \"%s\" directive",
+  ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
+                     "invalid number of arguments in \"%s\" directive",
                      name->data);
 
   return NGX_ERROR;

src/datadog_context.cpp

@@ -28,19 +28,21 @@ void DatadogContext::on_change_block(ngx_http_request_t *request,
 
   // This is a new subrequest, so add a RequestTracing for it.
   // TODO: Should `active_span` be `request_span` instead?
-  traces_.emplace_back(request, core_loc_conf, loc_conf, &traces_[0].active_span());
+  traces_.emplace_back(request, core_loc_conf, loc_conf,
+                       &traces_[0].active_span());
 }
 
 void DatadogContext::on_log_request(ngx_http_request_t *request) {
   auto trace = find_trace(request);
   if (trace == nullptr) {
-    throw std::runtime_error{"on_log_request failed: could not find request trace"};
+    throw std::runtime_error{
+        "on_log_request failed: could not find request trace"};
   }
   trace->on_log_request();
 }
 
-ngx_str_t DatadogContext::lookup_propagation_header_variable_value(ngx_http_request_t *request,
-                                                                   std::string_view key) {
+ngx_str_t DatadogContext::lookup_propagation_header_variable_value(
+    ngx_http_request_t *request, std::string_view key) {
   auto trace = find_trace(request);
   if (trace == nullptr) {
     throw std::runtime_error{
@@ -50,11 +52,12 @@ ngx_str_t DatadogContext::lookup_propagation_header_variable_value(ngx_http_requ
   return trace->lookup_propagation_header_variable_value(key);
 }
 
-ngx_str_t DatadogContext::lookup_span_variable_value(ngx_http_request_t *request,
-                                                     std::string_view key) {
+ngx_str_t DatadogContext::lookup_span_variable_value(
+    ngx_http_request_t *request, std::string_view key) {
   auto trace = find_trace(request);
   if (trace == nullptr) {
-    throw std::runtime_error{"lookup_span_variable_value failed: could not find request trace"};
+    throw std::runtime_error{
+        "lookup_span_variable_value failed: could not find request trace"};
   }
   return trace->lookup_span_variable_value(key);
 }
@@ -64,21 +67,24 @@ ngx_str_t DatadogContext::lookup_sampling_delegation_response_variable_value(
   auto trace = find_trace(request);
   if (trace == nullptr) {
     throw std::runtime_error{
-        "lookup_sampling_delegation_response_variable_value failed: could not find request trace"};
+        "lookup_sampling_delegation_response_variable_value failed: could not "
+        "find request trace"};
   }
   return trace->lookup_sampling_delegation_response_variable_value();
 }
 
 RequestTracing *DatadogContext::find_trace(ngx_http_request_t *request) {
-  const auto found = std::find_if(traces_.begin(), traces_.end(),
-                                  [=](const auto &trace) { return trace.request() == request; });
+  const auto found = std::find_if(
+      traces_.begin(), traces_.end(),
+      [=](const auto &trace) { return trace.request() == request; });
   if (found != traces_.end()) {
     return &*found;
   }
   return nullptr;
 }
 
-const RequestTracing *DatadogContext::find_trace(ngx_http_request_t *request) const {
+const RequestTracing *DatadogContext::find_trace(
+    ngx_http_request_t *request) const {
   return const_cast<DatadogContext *>(this)->find_trace(request);
 }
 
@@ -87,7 +93,8 @@ static void cleanup_datadog_context(void *data) noexcept {
 }
 
 static ngx_pool_cleanup_t *find_datadog_cleanup(ngx_http_request_t *request) {
-  for (auto cleanup = request->pool->cleanup; cleanup; cleanup = cleanup->next) {
+  for (auto cleanup = request->pool->cleanup; cleanup;
+       cleanup = cleanup->next) {
     if (cleanup->handler == cleanup_datadog_context) {
       return cleanup;
     }
@@ -96,8 +103,8 @@ static ngx_pool_cleanup_t *find_datadog_cleanup(ngx_http_request_t *request) {
 }
 
 DatadogContext *get_datadog_context(ngx_http_request_t *request) noexcept {
-  auto context =
-      static_cast<DatadogContext *>(ngx_http_get_module_ctx(request, ngx_http_datadog_module));
+  auto context = static_cast<DatadogContext *>(
+      ngx_http_get_module_ctx(request, ngx_http_datadog_module));
   if (context != nullptr || !request->internal) {
     return context;
   }
@@ -114,7 +121,8 @@ DatadogContext *get_datadog_context(ngx_http_request_t *request) noexcept {
   // If we found a context, attach with ngx_http_set_ctx so that we don't have
   // to loop through the cleanup handlers again.
   if (context != nullptr) {
-    ngx_http_set_ctx(request, static_cast<void *>(context), ngx_http_datadog_module);
+    ngx_http_set_ctx(request, static_cast<void *>(context),
+                     ngx_http_datadog_module);
   }
 
   return context;
@@ -138,7 +146,8 @@ void set_datadog_context(ngx_http_request_t *request, DatadogContext *context) {
   }
   cleanup->data = static_cast<void *>(context);
   cleanup->handler = cleanup_datadog_context;
-  ngx_http_set_ctx(request, static_cast<void *>(context), ngx_http_datadog_module);
+  ngx_http_set_ctx(request, static_cast<void *>(context),
+                   ngx_http_datadog_module);
 }
 
 // Supports early destruction of the DatadogContext (in case of an
@@ -147,7 +156,8 @@ void destroy_datadog_context(ngx_http_request_t *request) noexcept {
   auto cleanup = find_datadog_cleanup(request);
   if (cleanup == nullptr) {
     ngx_log_error(NGX_LOG_ERR, request->connection->log, 0,
-                  "Unable to find Datadog cleanup handler for request %p", request);
+                  "Unable to find Datadog cleanup handler for request %p",
+                  request);
     return;
   }
   delete static_cast<DatadogContext *>(cleanup->data);