Merge branch 'master' into cgilmour/bazel

DataDog · Aug 1, 2018 · a9083f0 · a9083f0
2 parents 00987c2 + bcba717
commit a9083f0
Show file tree

Hide file tree

Showing 4 changed files with 78 additions and 94 deletions.
diff --git a/src/span.cpp b/src/span.cpp
@@ -1,6 +1,7 @@
 #include "span.h"
 #include <iostream>
 #include <nlohmann/json.hpp>
+#include <regex>
 
 namespace ot = opentracing;
 using json = nlohmann::json;
@@ -12,6 +13,7 @@ namespace {
 const std::string datadog_span_type_tag = "span.type";
 const std::string datadog_resource_name_tag = "resource.name";
 const std::string datadog_service_name_tag = "service.name";
+const std::string http_url_tag = "http.url";
 }  // namespace
 
 SpanData::SpanData(std::string type, std::string service, ot::string_view resource,
@@ -66,6 +68,25 @@ Span::~Span() {
   }
 }
 
+namespace {
+// Matches path segments with numbers (except things that look like versions).
+// Similar to, but not the same as,
+// https://github.com/datadog/dd-trace-java/blob/master/dd-trace-ot/src/main/java/datadog/opentracing/decorators/URLAsResourceName.java#L14-L16
+std::regex PATH_MIXED_ALPHANUMERICS{
+    "(\\/)(?:(?:([[:alpha:]]*)(?:\\?[^\\/]*))|(?:(?![vV]\\d{1,2}\\/)[^\\/\\d\\?]*[\\d]+[^\\/]*))"};
+}  // namespace
+
+// Imperfectly audits the data in a Span, removing some things that could cause information leaks
+// or cardinality issues.
+// If you want to add any more steps to this function, we should use a more
+// sophisticated architecture. For now, YAGNI.
+void audit(SpanData *span) {
+  auto http_tag = span->meta.find(http_url_tag);
+  if (http_tag != span->meta.end()) {
+    http_tag->second = std::regex_replace(http_tag->second, PATH_MIXED_ALPHANUMERICS, "$1$2?");
+  }
+}
+
 void Span::FinishWithOptions(const ot::FinishSpanOptions &finish_span_options) noexcept try {
   if (is_finished_.exchange(true)) {
     return;
@@ -75,6 +96,7 @@ void Span::FinishWithOptions(const ot::FinishSpanOptions &finish_span_options) n
   auto end_time = get_time_();
   span_->duration =
       std::chrono::duration_cast<std::chrono::nanoseconds>(end_time - start_time_).count();
+  audit(span_.get());
   buffer_->finishSpan(std::move(span_));
   // According to the OT lifecycle, no more methods should be called on this Span. But just in case
   // let's make sure that span_ isn't nullptr. Fine line between defensive programming and voodoo.

diff --git a/test/integration/nginx/expected.json b/test/integration/nginx/expected.json
@@ -1,19 +1,5 @@
 [
   [
-    {
-      "error": 0,
-      "meta": {
-        "_sample_rate": "1.000000",
-        "component": "nginx",
-        "http.method": "GET",
-        "http.status_line": "",
-        "http.url": "http://localhost/"
-      },
-      "name": "/",
-      "resource": "/",
-      "service": "nginx",
-      "type": "web"
-    },
     {
       "error": 0,
       "meta": {
@@ -24,42 +10,13 @@
         "http.status_line": "",
         "http.url": "http://localhost/"
       },
-      "name": "/",
-      "resource": "/",
-      "service": "nginx",
-      "type": "web"
-    },
-    {
-      "error": 0,
-      "meta": {
-        "_sample_rate": "1.000000",
-        "component": "nginx",
-        "http.method": "GET",
-        "http.status_code": "200",
-        "http.status_line": "",
-        "http.url": "http://localhost/"
-      },
-      "name": "/index.html",
-      "resource": "/index.html",
+      "name": "GET /index.html",
+      "resource": "GET /index.html",
       "service": "nginx",
       "type": "web"
     }
   ],
   [
-    {
-      "error": 0,
-      "meta": {
-        "_sample_rate": "1.000000",
-        "component": "nginx",
-        "http.method": "GET",
-        "http.status_line": "",
-        "http.url": "http://localhost/"
-      },
-      "name": "/",
-      "resource": "/",
-      "service": "nginx",
-      "type": "web"
-    },
     {
       "error": 0,
       "meta": {
@@ -70,57 +27,13 @@
         "http.status_line": "",
         "http.url": "http://localhost/"
       },
-      "name": "/",
-      "resource": "/",
-      "service": "nginx",
-      "type": "web"
-    },
-    {
-      "error": 0,
-      "meta": {
-        "_sample_rate": "1.000000",
-        "component": "nginx",
-        "http.method": "GET",
-        "http.status_code": "200",
-        "http.status_line": "",
-        "http.url": "http://localhost/"
-      },
-      "name": "/index.html",
-      "resource": "/index.html",
+      "name": "GET /index.html",
+      "resource": "GET /index.html",
       "service": "nginx",
       "type": "web"
     }
   ],
   [
-    {
-      "error": 0,
-      "meta": {
-        "_sample_rate": "1.000000",
-        "component": "nginx",
-        "http.method": "GET",
-        "http.status_line": "",
-        "http.url": "http://localhost/"
-      },
-      "name": "/",
-      "resource": "/",
-      "service": "nginx",
-      "type": "web"
-    },
-    {
-      "error": 0,
-      "meta": {
-        "_sample_rate": "1.000000",
-        "component": "nginx",
-        "http.method": "GET",
-        "http.status_code": "200",
-        "http.status_line": "",
-        "http.url": "http://localhost/"
-      },
-      "name": "/",
-      "resource": "/",
-      "service": "nginx",
-      "type": "web"
-    },
     {
       "error": 0,
       "meta": {
@@ -131,8 +44,8 @@
         "http.status_line": "",
         "http.url": "http://localhost/"
       },
-      "name": "/index.html",
-      "resource": "/index.html",
+      "name": "GET /index.html",
+      "resource": "GET /index.html",
       "service": "nginx",
       "type": "web"
     }

diff --git a/test/integration/nginx/nginx.conf b/test/integration/nginx/nginx.conf
@@ -7,6 +7,7 @@ events {
 http {
     opentracing on;
     opentracing_tag http_user_agent $http_user_agent;
+    opentracing_trace_locations off;
 
     opentracing_load_tracer /usr/local/lib/libdd_opentracing_plugin.so /etc/dd-config.json;
 
@@ -15,7 +16,7 @@ http {
         server_name  localhost;
 
         location / {
-            opentracing_operation_name "$uri";
+            opentracing_operation_name "$request_method $uri";
             root   /var/www;
         }
     }

diff --git a/test/span_test.cpp b/test/span_test.cpp
@@ -84,6 +84,54 @@ TEST_CASE("span") {
     REQUIRE(result->duration == 10000000000);
   }
 
+  SECTION("audits span data") {
+    std::list<std::pair<std::string, std::string>> test_cases{
+        // Should remove query params
+        {"/", "/"},
+        {"/?asdf", "/?"},
+        {"/search", "/search"},
+        {"/search?", "/search?"},
+        {"/search?id=100&private=true", "/search?"},
+        {"/search?id=100&private=true?", "/search?"},
+        // Should replace all digits
+        {"/1", "/?"},
+        {"/9999", "/?"},
+        {"/user/1", "/user/?"},
+        {"/user/1/", "/user/?/"},
+        {"/user/1/repo/50", "/user/?/repo/?"},
+        {"/user/1/repo/50/", "/user/?/repo/?/"},
+        // Should replace segments with mixed-characters
+        {"/a1/v2", "/?/?"},
+        {"/v3/1a", "/v3/?"},
+        {"/V01/v9/abc/-1?", "/V01/v9/abc/?"},
+        {"/ABC/av-1/b_2/c.3/d4d/v5f/v699/7", "/ABC/?/?/?/?/?/?/?"},
+        {"/user/asdf123/repository/01234567-9ABC-DEF0-1234", "/user/?/repository/?"},
+        {"/ABC/a-1/b_2/c.3/d4d/5f/6", "/ABC/?/?/?/?/?/?"}};
+
+    std::shared_ptr<SpanBuffer> buffer_ptr{buffer};
+    for (auto& test_case : test_cases) {
+      auto span_id = get_id();
+      Span span{nullptr,
+                buffer_ptr,
+                get_time,
+                span_id,
+                span_id,
+                0,
+                std::move(SpanContext{span_id, span_id, {}}),
+                get_time(),
+                "",
+                "",
+                "",
+                ""};
+      span.SetTag("http.url", test_case.first);
+      const ot::FinishSpanOptions finish_options;
+      span.FinishWithOptions(finish_options);
+
+      auto& result = buffer->traces[span_id].finished_spans->back();
+      REQUIRE(result->meta.find("http.url")->second == test_case.second);
+    }
+  }
+
   SECTION("finishes once") {
     auto span_id = get_id();
     Span span{nullptr,