standardize return types from strategies (and by proxy from Spandex)

lib/spandex.ex

@@ -12,26 +12,31 @@ defmodule Spandex do
   def start_trace(name, opts) do
     strategy = opts[:strategy]
 
-    if strategy.get_trace(opts[:tracer]) do
-      _ = Logger.error("Tried to start a trace over top of another trace.")
-      {:error, :trace_running}
-    else
-      adapter = opts[:adapter]
-      trace_id = adapter.trace_id()
-
-      name
-      |> span(opts, trace_id, adapter)
-      |> with_span(fn span ->
-        Logger.metadata(trace_id: trace_id, span_id: span.id)
-
-        trace = %Trace{
-          spans: [],
-          stack: [span],
-          id: trace_id
-        }
-
-        strategy.put_trace(opts[:tracer], trace)
-      end)
+    case strategy.get_trace(opts[:tracer]) do
+      {:ok, nil} ->
+        adapter = opts[:adapter]
+        trace_id = adapter.trace_id()
+
+        name
+        |> span(opts, trace_id, adapter)
+        |> with_span(fn span ->
+          Logger.metadata(trace_id: trace_id, span_id: span.id)
+
+          trace = %Trace{
+            spans: [],
+            stack: [span],
+            id: trace_id
+          }
+
+          strategy.put_trace(opts[:tracer], trace)
+        end)
+
+      {:ok, _trace} ->
+        _ = Logger.error("Tried to start a trace over top of another trace.")
+        {:error, :trace_running}
+
+      {:error, _} = error ->
+        error
     end
   end
 
@@ -42,25 +47,28 @@ defmodule Spandex do
     adapter = opts[:adapter]
 
     case strategy.get_trace(opts[:tracer]) do
-      nil ->
+      {:error, _} = error ->
+        error
+
+      {:ok, nil} ->
         {:error, :no_trace_context}
 
-      %Trace{stack: [current_span | _]} = trace ->
+      {:ok, %Trace{stack: [current_span | _]} = trace} ->
         current_span
         |> Span.child_of(name, adapter.span_id(), adapter.now(), opts)
         |> with_span(fn span ->
-          strategy.put_trace(opts[:tracer], %{trace | stack: [span | trace.stack]})
+          _ = strategy.put_trace(opts[:tracer], %{trace | stack: [span | trace.stack]})
 
           Logger.metadata(span_id: span.id)
 
           {:ok, span}
         end)
 
-      %Trace{stack: [], id: trace_id} = trace ->
+      {:ok, %Trace{stack: [], id: trace_id} = trace} ->
         name
         |> span(opts, trace_id, adapter)
         |> with_span(fn span ->
-          strategy.put_trace(opts[:tracer], %{trace | stack: [span]})
+          _ = strategy.put_trace(opts[:tracer], %{trace | stack: [span]})
 
           Logger.metadata(span_id: span.id)
 
@@ -76,10 +84,10 @@ defmodule Spandex do
     strategy = opts[:strategy]
 
     case strategy.get_trace(opts[:tracer]) do
-      nil ->
+      {:ok, nil} ->
         {:error, :no_trace_context}
 
-      %Trace{stack: stack} = trace ->
+      {:ok, %Trace{stack: stack} = trace} ->
         index =
           if top? do
             -1
@@ -89,9 +97,12 @@ defmodule Spandex do
 
         new_stack = List.update_at(stack, index, &update_or_keep(&1, opts))
 
-        strategy.put_trace(opts[:tracer], %{trace | stack: new_stack})
+        _ = strategy.put_trace(opts[:tracer], %{trace | stack: new_stack})
 
         {:ok, Enum.at(new_stack, index)}
+
+      {:error, _} = error ->
+        error
     end
   end
 
@@ -108,15 +119,18 @@ defmodule Spandex do
     strategy = opts[:strategy]
 
     case strategy.get_trace(opts[:tracer]) do
-      nil ->
+      {:ok, nil} ->
         {:error, :no_trace_context}
 
-      %Trace{stack: stack, spans: spans} = trace ->
+      {:ok, %Trace{stack: stack, spans: spans} = trace} ->
         new_stack = Enum.map(stack, &update_or_keep(&1, opts))
 
         new_spans = Enum.map(spans, &update_or_keep(&1, opts))
 
         strategy.put_trace(opts[:tracer], %{trace | stack: new_stack, spans: new_spans})
+
+      {:error, _} = error ->
+        error
     end
   end
 
@@ -127,10 +141,10 @@ defmodule Spandex do
     adapter = opts[:adapter]
 
     case strategy.get_trace(opts[:tracer]) do
-      nil ->
+      {:ok, nil} ->
         {:error, :no_trace_context}
 
-      %Trace{spans: spans, stack: stack} ->
+      {:ok, %Trace{spans: spans, stack: stack}} ->
         unfinished_spans = Enum.map(stack, &ensure_completion_time_set(&1, adapter))
 
         sender = opts[:sender] || adapter.default_sender()
@@ -140,6 +154,9 @@ defmodule Spandex do
         |> sender.send_spans()
 
         strategy.delete_trace(opts[:tracer])
+
+      {:error, _} = error ->
+        error
     end
   end
 
@@ -150,17 +167,22 @@ defmodule Spandex do
     adapter = opts[:adapter]
 
     case strategy.get_trace(opts[:tracer]) do
-      nil ->
+      {:ok, nil} ->
         {:error, :no_trace_context}
 
-      %Trace{stack: []} ->
+      {:ok, %Trace{stack: []}} ->
         {:error, :no_span_context}
 
-      %Trace{stack: [span | tail], spans: spans} = trace ->
+      {:ok, %Trace{stack: [span | tail], spans: spans} = trace} ->
         finished_span = ensure_completion_time_set(span, adapter)
 
-        strategy.put_trace(opts[:tracer], %{trace | stack: tail, spans: [finished_span | spans]})
+        _ =
+          strategy.put_trace(opts[:tracer], %{trace | stack: tail, spans: [finished_span | spans]})
+
         {:ok, finished_span}
+
+      {:error, _} = error ->
+        error
     end
   end
 
@@ -184,11 +206,16 @@ defmodule Spandex do
     strategy = opts[:strategy]
 
     case strategy.get_trace(opts[:tracer]) do
-      nil ->
+      {:ok, nil} ->
         nil
 
-      %Trace{id: id} ->
+      {:ok, %Trace{id: id}} ->
         id
+
+      {:error, _} ->
+        # TODO: Alter the return type of this interface to allow for returning
+        # errors from fetching the trace.
+        nil
     end
   end
 
@@ -207,9 +234,19 @@ defmodule Spandex do
     strategy = opts[:strategy]
 
     case strategy.get_trace(opts[:tracer]) do
-      nil -> nil
-      %Trace{stack: []} -> nil
-      %Trace{stack: [span | _]} -> span
+      {:ok, nil} ->
+        nil
+
+      {:ok, %Trace{stack: []}} ->
+        nil
+
+      {:ok, %Trace{stack: [span | _]}} ->
+        span
+
+      {:error, _} ->
+        # TODO: Alter the return type of this interface to allow for returning
+        # errors from fetching the trace.
+        nil
     end
   end
 
@@ -220,14 +257,17 @@ defmodule Spandex do
     adapter = opts[:adapter]
 
     case strategy.get_trace(opts[:tracer]) do
-      nil ->
+      {:ok, nil} ->
         opts_with_parent = Keyword.put(opts, :parent_id, span_id)
         top_span = span(name, opts_with_parent, trace_id, adapter)
 
         strategy.put_trace(opts[:tracer], %Trace{id: trace_id, stack: [top_span], spans: []})
 
-      _ ->
+      {:ok, _} ->
         {:error, :trace_already_present}
+
+      {:error, _} = error ->
+        error
     end
   end
 
@@ -238,7 +278,7 @@ defmodule Spandex do
     adapter = opts[:adapter]
 
     case strategy.get_trace(opts[:tracer]) do
-      nil ->
+      {:ok, nil} ->
         span
         |> Span.child_of(name, adapter.span_id(), adapter.now(), opts)
         |> with_span(fn span ->
@@ -249,8 +289,11 @@ defmodule Spandex do
           })
         end)
 
