Better span sampler (#903)

* Better span sampler

This updates OpenTelemetry.Sampler so that it uses
`traces_sample_rate` for sampling spans before they
get sent to the span processor.

This way we're not processing spans when they are
dropped during sampling, which was previously the
case as the Client was responsible for sampling
just before attemping to send a transaction.

* Enhance sampling logic to record discarded transactions

* Clarify trace-level sampling decision-making

Previously it incorrectly referred to "parent" where
in reality we're dealing with spans from the same
trace and respect sampling decisions that were
already made for a given trace.

This is not the same thing as parent/child spans
that we're handling in the SpanProcessor as this
type of relationship is established post-sampling.

* Remove unnecessary sleep calls in sampler tests

* Fix flaky test

* Clean up make_sampling_decision

* Simplify client reports for dropped transactions

* Update traces_sample_rate default value to be nil

* Account for `nil` sample_rate when sampling

* Use put_test_config

* Update traces_sample_rate spec to allow nil value

Author: solnic

lib/sentry/client.ex

@@ -110,14 +110,12 @@ defmodule Sentry.Client do
   @spec send_transaction(Transaction.t(), keyword()) ::
           {:ok, transaction_id :: String.t()}
           | {:error, ClientError.t()}
-          | :unsampled
           | :excluded
   def send_transaction(%Transaction{} = transaction, opts \\ []) do
     opts = NimbleOptions.validate!(opts, Options.send_transaction_schema())
 
     result_type = Keyword.get_lazy(opts, :result, &Config.send_result/0)
     client = Keyword.get_lazy(opts, :client, &Config.client/0)
-    sample_rate = Keyword.get_lazy(opts, :sample_rate, &Config.traces_sample_rate/0)
     before_send = Keyword.get_lazy(opts, :before_send, &Config.before_send/0)
     after_send_event = Keyword.get_lazy(opts, :after_send_event, &Config.after_send_event/0)
 
@@ -126,16 +124,11 @@ defmodule Sentry.Client do
         Application.get_env(:sentry, :request_retries, Transport.default_retries())
       end)
 
-    with :ok <- sample_event(sample_rate),
-         {:ok, %Transaction{} = transaction} <- maybe_call_before_send(transaction, before_send) do
+    with {:ok, %Transaction{} = transaction} <- maybe_call_before_send(transaction, before_send) do
       send_result = encode_and_send(transaction, result_type, client, request_retries)
       _ignored = maybe_call_after_send(transaction, send_result, after_send_event)
       send_result
     else
-      :unsampled ->
-        ClientReport.Sender.record_discarded_events(:sample_rate, [transaction])
-        :unsampled
-
       :excluded ->
         :excluded
     end

lib/sentry/client_report/sender.ex

@@ -17,14 +17,21 @@ defmodule Sentry.ClientReport.Sender do
     GenServer.start_link(__MODULE__, nil, name: Keyword.get(opts, :name, __MODULE__))
   end
 
+  def record_discarded_events(reason, info, genserver \\ __MODULE__)
+
+  @spec record_discarded_events(atom(), String.t(), GenServer.server()) :: :ok
+  def record_discarded_events(reason, data_category, genserver)
+      when is_binary(data_category) do
+    GenServer.cast(genserver, {:record_discarded_events, reason, data_category})
+  end
+
   @spec record_discarded_events(atom(), [item], GenServer.server()) :: :ok
         when item:
                Sentry.Attachment.t()
                | Sentry.CheckIn.t()
                | ClientReport.t()
                | Sentry.Event.t()
-               | Sentry.Transaction.t()
-  def record_discarded_events(reason, event_items, genserver \\ __MODULE__)
+  def record_discarded_events(reason, event_items, genserver)
       when is_list(event_items) do
     # We silently ignore events whose reasons aren't valid because we have to add it to the allowlist in Snuba
     # https://develop.sentry.dev/sdk/client-reports/

lib/sentry/config.ex

