From 19a44fbd6830ae71ab161028d67ce6ae40091766 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Romario=20L=C3=B3pez?= <romariolopezc@gmail.com>
Date: Thu, 20 Apr 2023 05:04:18 -0600
Subject: [PATCH] Improve Req with OTel http spec (#163)

* Improve span_name

Use span_name if provided. Fallback to url.path if there is no
path_params.

* Add unreleased changelog

* Update examples and module description

* Change span_name and http.url to follow OTEL spec

* Remove unused function

* Improve changelog

* Fix reading span_name from request.options

* Don't use URI.path

* address changelog
---
 .../opentelemetry_req/CHANGELOG.md            |  9 +++
 .../lib/opentelemetry_req.ex                  | 64 +++++++++++++------
 2 files changed, 53 insertions(+), 20 deletions(-)

diff --git a/instrumentation/opentelemetry_req/CHANGELOG.md b/instrumentation/opentelemetry_req/CHANGELOG.md
index 775badb..e7694b4 100644
--- a/instrumentation/opentelemetry_req/CHANGELOG.md
+++ b/instrumentation/opentelemetry_req/CHANGELOG.md
@@ -1,5 +1,14 @@
 # Changelog
 
+## Unreleased
+
+* Change span_name to follow [OpenTelemetry http spec](https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/http/#name)
+* Use span_name if provided, otherwise use path_params. If there are no path_params,
+  default to http.method
+* Change http.url to follow [OpenTelemetry http spec](https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/http/#http-client).
+  Full HTTP request URL in the form scheme://host[:port]/path?query[#fragment].
+  Must not contain credentials passed via URL.
+
 ## 0.1.2
 
 ### Fixes
diff --git a/instrumentation/opentelemetry_req/lib/opentelemetry_req.ex b/instrumentation/opentelemetry_req/lib/opentelemetry_req.ex
index a0675e6..6f3aeff 100644
--- a/instrumentation/opentelemetry_req/lib/opentelemetry_req.ex
+++ b/instrumentation/opentelemetry_req/lib/opentelemetry_req.ex
@@ -5,17 +5,20 @@ defmodule OpentelemetryReq do
   expected by default and an error will be raised if the path params option is
   not set for the request.
 
-  Given the steps pipeline can be halted to skip further steps from running, it is important
-  to _append_ request and response steps _after_ this step to ensure execution. Spans are not
-  created until the request is completed or errored.
+  Spans are not created until the request is completed or errored.
 
-  ### Example
+  ## Request Options
+
+    * `:span_name` - `String.t()` if provided, overrides the span name. Defaults to `nil`.
+    * `:no_path_params` - `boolean()` when set to `true` no path params are expected for the request. Defaults to `false`
+    * `:propagate_trace_ctx` - `boolean()` when set to `true`, trace headers will be propagated. Defaults to `false`
+
+  ### Example with path_params
 
   ```
   client =
     Req.new()
-    |> OpentelemetryReq.attach()
-    |> Req.Request.merge_options(
+    |> OpentelemetryReq.attach(
       base_url: "http://localhost:4000",
       propagate_trace_ctx: true
     )
@@ -26,11 +29,24 @@ defmodule OpentelemetryReq do
     path_params: [user_id: user_id]
   )
   ```
-  ## Request Options
 
-    * `:span_name` - `String.t()` if provided, overrides the span name. Defaults to `nil`.
-    * `:no_path_params` - `boolean()` when set to `true` no path params are expected for the request. Defaults to `false`
-    * `:propagate_trace_ctx` - `boolean()` when set to `true`, trace headers will be propagated. Defaults to `false`
+  ### Example without path_params
+
+  ```
+  client =
+    Req.new()
+    |> OpentelemetryReq.attach(
+      base_url: "http://localhost:4000",
+      propagate_trace_ctx: true,
+      no_path_params: true
+    )
+
+  client
+  |> Req.get(
+    url: "/api/users"
+  )
+  ```
+  If you don't set `path_params` the request will raise.
   """
 
   alias OpenTelemetry.Tracer
@@ -106,12 +122,23 @@ defmodule OpentelemetryReq do
   defp format_exception(_), do: ""
 
   defp span_name(request) do
-    Req.Request.get_private(request, :path_params_template)
+    case request.options[:span_name] do
+      nil ->
+        method = http_method(request.method)
+
+        case Req.Request.get_private(request, :path_params_template) do
+          nil -> method
+          params_template -> "#{method} #{params_template}"
+        end
+
+      span_name ->
+        span_name
+    end
   end
 
   defp build_req_attrs(request) do
     uri = request.url
-    url = url(uri)
+    url = sanitize_url(uri)
 
     %{
       Trace.http_method() => http_method(request.method),
@@ -124,6 +151,11 @@ defmodule OpentelemetryReq do
     |> maybe_append_retry_count(request)
   end
 
+  defp sanitize_url(uri) do
+    %{uri | userinfo: nil}
+    |> URI.to_string()
+  end
+
   defp maybe_append_req_content_length(attrs, req) do
     case Req.Request.get_header(req, "content-length") do
       [] ->
@@ -154,14 +186,6 @@ defmodule OpentelemetryReq do
     end
   end
 
-  defp url(uri) do
-    if uri.query do
-      uri.path <> "?" <> uri.query
-    else
-      uri.path
-    end
-  end
-
   defp http_method(method) do
     case method do
       :get -> :GET