-      _ ->
+      {:ok, _} ->
         {:error, :trace_already_present}
+
+      {:error, _} = error ->
+        error
     end
   end
 

lib/strategy.ex

@@ -6,7 +6,7 @@ defmodule Spandex.Strategy do
 
   alias Spandex.Trace
 
-  @callback get_trace(tracer) :: Trace.t() | nil
+  @callback get_trace(tracer) :: {:ok, Trace.t() | nil} | {:error, term}
   @callback put_trace(tracer, Trace.t()) :: {:ok, Trace.t()} | {:error, term}
-  @callback delete_trace(tracer) :: :ok | {:error, term}
+  @callback delete_trace(tracer) :: {:ok, Trace.t()} | {:error, term}
 end

lib/strategy/pdict.ex

@@ -8,7 +8,7 @@ defmodule Spandex.Strategy.Pdict do
 
   @impl Spandex.Strategy
   def get_trace(tracer) do
-    Process.get({:spandex_trace, tracer})
+    {:ok, Process.get({:spandex_trace, tracer})}
   end
 
   @impl Spandex.Strategy
@@ -20,8 +20,6 @@ defmodule Spandex.Strategy.Pdict do
 
   @impl Spandex.Strategy
   def delete_trace(tracer) do
-    Process.delete({:spandex_trace, tracer})
-
-    :ok
+    {:ok, Process.delete({:spandex_trace, tracer})}
   end
 end

test/plug/add_context_test.exs

@@ -28,7 +28,7 @@ defmodule Spandex.Plug.AddContextTest do
           tracer_opts: []
         )
 
-      :ok = Tracer.finish_trace()
+      {:ok, _} = Tracer.finish_trace()
 
       %{resource: resource, http: http} = Spandex.Test.Util.find_span("request")
 
@@ -53,7 +53,7 @@ defmodule Spandex.Plug.AddContextTest do
       assert Keyword.fetch!(Logger.metadata(), :trace_id) == tid
       assert Keyword.fetch!(Logger.metadata(), :span_id) == expected_span.id
 
-      :ok = Tracer.finish_trace()
+      {:ok, _} = Tracer.finish_trace()
 
       %{trace_id: trace_id, type: type, http: http, resource: resource} =
         Spandex.Test.Util.find_span("request")

test/plug/end_trace_test.exs

@@ -23,7 +23,7 @@ defmodule Spandex.Plug.EndTraceTest do
 
       assert Tracer.current_trace_id() == tid
 
-      :ok = Tracer.finish_trace()
+      {:ok, _} = Tracer.finish_trace()
     end
 
     test "updates top span and finish span, when we trace request for 200", %{