fix: split DD_TAGS either by space or comma (#105)

Author: dmehala

src/datadog/curl.cpp

@@ -225,7 +225,6 @@ class CurlImpl {
                                   void *user_data);
   static bool is_non_whitespace(unsigned char);
   static char to_lower(unsigned char);
-  static StringView trim(StringView);
 
  public:
   explicit CurlImpl(const std::shared_ptr<Logger> &, const Clock &,

src/datadog/parse_util.cpp

@@ -9,6 +9,7 @@
 #include <string>
 
 #include "error.h"
+#include "string_util.h"
 
 namespace datadog {
 namespace tracing {
@@ -154,28 +155,82 @@ Expected<std::unordered_map<std::string, std::string>> parse_tags(
     std::vector<StringView> list) {
   std::unordered_map<std::string, std::string> tags;
 
+  std::string key;
+  std::string value;
+
   for (const StringView &token : list) {
     const auto separator = std::find(token.begin(), token.end(), ':');
+
     if (separator == token.end()) {
-      std::string message;
-      message += "Unable to parse a key/value from the tag text \"";
-      append(message, token);
-      // message +=
-      //     "\" because it does not contain the separator character \":\".  "
-      //     "Error occurred in list of tags \"";
-      // append(message, input);
-      message += ".";
-      return Error{Error::TAG_MISSING_SEPARATOR, std::move(message)};
+      key = std::string{trim(token)};
+    } else {
+      key = std::string{trim(range(token.begin(), separator))};
+      if (key.empty()) {
+        continue;
+      }
+      value = std::string{trim(range(separator + 1, token.end()))};
     }
 
-    std::string key{token.begin(), separator};
-    std::string value{separator + 1, token.end()};
     // If there are duplicate values, then the last one wins.
     tags.insert_or_assign(std::move(key), std::move(value));
   }
 
   return tags;
 }
 
+// This function scans the input string to identify a separator (',' or ' ').
+// Then, split tags using the identified separator and call `parse_tags` with
+// the resulting unordered_map.
+//
+// Why?
+// RFC: DD_TAGS - support space separation
+//   The trace agent parses DD_TAGS as a space separated list of tags. The
+//   tracers parse this as a comma-separated list. We need to have the tracers
+//   parse DD_TAGS as a space-separated list when possible so that the agent and
+//   tracers can use the same DD_TAGS strings while maintaining backwards
+//   compatibility with comma-separated lists.
+Expected<std::unordered_map<std::string, std::string>> parse_tags(
+    StringView input) {
+  std::vector<StringView> tags;
+
+  size_t beg = 0;
+  const size_t end = input.size();
+
+  char separator = 0;
+  size_t i = beg;
+
+  for (; i < end; ++i) {
+    if (input[i] == ',') {
+      separator = ',';
+      break;
+    } else if (input[i] == ' ') {
+      separator = ' ';
+      break;
+    }
+  }
+
+  if (separator == 0) {
+    goto capture_all;
+  }
+
+  for (; i < end; ++i) {
+    if (input[i] == separator) {
+      auto tag = input.substr(beg, i - beg);
+      if (tag.size() > 0) {
+        tags.emplace_back(tag);
+      }
+      beg = i + 1;
+    }
+  }
+
+capture_all:
+  auto tag = input.substr(beg, i - beg);
+  if (tag.size() > 0) {
+    tags.emplace_back(tag);
+  }
+
+  return parse_tags(tags);
+}
+
 }  // namespace tracing
 }  // namespace datadog

src/datadog/parse_util.h

@@ -49,11 +49,8 @@ std::vector<StringView> parse_list(StringView input);
 Expected<std::unordered_map<std::string, std::string>> parse_tags(
     std::vector<StringView> list);
 
-inline Expected<std::unordered_map<std::string, std::string>> parse_tags(
-    StringView input) {
-  // Within a tag, the key and value are separated by a colon (":").
-  return parse_tags(parse_list(input));
-}
+Expected<std::unordered_map<std::string, std::string>> parse_tags(
+    StringView input);
 
 }  // namespace tracing
 }  // namespace datadog

src/datadog/string_util.cpp

@@ -70,5 +70,12 @@ std::string join_tags(
   });
 }
 
+StringView trim(StringView str) {
+  str.remove_prefix(std::min(str.find_first_not_of(' '), str.size()));
+  const auto pos = str.find_last_not_of(' ');
+  if (pos != str.npos) str.remove_suffix(str.size() - pos - 1);
+  return str;
+}
+
 }  // namespace tracing
 }  // namespace datadog

src/datadog/string_util.h

@@ -4,6 +4,7 @@
 #include <unordered_map>
 
 #include "propagation_style.h"
+#include "string_view.h"
 
 namespace datadog {
 namespace tracing {
@@ -25,5 +26,7 @@ std::string join_propagation_styles(const std::vector<PropagationStyle>&);
 std::string join_tags(
     const std::unordered_map<std::string, std::string>& values);
 
+StringView trim(StringView);
+
 }  // namespace tracing
 }  // namespace datadog

test/test_parse_util.cpp

@@ -11,7 +11,9 @@
 
 using namespace datadog::tracing;
 
