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

…62)

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

README.md

@@ -103,24 +103,24 @@ defmodule ManuallyTraced do
 
   # Does not handle exceptions for you.
   def trace_me() do
-    _ = Tracer.start_trace("my_trace") #also opens a span
-    _ = Tracer.update_span(service: :my_app, type: :db)
+    Tracer.start_trace("my_trace") #also opens a span
+    Tracer.update_span(service: :my_app, type: :db)
 
     result = span_me()
 
-    _ = Tracer.finish_trace()
+    Tracer.finish_trace()
 
     result
   end
 
   # Does not handle exceptions for you.
   def span_me() do
-    _ = Tracer.start_span("this_span")
-    _ = Tracer.update_span(service: :my_app, type: :web)
+    Tracer.start_span("this_span")
+    Tracer.update_span(service: :my_app, type: :web)
 
     result = span_me_also()
 
-    _ = Tracer.finish_span()
+    Tracer.finish_span()
   end
 
   # Handles exception at the span level. Trace still must be reported.

lib/spandex.ex

@@ -26,7 +26,7 @@ defmodule Spandex do
   def start_trace(name, opts) do
     strategy = opts[:strategy]
 
-    if strategy.get_trace(opts[:tracer]) do
+    if strategy.trace_active?(opts[:trace_key]) do
       Logger.error("Tried to start a trace over top of another trace.")
       {:error, :trace_running}
     else
@@ -44,12 +44,15 @@ defmodule Spandex do
   def start_span(name, opts) do
     strategy = opts[:strategy]
 
-    case strategy.get_trace(opts[:tracer]) do
-      nil ->
+    case strategy.get_trace(opts[:trace_key]) do
+      {:error, :no_trace_context} = error ->
         Logger.error("Tried to start a span without an active trace.")
-        {:error, :no_trace_context}
+        error
 
-      trace ->
+      {:error, _} = error ->
+        error
+
+      {:ok, trace} ->
         do_start_span(name, trace, opts)
     end
   end
@@ -66,17 +69,20 @@ defmodule Spandex do
   def update_span(opts, top?) do
     strategy = opts[:strategy]
 
-    case strategy.get_trace(opts[:tracer]) do
-      nil ->
+    case strategy.get_trace(opts[:trace_key]) do
+      {:error, :no_trace_context} = error ->
         Logger.error("Tried to update a span without an active trace.")
-        {:error, :no_trace_context}
+        error
 
-      %Trace{stack: []} ->
+      {:ok, %Trace{stack: []}} ->
         Logger.error("Tried to update a span without an active span.")
         {:error, :no_span_context}
 
-      trace ->
+      {:ok, trace} ->
         do_update_span(trace, opts, top?)
+
+      {:error, _} = error ->
+        error
     end
   end
 
@@ -99,15 +105,18 @@ defmodule Spandex do
   def update_all_spans(opts) do
     strategy = opts[:strategy]
 
-    case strategy.get_trace(opts[:tracer]) do
-      nil ->
+    case strategy.get_trace(opts[:trace_key]) do
+      {:error, :no_trace_context} = error ->
         Logger.error("Tried to update a span without an active trace.")
-        {:error, :no_trace_context}
+        error
 
-      %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})
+        strategy.put_trace(opts[:trace_key], %{trace | stack: new_stack, spans: new_spans})
+
+      {:error, _} = error ->
+        error
     end
   end
 
@@ -122,16 +131,19 @@ defmodule Spandex do
     strategy = opts[:strategy]
     adapter = opts[:adapter]
 
-    case strategy.get_trace(opts[:tracer]) do
-      nil ->
+    case strategy.get_trace(opts[:trace_key]) do
+      {:error, :no_trace_context} = error ->
         Logger.error("Tried to finish a trace without an active trace.")
-        {:error, :no_trace_context}
+        error
 
-      %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()
         sender.send_spans(spans ++ unfinished_spans)
-        strategy.delete_trace(opts[:tracer])
+        strategy.delete_trace(opts[:trace_key])
+
+      {:error, _} = error ->
+        error
     end
   end
 
@@ -147,19 +159,28 @@ defmodule Spandex do
     strategy = opts[:strategy]
     adapter = opts[:adapter]
 
-    case strategy.get_trace(opts[:tracer]) do
-      nil ->
+    case strategy.get_trace(opts[:trace_key]) do
+      {:error, :no_trace_context} = error ->
         Logger.error("Tried to finish a span without an active trace.")
-        {:error, :no_trace_context}
+        error
 
-      %Trace{stack: []} ->
+      {:ok, %Trace{stack: []}} ->
         Logger.error("Tried to finish a span without an active span.")
         {: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[:trace_key], %{
+          trace
+          | stack: tail,
+            spans: [finished_span | spans]
+        })
+
         {:ok, finished_span}
+
+      {:error, _} = error ->
+        error
     end
   end
 
@@ -182,12 +203,14 @@ defmodule Spandex do
   def current_trace_id(opts) do
     strategy = opts[:strategy]
 
