Revise resource names: (#8)

* Revise resource names:

- Add configuration directive `datadog_resource_name` for setting the resource name of the request span.
- Add configuration directive `datadog_location_resource_name` for setting the resource name of the location span.
- Change the default resource name to be `$request_method $uri` instead of `$request_method $datadog_location`.

Author: dgoffredo

README.md

@@ -41,7 +41,7 @@ default tracing behavior to nginx:
 - Create one span per request:
     - Service name is "nginx".
     - Operation name is "nginx.request".
-    - Resource name is `"$request_method $datadog_location"`, e.g. "GET /api".
+    - Resource name is `"$request_method $uri"`, e.g. "GET /api/book/0-345-24223-8/title".
     - Includes multiple `http.*` [tags][8].
 
 Custom configuration can be specified via the [datadog](doc/API.md#datadog)

doc/API.md

@@ -125,6 +125,33 @@ those provided by this module).
 The location span is a span created in addition to the request span.  See
 `datadog_trace_locations`.
 
+### `datadog_resource_name`
+
+- **syntax** `datadog_resource_name <variable_pattern>`
+- **default**: `$request_method $uri`, e.g. "GET /api/book/0-345-24223-8/title"
+- **context**: `http`, `server`, `location`, `if`
+
+Set the request span's "resource name" (sometimes called "endpoint") to the
+result of evaluating the specified `<variable_pattern>` in the context of the
+current request. `<variable_pattern>` is a string that may contain
+`$`-[variables][2] (including those provided by this module).
+
+The request span is the span created while processing a request.
+
+### `datadog_location_resource_name`
+
+- **syntax** `datadog_location_resource_name <variable_pattern>`
+- **default**: `$request_method $uri`, e.g. "GET /api/book/0-345-24223-8/title"
+- **context**: `http`, `server`, `location`, `if`
+
+Set the location span's "resource name" (sometimes called "endpoint") to the
+result of evaluating the specified `<variable_pattern>` in the context of the
+current request. `<variable_pattern>` is a string that may contain
+`$`-[variables][2] (including those provided by this module).
+
+The location span is a span created in addition to the request span.  See
+`datadog_trace_locations`.
+
 ### `datadog_trust_incoming_span`
 
 - **syntax** `datadog_trust_incoming_span on|off`

example/services/nginx/nginx.conf

@@ -32,6 +32,10 @@ http {
             datadog_trace_locations on;
             # This tag will be on the location-specific span.
             datadog_tag special.tag "The URI is $uri";
+            # The resource name is customizable for both the request span and
+            # the location span.
+            datadog_resource_name "request URI $uri";
+            datadog_location_resource_name "location URI $uri";
             proxy_pass http://http:8080;
         }
 

src/datadog_conf.h

@@ -78,6 +78,8 @@ struct datadog_loc_conf_t {
   ngx_flag_t enable_locations;
   NgxScript operation_name_script;
   NgxScript loc_operation_name_script;
+  NgxScript resource_name_script;
+  NgxScript loc_resource_name_script;
   ngx_flag_t trust_incoming_span;
   ngx_array_t *tags;
   // `response_info_script` is a script that can contain variables that refer

src/datadog_directive.cpp

@@ -487,6 +487,17 @@ char *set_datadog_location_operation_name(ngx_conf_t *cf, ngx_command_t *command
   return set_script(cf, command, loc_conf->loc_operation_name_script);
 }
 
+char *set_datadog_resource_name(ngx_conf_t *cf, ngx_command_t *command, void *conf) noexcept {
+  auto loc_conf = static_cast<datadog_loc_conf_t *>(conf);
+  return set_script(cf, command, loc_conf->resource_name_script);
+}
+
+char *set_datadog_location_resource_name(ngx_conf_t *cf, ngx_command_t *command,
+                                         void *conf) noexcept {
+  auto loc_conf = static_cast<datadog_loc_conf_t *>(conf);
+  return set_script(cf, command, loc_conf->loc_resource_name_script);
+}
+
 char *toggle_opentracing(ngx_conf_t *cf, ngx_command_t *command, void *conf) noexcept {
   const auto loc_conf = static_cast<datadog_loc_conf_t *>(conf);
   const auto values = static_cast<const ngx_str_t *>(cf->args->elts);

src/datadog_directive.h

@@ -38,6 +38,11 @@ char *set_datadog_operation_name(ngx_conf_t *cf, ngx_command_t *command, void *c
 char *set_datadog_location_operation_name(ngx_conf_t *cf, ngx_command_t *command,
                                           void *conf) noexcept;
 
+char *set_datadog_resource_name(ngx_conf_t *cf, ngx_command_t *command, void *conf) noexcept;
+
+char *set_datadog_location_resource_name(ngx_conf_t *cf, ngx_command_t *command,
+                                         void *conf) noexcept;
+
 char *toggle_opentracing(ngx_conf_t *cf, ngx_command_t *command, void *conf) noexcept;
 
 char *datadog_enable(ngx_conf_t *cf, ngx_command_t *command, void *conf) noexcept;

src/ngx_http_datadog_module.cpp

@@ -176,6 +176,20 @@ static ngx_command_t datadog_commands[] = {
       0,
       nullptr),
 
+    { ngx_string("datadog_resource_name"),
+      anywhere | NGX_CONF_TAKE1,
+      set_datadog_resource_name,
+      NGX_HTTP_LOC_CONF_OFFSET,
+      0,
+      nullptr},
+
+    { ngx_string("datadog_location_resource_name"),
+      anywhere | NGX_CONF_TAKE1,
+      set_datadog_location_resource_name,
+      NGX_HTTP_LOC_CONF_OFFSET,
+      0,
+      nullptr},
+
     DEFINE_COMMAND_WITH_OLD_ALIAS(
       "datadog_trust_incoming_span",
       "opentracing_trust_incoming_span",
@@ -295,10 +309,13 @@ static ngx_int_t datadog_module_init(ngx_conf_t *cf) noexcept {
   if (tags.empty()) return NGX_OK;
   main_conf->tags = ngx_array_create(cf->pool, tags.size(), sizeof(datadog_tag_t));
   if (!main_conf->tags) return NGX_ERROR;
-  for (const auto &tag : tags)
+  for (const auto &tag : tags) {
     if (add_datadog_tag(cf, main_conf->tags, to_ngx_str(tag.first), to_ngx_str(tag.second)) !=
-        NGX_CONF_OK)
+        NGX_CONF_OK) {
       return NGX_ERROR;
+    }
+  }
+
   return NGX_OK;
 }
 
@@ -398,13 +415,13 @@ static void *create_datadog_loc_conf(ngx_conf_t *conf) noexcept {
 
 namespace {
 
-// Merge the specified `previous` operation name script into the specified
-// `current` operation name script in the context of the specified `conf`.  If
-// `current` does not have a value and `previous` does, then `previous` will be
-// used.  If neither has a value, then a hard-coded default will be used.
-// Return `NGX_CONF_OK` on success, or another value otherwise.
-char *merge_operation_name_script(ngx_conf_t *conf, NgxScript &previous, NgxScript &current,
-                                  ot::string_view default_pattern) {
+// Merge the specified `previous` script into the specified `current` script in
+// the context of the specified `conf`.  If `current` does not have a value and
+// `previous` does, then `previous` will be used.  If neither has a value, then
+// the specified `default_pattern` will be used.  Return `NGX_CONF_OK` on
+// success, or another value otherwise.
+char *merge_script(ngx_conf_t *conf, NgxScript &previous, NgxScript &current,
+                   ot::string_view default_pattern) {
   if (current.is_valid()) {
     return NGX_CONF_OK;
   }
@@ -420,28 +437,6 @@ char *merge_operation_name_script(ngx_conf_t *conf, NgxScript &previous, NgxScri
   return NGX_CONF_OK;
 }
 
-char *merge_response_info_script(ngx_conf_t *conf, NgxScript &previous, NgxScript &current) {
-  // The response info script is the same for each `datadog_loc_conf_t`.  The only
-  // reason it's a member of `datadog_loc_conf_t` is so that it is available at
-  // the end of each request, when we might like to inspect e.g. response
-  // headers.
-  if (current.is_valid()) {
-    return NGX_CONF_OK;
-  }
-
-  if (!previous.is_valid()) {
-    // Response header inspection is not currently used by this module, but I'm
-    // leaving the boilerplate for future use.
-    const ngx_int_t rc = previous.compile(conf, ngx_string(""));
-    if (rc != NGX_OK) {
-      return (char *)NGX_CONF_ERROR;
-    }
-  }
-
-  current = previous;
-  return NGX_CONF_OK;
-}
-
 }  // namespace
 
 //------------------------------------------------------------------------------
@@ -455,19 +450,26 @@ static char *merge_datadog_loc_conf(ngx_conf_t *cf, void *parent, void *child) n
   ngx_conf_merge_value(conf->enable_locations, prev->enable_locations,
                        TracingLibrary::trace_locations_by_default());
 
+  if (const auto rc = merge_script(cf, prev->operation_name_script, conf->operation_name_script,
+                                   TracingLibrary::default_request_operation_name_pattern())) {
+    return rc;
+  }
   if (const auto rc =
-          merge_operation_name_script(cf, prev->operation_name_script, conf->operation_name_script,
-                                      TracingLibrary::default_request_operation_name_pattern())) {
+          merge_script(cf, prev->loc_operation_name_script, conf->loc_operation_name_script,
+                       TracingLibrary::default_location_operation_name_pattern())) {
     return rc;
   }
-  if (const auto rc = merge_operation_name_script(
-          cf, prev->loc_operation_name_script, conf->loc_operation_name_script,
-          TracingLibrary::default_location_operation_name_pattern())) {
+  if (const auto rc = merge_script(cf, prev->resource_name_script, conf->resource_name_script,
+                                   TracingLibrary::default_resource_name_pattern())) {
+    return rc;
+  }
+  if (const auto rc =
+          merge_script(cf, prev->loc_resource_name_script, conf->loc_resource_name_script,
+                       TracingLibrary::default_resource_name_pattern())) {
     return rc;
   }
-
   if (const auto rc =
-          merge_response_info_script(cf, prev->response_info_script, conf->response_info_script)) {
+          merge_script(cf, prev->response_info_script, conf->response_info_script, "")) {
     return rc;
   }
 
@@ -510,8 +512,8 @@ static char *merge_datadog_loc_conf(ngx_conf_t *cf, void *parent, void *child) n
 
         *tag = kv.second;
       } else {
-        datadog_tag_t *tag = (datadog_tag_t *)conf->tags->elts;
-        tag[index] = kv.second;
+        datadog_tag_t *tags = (datadog_tag_t *)conf->tags->elts;
+        tags[index] = kv.second;
       }
 
       index++;

src/request_tracing.cpp

@@ -33,6 +33,24 @@ static std::string get_request_operation_name(ngx_http_request_t *request,
     return to_string(core_loc_conf->name);
 }
 
+static std::string get_loc_resource_name(ngx_http_request_t *request,
+                                         const datadog_loc_conf_t *loc_conf) {
+  if (loc_conf->loc_resource_name_script.is_valid()) {
+    return to_string(loc_conf->loc_resource_name_script.run(request));
+  } else {
+    return "[invalid_resource_name_pattern]";
+  }
+}
+
+static std::string get_request_resource_name(ngx_http_request_t *request,
+                                             const datadog_loc_conf_t *loc_conf) {
+  if (loc_conf->resource_name_script.is_valid()) {
+    return to_string(loc_conf->resource_name_script.run(request));
+  } else {
+    return "[invalid_resource_name_pattern]";
+  }
+}
+
 static void add_script_tags(ngx_array_t *tags, ngx_http_request_t *request, ot::Span &span) {
   if (!tags) return;
   auto add_tag = [&](const datadog_tag_t &tag) {
@@ -153,6 +171,7 @@ void RequestTracing::on_exit_block(std::chrono::steady_clock::time_point finish_
     //
     // See on_log_request below
     span_->SetOperationName(get_loc_operation_name(request_, core_loc_conf_, loc_conf_));
+    span_->SetTag("resource.name", get_loc_resource_name(request_, loc_conf_));
 
     span_->Finish({ot::FinishTimestamp{finish_timestamp}});
   } else {
@@ -178,6 +197,7 @@ void RequestTracing::on_log_request() {
   auto core_loc_conf = static_cast<ngx_http_core_loc_conf_t *>(
       ngx_http_get_module_loc_conf(request_, ngx_http_core_module));
   request_span_->SetOperationName(get_request_operation_name(request_, core_loc_conf, loc_conf_));
+  request_span_->SetTag("resource.name", get_request_resource_name(request_, loc_conf_));
 
   // Note: At this point, we could run an `NginxScript` to interrogate the
   // proxied server's response headers, e.g. to retrieve a deferred sampling

src/tracing_library.cpp

@@ -198,10 +198,11 @@ std::unordered_map<string_view, string_view> TracingLibrary::default_tags() {
       // See
       // https://docs.datadoghq.com/logs/log_configuration/attributes_naming_convention/#common-attributes
       {"http.useragent", "$http_user_agent"},
-      {"resource.name", "$request_method $datadog_location"},
       {"nginx.location", "$datadog_location"}};
 }
 
+string_view TracingLibrary::default_resource_name_pattern() { return "$request_method $uri"; }
+
 bool TracingLibrary::tracing_on_by_default() { return true; }
 
 bool TracingLibrary::trace_locations_by_default() { return false; }

src/tracing_library.h

@@ -111,6 +111,13 @@ struct TracingLibrary {
   // (realistically this means that it will refer to a string literal).
   static string_view default_location_operation_name_pattern();
 
+  // Return the pattern of an nginx variable script that will be used for the
+  // resource name of spans that do not have a resource name configured in the
+  // nginx configuration.  Note that the storage to which the returned value
+  // refers must outlive any usage of the return value (realistically this
+  // means that it will refer to a string literal).
+  static string_view default_resource_name_pattern();
+
   // Return a mapping of tag name to nginx variable script pattern.  These
   // tags will be defined automatically during configuration as if they
   // appeared in the nginx configuration file's http section, e.g.