-TEST_CASE("parse_int") {
+#define PARSE_UTIL_TEST(x) TEST_CASE(x, "[parse_util]")
+
+PARSE_UTIL_TEST("parse_int") {
   struct TestCase {
     int line;
     std::string name;
@@ -74,7 +76,7 @@ TEST_CASE("parse_int") {
 
 // This test case is similar to the one above, except that negative numbers are
 // not supported, and the underflow and overflow values are different.
-TEST_CASE("parse_uint64") {
+PARSE_UTIL_TEST("parse_uint64") {
   struct TestCase {
     int line;
     std::string name;
@@ -133,3 +135,101 @@ TEST_CASE("parse_uint64") {
     REQUIRE(result.error().code == expected);
   }
 }
+
+PARSE_UTIL_TEST("parse_tags") {
+  struct TestCase {
+    int line;
+    StringView name;
+    StringView input;
+    std::unordered_map<std::string, std::string> expected;
+  };
+
+  auto test_case = GENERATE(values<TestCase>({
+      {__LINE__,
+       "space separated tags",
+       "env:test aKey:aVal bKey:bVal cKey:",
+       {
+           {"env", "test"},
+           {"aKey", "aVal"},
+           {"bKey", "bVal"},
+           {"cKey", ""},
+       }},
+      {__LINE__,
+       "comma separated tags",
+       "env:test aKey:aVal bKey:bVal cKey:",
+       {
+           {"env", "test"},
+           {"aKey", "aVal"},
+           {"bKey", "bVal"},
+           {"cKey", ""},
+       }},
+      {__LINE__,
+       "mixed separator but comma first",
+       "env:test,aKey:aVal bKey:bVal cKey:",
+       {
+           {"env", "test"},
+           {"aKey", "aVal bKey:bVal cKey:"},
+       }},
+      {__LINE__,
+       "mixed separator but space first",
+       "env:test     bKey :bVal dKey: dVal cKey:",
+       {
+           {"env", "test"},
+           {"bKey", ""},
+           {"dKey", ""},
+           {"dVal", ""},
+           {"cKey", ""},
+       }},
+      {__LINE__,
+       "mixed separator but space first",
+       "env:keyWithA:Semicolon bKey:bVal cKey",
+       {
+           {"env", "keyWithA:Semicolon"},
+           {"bKey", "bVal"},
+           {"cKey", ""},
+       }},
+      // {__LINE__,
+      //  "mixed separator edge case",
+      //  "env:keyWith:  , ,   Lots:Of:Semicolons ",
+      //  {
+      //      {"env", "keyWith:"},
+      //      {"Lots", "Of:Semicolons"},
+      //  }},
+      {__LINE__,
+       "comma separated but some tags without value",
+       "a:b,c,d",
+       {
+           {"a", "b"},
+           {"c", ""},
+           {"d", ""},
+       }},
+      {__LINE__,
+       "one separator without value",
+       "a,1",
+       {
+           {"a", ""},
+           {"1", ""},
+       }},
+      {__LINE__,
+       "no separator",
+       "a:b:c:d",
+       {
+           {"a", "b:c:d"},
+       }},
+      {__LINE__,
+       "input is trimed",
+       "key1:val1, key2 : val2 ",
+       {
+           {"key1", "val1"},
+           {"key2", "val2"},
+       }},
+  }));
+
+  CAPTURE(test_case.line);
+  CAPTURE(test_case.name);
+  CAPTURE(test_case.input);
+
+  const auto tags = parse_tags(test_case.input);
+  REQUIRE(tags);
+  CHECK(*tags == test_case.expected);
+}

test/test_tracer_config.cpp

@@ -213,21 +213,45 @@ TEST_CASE("TracerConfig::defaults") {
 
     auto test_case = GENERATE(values<TestCase>({
         {"empty", "", {}, nullopt},
-        {"missing colon", "foo", {}, Error::TAG_MISSING_SEPARATOR},
+        {"missing colon",
+         "foo",
+         {
+             {"foo", ""},
+         },
+         nullopt},
         {"trailing comma",
          "foo:bar, baz:123,",
-         {},
-         Error::TAG_MISSING_SEPARATOR},
-        {"overwrite value", "foo:baz", {{"foo", "baz"}}, nullopt},
+         {
+             {"foo", "bar"},
+             {"baz", "123"},
+         },
+         nullopt},
+        {"overwrite value",
+         "foo:baz",
+         {
+             {"foo", "baz"},
+         },
+         nullopt},
         {"additional values",
          "baz:123, bam:three",
-         {{"baz", "123"}, {"bam", "three"}},
+         {
+             {"baz", "123"},
+             {"bam", "three"},
+         },
          nullopt},
         {"commas optional",
          "baz:123 bam:three",
-         {{"baz", "123"}, {"bam", "three"}},
+         {
+             {"baz", "123"},
+             {"bam", "three"},
+         },
+         nullopt},
+        {"last one wins",
+         "baz:123 baz:three",
+         {
+             {"baz", "three"},
+         },
          nullopt},
-        {"last one wins", "baz:123 baz:three", {{"baz", "three"}}, nullopt},
     }));
 
     // This will be overriden by the DD_TAGS environment variable.
@@ -1053,8 +1077,8 @@ TEST_CASE("TracerConfig::span_sampler") {
           REQUIRE(file.is_open());
           // We could do any of the failures tested in the "must be valid"
           // section, since it's the same parser. Instead, just to cover the
-          // code path specific to DD_SPAN_SAMPLING_RULES_FILE, pick any error,
-          // e.g. invalid JSON.
+          // code path specific to DD_SPAN_SAMPLING_RULES_FILE, pick any
+          // error, e.g. invalid JSON.
           file << "this is clearly not JSON";
           file.close();
           const EnvGuard guard{"DD_SPAN_SAMPLING_RULES_FILE",