-    case strategy.get_trace(opts[:tracer]) do
-      nil ->
-        nil
-
-      %Trace{id: id} ->
+    case strategy.get_trace(opts[:trace_key]) do
+      {: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,10 +230,17 @@ defmodule Spandex do
   def current_span(opts) do
     strategy = opts[:strategy]
 
-    case strategy.get_trace(opts[:tracer]) do
-      nil -> nil
-      %Trace{stack: []} -> nil
-      %Trace{stack: [span | _]} -> span
+    case strategy.get_trace(opts[:trace_key]) do
+      {: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
 
@@ -225,7 +255,7 @@ defmodule Spandex do
     strategy = opts[:strategy]
     opts = Keyword.put(opts, :parent_id, span_id)
 
-    if strategy.get_trace(opts[:tracer]) do
+    if strategy.trace_active?(opts[:trace_key]) do
       Logger.error("Tried to continue a trace over top of another trace.")
       {:error, :trace_already_present}
     else
@@ -243,7 +273,7 @@ defmodule Spandex do
   def continue_trace_from_span(name, span, opts) do
     strategy = opts[:strategy]
 
-    if strategy.get_trace(opts[:tracer]) do
+    if strategy.trace_active?(opts[:trace_key]) do
       Logger.error("Tried to continue a trace over top of another trace.")
       {:error, :trace_already_present}
     else
@@ -270,7 +300,7 @@ defmodule Spandex do
 
     with {:ok, top_span} <- span(name, opts, trace_id, adapter) do
       trace = %Trace{id: trace_id, stack: [top_span], spans: []}
-      strategy.put_trace(opts[:tracer], trace)
+      strategy.put_trace(opts[:trace_key], trace)
     end
   end
 
@@ -280,7 +310,7 @@ defmodule Spandex do
 
     with {:ok, span} <- Span.child_of(span, name, adapter.span_id(), adapter.now(), opts) do
       trace = %Trace{id: adapter.trace_id(), stack: [span], spans: []}
-      strategy.put_trace(opts[:tracer], trace)
+      strategy.put_trace(opts[:trace_key], trace)
     end
   end
 
@@ -289,7 +319,7 @@ defmodule Spandex do
     adapter = opts[:adapter]
 
     with {:ok, span} <- Span.child_of(current_span, name, adapter.span_id(), adapter.now(), opts),
-         {:ok, _trace} <- strategy.put_trace(opts[:tracer], %{trace | stack: [span | trace.stack]}) do
+         {:ok, _trace} <- strategy.put_trace(opts[:trace_key], %{trace | stack: [span | trace.stack]}) do
       Logger.metadata(span_id: span.id)
       {:ok, span}
     end
@@ -300,7 +330,7 @@ defmodule Spandex do
     adapter = opts[:adapter]
 
     with {:ok, span} <- span(name, opts, trace_id, adapter),
-         {:ok, _trace} <- strategy.put_trace(opts[:tracer], %{trace | stack: [span]}) do
+         {:ok, _trace} <- strategy.put_trace(opts[:trace_key], %{trace | stack: [span]}) do
       Logger.metadata(span_id: span.id)
       {:ok, span}
     end
@@ -314,15 +344,15 @@ defmodule Spandex do
     with {:ok, span} <- span(name, opts, trace_id, adapter) do
       Logger.metadata(trace_id: trace_id, span_id: span.id)
       trace = %Trace{spans: [], stack: [span], id: trace_id}
-      strategy.put_trace(opts[:tracer], trace)
+      strategy.put_trace(opts[:trace_key], trace)
     end
   end
 
   defp do_update_span(%Trace{stack: stack} = trace, opts, true) do
     strategy = opts[:strategy]
     new_stack = List.update_at(stack, -1, &update_or_keep(&1, opts))
 
-    with {:ok, _trace} <- strategy.put_trace(opts[:tracer], %{trace | stack: new_stack}) do
+    with {:ok, _trace} <- strategy.put_trace(opts[:trace_key], %{trace | stack: new_stack}) do
       {:ok, Enum.at(new_stack, -1)}
     end
   end
@@ -332,7 +362,7 @@ defmodule Spandex do
     updated_span = update_or_keep(current_span, opts)
     new_stack = [updated_span | other_spans]
 
-    with {:ok, _trace} <- strategy.put_trace(opts[:tracer], %{trace | stack: new_stack}) do
+    with {:ok, _trace} <- strategy.put_trace(opts[:trace_key], %{trace | stack: new_stack}) do
       {:ok, updated_span}
     end
   end

lib/strategy.ex

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

lib/strategy/pdict.ex

@@ -7,21 +7,30 @@ defmodule Spandex.Strategy.Pdict do
   @behaviour Spandex.Strategy
 
   @impl Spandex.Strategy
-  def get_trace(tracer) do
-    Process.get({:spandex_trace, tracer})
+  def trace_active?(trace_key) do
+    Process.get({:spandex_trace, trace_key})
   end
 
   @impl Spandex.Strategy
-  def put_trace(tracer, trace) do
-    Process.put({:spandex_trace, tracer}, trace)
+  def get_trace(trace_key) do
+    trace = Process.get({:spandex_trace, trace_key})
 
-    {:ok, trace}
+    if trace do
+      {:ok, trace}
+    else
+      {:error, :no_trace_context}
+    end
   end
 
   @impl Spandex.Strategy
-  def delete_trace(tracer) do
-    Process.delete({:spandex_trace, tracer})
+  def put_trace(trace_key, trace) do
+    Process.put({:spandex_trace, trace_key}, trace)
 
-    :ok
+    {:ok, trace}
+  end
+
+  @impl Spandex.Strategy
+  def delete_trace(trace_key) do
+    {:ok, Process.delete({:spandex_trace, trace_key})}
   end
 end