Address review comments; preserve final /

Author: cataphract

BUILD.bazel

@@ -17,7 +17,7 @@ cc_library(
       "src/datadog/datadog_agent_config.cpp",
       "src/datadog/datadog_agent.cpp",
       "src/datadog/default_http_client_null.cpp",
-      "src/datadog/endpoint_guessing.cpp",
+      "src/datadog/endpoint_inferral.cpp",
       "src/datadog/environment.cpp",
       "src/datadog/error.cpp",
       "src/datadog/extraction_util.cpp",
@@ -61,7 +61,7 @@ cc_library(
       "src/datadog/datadog_agent.h",
       "src/datadog/default_http_client.h",
       "src/datadog/extracted_data.h",
-      "src/datadog/endpoint_guessing.h",
+      "src/datadog/endpoint_inferral.h",
       "src/datadog/extraction_util.h",
       "src/datadog/glob.h",
       "src/datadog/hex.h",

CMakeLists.txt

@@ -173,7 +173,7 @@ target_sources(dd-trace-cpp-objects
     src/datadog/collector_response.cpp
     src/datadog/datadog_agent_config.cpp
     src/datadog/datadog_agent.cpp
-    src/datadog/endpoint_guessing.cpp
+    src/datadog/endpoint_inferral.cpp
     src/datadog/environment.cpp
     src/datadog/error.cpp
     src/datadog/extraction_util.cpp

include/datadog/http_client.h

@@ -26,6 +26,7 @@ class HTTPClient {
     std::string scheme;     // http, https, or unix
     std::string authority;  // domain:port or /path/to/socket
     std::string path;       // resource, e.g. /v0.4/traces
+    std::string query;      // query string without '?'
 
     static Expected<HTTPClient::URL> parse(StringView input);
   };

src/datadog/endpoint_guessing.h

@@ -1,4 +1,4 @@
-#include "endpoint_guessing.h"
+#include "endpoint_inferral.h"
 
 #include <cstdint>
 
@@ -8,14 +8,14 @@ namespace {
 
 constexpr size_t MAX_COMPONENTS = 8;
 
-inline constexpr bool is_digit(char c) noexcept { return c >= '0' && c <= '9'; }
-inline constexpr bool is_hex_alpha(char c) noexcept {
+inline constexpr bool is_digit(char c) { return c >= '0' && c <= '9'; }
+inline constexpr bool is_hex_alpha(char c) {
   return (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F');
 }
-inline constexpr bool is_delim(char c) noexcept {
+inline constexpr bool is_delim(char c) {
   return c == '.' || c == '_' || c == '-';
 }
-inline constexpr bool is_str_special(char c) noexcept {
+inline constexpr bool is_str_special(char c) {
   return c == '%' || c == '&' || c == '\'' || c == '(' || c == ')' ||
          c == '*' || c == '+' || c == ',' || c == ':' || c == '=' || c == '@';
 }
@@ -38,7 +38,7 @@ enum component_type : std::uint8_t {
   is_str = 1 << 4,
 };
 
-std::string_view to_string(component_type type) noexcept {
+StringView to_string(component_type type) {
   switch (type) {
     case component_type::is_int:
       return "{param:int}";
@@ -50,16 +50,19 @@ std::string_view to_string(component_type type) noexcept {
       return "{param:hex_id}";
     case component_type::is_str:
       return "{param:str}";
-    default:
+    case component_type::none:
+      // should not be reached
       return "";
   }
+  // should never reach here
+  return "";
 }
 
-inline uint8_t bool2mask(bool x) noexcept {
-  return static_cast<uint8_t>(-int{x});  // 0 -> 0x00, 1 -> 0xFF
+inline uint8_t bool2mask(bool x) {
+  return x ? 0xFF : 0x00;
 }
 
-component_type component_replacement(std::string_view path) noexcept {
+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
   bool found_special_char = false;
@@ -121,31 +124,26 @@ component_type component_replacement(std::string_view path) noexcept {
 }
 }  // namespace
 
-std::string guess_endpoint(std::string_view orig_path) {
-  auto path = orig_path;
-
-  // remove the query string if any
-  auto query_pos = path.find('?');
-  if (query_pos != std::string_view::npos) {
-    path = path.substr(0, query_pos);
-  }
-
+std::string infer_endpoint(StringView path) {
+  // Expects a clean path without query string (e.g., "/api/users/123")
   if (path.empty() || path.front() != '/') {
     return "/";
   }
 
   std::string result{};
   size_t component_count = 0;
+  bool final_slash = true;
 
-  path.remove_prefix(1);
+  path.remove_prefix(1); // drop the leading '/'
   while (!path.empty()) {
     auto slash_pos = path.find('/');
 
-    std::string_view component = path.substr(0, slash_pos);
+    StringView component = path.substr(0, slash_pos);
 
-    // remove current component from the path
-    if (slash_pos == std::string_view::npos) {
-      path = std::string_view{};
+    // remove current component from the path (for the next iteration)
+    if (slash_pos == StringView::npos) {
+      path = StringView{};
+      final_slash = false;
     } else {
       path.remove_prefix(slash_pos + 1);
     }
@@ -173,6 +171,10 @@ std::string guess_endpoint(std::string_view orig_path) {
     return "/";
   }
 
+  if (final_slash) {
+    result.append("/");
+  }
+
   return result;
 }
 

src/datadog/endpoint_guessing.cpp src/datadog/endpoint_inferral.cppsrc/datadog/endpoint_guessing.cpp renamed to src/datadog/endpoint_inferral.cpp

@@ -0,0 +1,17 @@
+#pragma once
+
+#include <datadog/string_view.h>
+
+#include <string>
+
+namespace datadog::tracing {
+
+// Infer the endpoint pattern from a URL path by replacing parameters with
+// placeholders like {param:int}, {param:hex}, etc.
+//
+// The input should be a clean path without query string (e.g.,
+// "/api/users/123"). URL parsing should be handled by the caller using
+// HTTPClient::URL::parse().
+std::string infer_endpoint(StringView path);
+
+}

src/datadog/endpoint_inferral.h

@@ -61,7 +61,7 @@ Expected<HTTPClient::URL> HTTPClient::URL::parse(StringView input) {
                    std::move(message)};
     }
     return HTTPClient::URL{std::string(scheme), std::string(authority_and_path),
-                           ""};
+                           "", ""};
   }
 
   // The scheme is either "http" or "https".  This means that the part after
@@ -70,12 +70,25 @@ Expected<HTTPClient::URL> HTTPClient::URL::parse(StringView input) {
   // the Datadog Agent service, and so they will not have a resource
   // location.  Still, let's parse it properly.
   const auto after_authority = authority_and_path.find('/');
+
+  std::string path;
+  std::string query;
+  if (after_authority != StringView::npos) {
+    StringView path_and_query = authority_and_path.substr(after_authority);
+    const auto query_pos = path_and_query.find('?');
+    if (query_pos != StringView::npos) {
+      path = std::string(path_and_query.substr(0, query_pos));
+      query = std::string(path_and_query.substr(query_pos + 1));
+    } else {
+      path = std::string(path_and_query);
+    }
+  }
+
   return HTTPClient::URL{
       std::string(scheme),
       std::string(authority_and_path.substr(0, after_authority)),
-      (after_authority == StringView::npos)
-          ? ""
-          : std::string(authority_and_path.substr(after_authority))};
+      std::move(path),
+      std::move(query)};
 }
 
 }  // namespace tracing

src/datadog/http_client.cpp

@@ -10,6 +10,9 @@ const std::string span_type = "span.type";
 const std::string operation_name = "operation";
 const std::string resource_name = "resource.name";
 const std::string version = "version";
+const std::string http_endpoint = "http.endpoint";
+const std::string http_route = "http.route";
+const std::string http_url = "http.url";
 
 namespace internal {
 
@@ -30,9 +33,6 @@ const std::string process_id = "process_id";
 const std::string language = "language";
 const std::string runtime_id = "runtime-id";
 const std::string w3c_parent_id = "_dd.parent_id";
-const std::string http_endpoint = "http.endpoint";
-const std::string http_route = "http.route";
-const std::string http_url = "http.url";
 const std::string trace_source = "_dd.p.ts";
 const std::string apm_enabled = "_dd.apm.enabled";
 const std::string ksr = "_dd.p.ksr";

src/datadog/tags.cpp

@@ -19,6 +19,9 @@ extern const std::string span_type;
 extern const std::string operation_name;
 extern const std::string resource_name;
 extern const std::string version;
+extern const std::string http_endpoint;
+extern const std::string http_route;
+extern const std::string http_url;
 
 namespace internal {
 extern const std::string propagation_error;
@@ -38,9 +41,6 @@ extern const std::string process_id;
 extern const std::string language;
 extern const std::string runtime_id;
 extern const std::string w3c_parent_id;
-extern const std::string http_endpoint;
-extern const std::string http_route;
-extern const std::string http_url;
 extern const std::string trace_source;  // _dd.p.ts
 extern const std::string apm_enabled;   // _dd.apm.enabled
 extern const std::string ksr;           // _dd.p.ksr

src/datadog/tags.h

@@ -2,6 +2,7 @@
 #include <datadog/dict_reader.h>
 #include <datadog/dict_writer.h>
 #include <datadog/error.h>
+#include <datadog/http_client.h>
 #include <datadog/injection_options.h>
 #include <datadog/logger.h>
 #include <datadog/optional.h>
@@ -18,7 +19,7 @@
 #include <vector>
 
 #include "config_manager.h"
-#include "endpoint_guessing.h"
+#include "endpoint_inferral.h"
 #include "hex.h"
 #include "platform_util.h"
 #include "span_data.h"
@@ -254,33 +255,22 @@ void TraceSegment::span_finished() {
   // c) http.url is set, and
   // d) http.route is not set or resource_renaming_mode is ALWAYS_CALCULATE
   if (resource_renaming_mode_ != ResourceRenamingMode::DISABLED &&
-      local_root.tags.find(tags::internal::http_endpoint) ==
+      local_root.tags.find(tags::http_endpoint) ==
           local_root.tags.end()) {
-    auto http_url_tag = local_root.tags.find(tags::internal::http_url);
+    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_ == ResourceRenamingMode::ALWAYS_CALCULATE ||
-         local_root.tags.find(tags::internal::http_route) ==
+         local_root.tags.find(tags::http_route) ==
              local_root.tags.end());
 
     if (should_calculate_endpoint) {
-      // extract just the path from the http.url for endpoint guessing
-      // http.url format is: scheme://host:port/path?query
-      std::string_view path = http_url_tag->second;
-
-      auto scheme_end = path.find("://");
-      if (scheme_end != std::string_view::npos) {
-        path.remove_prefix(scheme_end + 3);  // skip scheme and ://
-        auto path_start = path.find('/');
-        if (path_start != std::string_view::npos) {
-          path.remove_prefix(path_start);
-        } else {
-          path = "/";
-        }
-
-        // query string is ignored by guess_endpoint
-
-        local_root.tags[tags::internal::http_endpoint] = guess_endpoint(path);
+      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);
       }
     }
   }