@@ -145,12 +145,13 @@ defmodule Sentry.Config do
     ],
     traces_sample_rate: [
       type: {:custom, __MODULE__, :__validate_traces_sample_rate__, []},
-      default: 0.0,
+      default: nil,
       doc: """
       The sample rate for transaction events. A value between `0.0` and `1.0` (inclusive).
       A value of `0.0` means no transactions will be sampled, while `1.0` means all transactions
-      will be sampled. This value is also used to determine if tracing is enabled: if it's
-      greater than `0`, tracing is enabled.
+      will be sampled.
+
+      This value is also used to determine if tracing is enabled: if it's not `nil`, tracing is enabled.
 
       Tracing requires OpenTelemetry packages to work. See [the
       OpenTelemetry setup documentation](https://opentelemetry.io/docs/languages/erlang/getting-started/)
@@ -621,7 +622,7 @@ defmodule Sentry.Config do
   @spec sample_rate() :: float()
   def sample_rate, do: fetch!(:sample_rate)
 
-  @spec traces_sample_rate() :: float()
+  @spec traces_sample_rate() :: nil | float()
   def traces_sample_rate, do: fetch!(:traces_sample_rate)
 
   @spec hackney_opts() :: keyword()
@@ -662,7 +663,7 @@ defmodule Sentry.Config do
   def integrations, do: fetch!(:integrations)
 
   @spec tracing?() :: boolean()
-  def tracing?, do: fetch!(:traces_sample_rate) > 0.0
+  def tracing?, do: not is_nil(fetch!(:traces_sample_rate))
 
   @spec put_config(atom(), term()) :: :ok
   def put_config(key, value) when is_atom(key) do
@@ -763,12 +764,12 @@ defmodule Sentry.Config do
     end
   end
 
-  def __validate_traces_sample_rate__(float) do
-    if is_float(float) and float >= 0.0 and float <= 1.0 do
-      {:ok, float}
+  def __validate_traces_sample_rate__(value) do
+    if is_nil(value) or (is_float(value) and value >= 0.0 and value <= 1.0) do
+      {:ok, value}
     else
       {:error,
-       "expected :traces_sample_rate to be a float between 0.0 and 1.0 (included), got: #{inspect(float)}"}
+       "expected :traces_sample_rate to be nil or a value between 0.0 and 1.0 (included), got: #{inspect(value)}"}
     end
   end
 

lib/sentry/opentelemetry/sampler.ex

@@ -2,30 +2,127 @@ if Code.ensure_loaded?(:otel_sampler) do
   defmodule Sentry.OpenTelemetry.Sampler do
     @moduledoc false
 
+    alias OpenTelemetry.{Span, Tracer}
+    alias Sentry.ClientReport
+
     @behaviour :otel_sampler
 
+    @sentry_sample_rate_key "sentry-sample_rate"
+    @sentry_sample_rand_key "sentry-sample_rand"
+    @sentry_sampled_key "sentry-sampled"
+
+    @impl true
     def setup(config) do
       config
     end
 
+    @impl true
     def description(_) do
       "SentrySampler"
     end
 
+    @impl true
     def should_sample(
-          _ctx,
+          ctx,
           _trace_id,
           _links,
           span_name,
           _span_kind,
           _attributes,
           config
         ) do
-      if span_name in config[:drop] do
-        {:drop, [], []}
-      else
-        {:record_and_sample, [], []}
+      result =
+        if span_name in config[:drop] do
+          {:drop, [], []}
+        else
+          sample_rate = Sentry.Config.traces_sample_rate()
+
+          case get_trace_sampling_decision(ctx) do
+            {:inherit, trace_sampled, tracestate} ->
+              decision = if trace_sampled, do: :record_and_sample, else: :drop
+
+              {decision, [], tracestate}
+
+            :no_trace ->
+              make_sampling_decision(sample_rate)
+          end
+        end
+
+      case result do
+        {:drop, _, _} ->
+          record_discarded_transaction()
+          result
+
+        _ ->
+          result
       end
     end
+
+    defp get_trace_sampling_decision(ctx) do
+      case Tracer.current_span_ctx(ctx) do
+        :undefined ->
+          :no_trace
+
+        span_ctx ->
+          tracestate = Span.tracestate(span_ctx)
+          trace_sampled = get_tracestate_value(tracestate, @sentry_sampled_key)
+
+          case trace_sampled do
+            "true" ->
+              {:inherit, true, tracestate}
+
+            "false" ->
+              {:inherit, false, tracestate}
+
+            nil ->
+              :no_trace
+          end
+      end
+    end
+
+    defp make_sampling_decision(sample_rate) do
+      cond do
+        is_nil(sample_rate) ->
+          {:drop, [], []}
+
+        sample_rate == 0.0 ->
+          tracestate = build_tracestate(sample_rate, 1.0, false)
+          {:drop, [], tracestate}
+
+        sample_rate == 1.0 ->
+          tracestate = build_tracestate(sample_rate, 0.0, true)
+          {:record_and_sample, [], tracestate}
+
+        true ->
+          random_value = :rand.uniform()
+          sampled = random_value < sample_rate
+          tracestate = build_tracestate(sample_rate, random_value, sampled)
+          decision = if sampled, do: :record_and_sample, else: :drop
+          {decision, [], tracestate}
+      end
+    end
+
+    defp build_tracestate(sample_rate, random_value, sampled) do
+      [
+        {@sentry_sample_rate_key, Float.to_string(sample_rate)},
+        {@sentry_sample_rand_key, Float.to_string(random_value)},
+        {@sentry_sampled_key, to_string(sampled)}
+      ]
+    end
+
+    defp get_tracestate_value({:tracestate, tracestate}, key) do
+      get_tracestate_value(tracestate, key)
+    end
+
+    defp get_tracestate_value(tracestate, key) when is_list(tracestate) do
+      case List.keyfind(tracestate, key, 0) do
+        {^key, value} -> value
+        nil -> nil
+      end
+    end
+
+    defp record_discarded_transaction() do
+      ClientReport.Sender.record_discarded_events(:sample_rate, "transaction")
+    end
   end
 end

lib/sentry/opentelemetry/span_processor.ex

@@ -39,6 +39,9 @@ if Code.ensure_loaded?(OpenTelemetry) do
             :ignored ->
               true
 
+            :excluded ->
+              true
+
             {:error, error} ->
               Logger.warning("Failed to send transaction to Sentry: #{inspect(error)}")
               {:error, :invalid_span}

test/sentry/config_test.exs

@@ -154,8 +154,12 @@ defmodule Sentry.ConfigTest do
     end
 
     test ":traces_sample_rate" do
+      assert Config.validate!([])[:traces_sample_rate] == nil
+
+      assert Config.validate!(traces_sample_rate: nil)[:traces_sample_rate] == nil
+      assert Config.validate!(traces_sample_rate: 0.0)[:traces_sample_rate] == 0.0
+      assert Config.validate!(traces_sample_rate: 0.5)[:traces_sample_rate] == 0.5
       assert Config.validate!(traces_sample_rate: 1.0)[:traces_sample_rate] == 1.0
-      assert Config.validate!([])[:traces_sample_rate] == 0.0
 
       assert_raise ArgumentError, ~r/invalid value for :traces_sample_rate option/, fn ->
         Config.validate!(traces_sample_rate: 2.0)