Address review comments

Author: cataphract

src/datadog/endpoint_inferral.cpp

@@ -37,6 +37,9 @@ enum component_type : std::uint8_t {
   is_hex_id = 1 << 3,
   is_str = 1 << 4,
 };
+static constexpr auto all_components =
+    is_int | is_int_id | is_hex | is_hex_id | is_str;
+static_assert(all_components == (is_str << 1) - 1);
 
 StringView to_string(component_type type) {
   switch (type) {
@@ -62,7 +65,7 @@ inline uint8_t bool2mask(bool x) { return x ? 0xFF : 0x00; }
 
 component_type component_replacement(StringView path) noexcept {
   // viable_components is a bitset of the component types not yet excluded
-  std::uint8_t viable_components = 0x1F;  // (is_str << 1) - 1
+  std::uint8_t viable_components = all_components;
   bool found_special_char = false;
   bool found_digit = false;
 
@@ -114,6 +117,7 @@ component_type component_replacement(StringView path) noexcept {
     return component_type::none;
   }
 
+  // Get least significant set bit to determine component w/ highest precedence
   // c++20: use std::countr_zero
   std::uint8_t lsb = static_cast<std::uint8_t>(
       viable_components &

src/datadog/trace_segment.cpp

@@ -81,6 +81,33 @@ void inject_trace_tags(
   }
 }
 
+void maybe_calculate_http_endpoint(HttpEndpointCalculationMode renaming_mode,
+                                   SpanData& local_root) {
+  // calculate http.endpoint if:
+  // a) the feature is not disabled, and
+  // b) the tag http.endpoint is not already set, and
+  // c) http.url is set, and
+  // d) http.route is not set or resource_renaming_mode is ALWAYS_CALCULATE
+  if (renaming_mode == HttpEndpointCalculationMode::DISABLED ||
+      local_root.tags.find(tags::http_endpoint) != local_root.tags.end()) {
+    return;
+  }
+  auto http_url_tag = local_root.tags.find(tags::http_url);
+  const bool should_calculate_endpoint =
+      http_url_tag != local_root.tags.end() &&
+      (renaming_mode == HttpEndpointCalculationMode::ALWAYS_CALCULATE ||
+       local_root.tags.find(tags::http_route) == local_root.tags.end());
+
+  if (should_calculate_endpoint) {
+    Expected<HTTPClient::URL> url_result =
+        HTTPClient::URL::parse(http_url_tag->second);
+    if (url_result.has_value()) {
+      const std::string& path = url_result->path;
+      local_root.tags[tags::http_endpoint] =
+          infer_endpoint(path.empty() ? "/" : path);
+    }
+  }
+}
 }  // namespace
 
 TraceSegment::TraceSegment(
@@ -250,30 +277,7 @@ void TraceSegment::span_finished() {
     span.tags[tags::internal::runtime_id] = runtime_id_.string();
   }
 
-  // calculate http.endpoint if:
-  // a) the feature is not disabled, and
-  // b) the tag http.endpoint is not already set, and
-  // c) http.url is set, and
-  // d) http.route is not set or resource_renaming_mode is ALWAYS_CALCULATE
-  if (resource_renaming_mode_ != HttpEndpointCalculationMode::DISABLED &&
-      local_root.tags.find(tags::http_endpoint) == local_root.tags.end()) {
-    auto http_url_tag = local_root.tags.find(tags::http_url);
-    const bool should_calculate_endpoint =
-        http_url_tag != local_root.tags.end() &&
-        (resource_renaming_mode_ ==
-             HttpEndpointCalculationMode::ALWAYS_CALCULATE ||
-         local_root.tags.find(tags::http_route) == local_root.tags.end());
-
-    if (should_calculate_endpoint) {
-      Expected<HTTPClient::URL> url_result =
-          HTTPClient::URL::parse(http_url_tag->second);
-      if (url_result.has_value()) {
-        const std::string& path = url_result->path;
-        local_root.tags[tags::http_endpoint] =
-            infer_endpoint(path.empty() ? "/" : path);
-      }
-    }
-  }
+  maybe_calculate_http_endpoint(resource_renaming_mode_, local_root);
 
   if (config_manager_->report_traces()) {
     telemetry::distribution::add(metrics::tracer::trace_chunk_size,

test/test_endpoint_inferral.cpp

@@ -32,15 +32,15 @@ TEST_ENDPOINT("hex and hex_id replacement") {
   CHECK(infer_endpoint("/x/ab_cd-9") == "/x/{param:hex_id}");
 }
 
-TEST_ENDPOINT("str replacement by special or length") {
-  CHECK(infer_endpoint("/x/a%z") == "/x/{param:str}");
+TEST_ENDPOINT("long sequences of more than 20 chars yield str") {
   std::string longseg(20, 'a');
   std::string path = std::string("/x/") + longseg;
   CHECK(infer_endpoint(path) == "/x/{param:str}");
 }
 
 TEST_ENDPOINT("other specials yield str") {
-  const char specials[] = {'&', '\'', '(', ')', '*', '+', ',', ':', '=', '@'};
+  const char specials[] = {'%', '&', '\'', '(', ')', '*',
+                           '+', ',', ':',  '=', '@'};
   for (char c : specials) {
     std::string s = "/x/a";
     s.push_back(c);