Repository: newrelic/elixir_agent (elixir)

Issue:
Sample processing can fail when sampling data
Hello! 👋 

**Describe the bug**

We got two New Relic errors with message `(ArithmeticError) bad argument in arithmetic expression`. The stack trace pointed to https://github.com/newrelic/elixir_agent/blob/master/lib/new_relic/sampler/process.ex#L79

It looks like the agent does not handle a `nil` return case when fetching info about the process if the process is no longer alive. According to erlang docs, "Returns undefined [nil in Process.info/2] if the process is not alive."

I haven't found details on why the process wasn't alive. Both error occurrences were a single error on different days.

**Environment**

* Elixir & Erlang version: 1.12.1-otp-24
* Agent version: 1.27.4


Pull request:
Safer math in Process sampler
Protect against race conditions in Process Sampler math. In case a process dies while we are sampling it.

Closes #367 

Patch:
diff --git a/.github/workflows/elixir.yml b/.github/workflows/elixir.yml
index b8c996c..1ff868d 100644
--- a/.github/workflows/elixir.yml
+++ b/.github/workflows/elixir.yml
@@ -17,18 +17,16 @@ jobs:
     strategy:
       matrix:
         include:
-          - elixir: '1.15'
+          - elixir: '1.18'
+            otp: '27'
+          - elixir: '1.17'
+            otp: '27'
+          - elixir: '1.16'
             otp: '26'
           - elixir: '1.15'
-            otp: '25'
-          - elixir: '1.14'
             otp: '26'
-          - elixir: '1.13'
+          - elixir: '1.14'
             otp: '25'
-          - elixir: '1.12'
-            otp: '24'
-          - elixir: '1.11'
-            otp: '23'
 
     steps:
     - name: Checkout
@@ -84,14 +82,14 @@ jobs:
     strategy:
       matrix:
         include:
-          - elixir: '1.15'
+          - elixir: '1.18'
+            otp: '27'
+          - elixir: '1.17'
+            otp: '27'
+          - elixir: '1.16'
             otp: '26'
-          - elixir: '1.14'
+          - elixir: '1.15'
             otp: '26'
-          - elixir: '1.13'
-            otp: '25'
-          - elixir: '1.12'
-            otp: '24'
 
     defaults:
       run:
diff --git a/lib/new_relic/config.ex b/lib/new_relic/config.ex
index 1819619..807fe7d 100644
--- a/lib/new_relic/config.ex
+++ b/lib/new_relic/config.ex
@@ -115,6 +115,8 @@ defmodule NewRelic.Config do
 
   * `:plug_instrumentation_enabled` (default `true`)
     * Controls all Plug instrumentation
+  * `:distributed_tracing` (default `true`)
+    * Controls Distributed Tracking
   * `:ecto_instrumentation_enabled` (default `true`)
     * Controls all Ecto instrumentation
   * `:redix_instrumentation_enabled` (default `true`)
@@ -139,6 +141,10 @@ defmodule NewRelic.Config do
     get(:features, :db_query_collection)
   end
 
+  def feature?(:distributed_tracing) do
+    get(:features, :distributed_tracing)
+  end
+
   def feature?(:plug_instrumentation) do
     get(:features, :plug_instrumentation)
   end
diff --git a/lib/new_relic/distributed_trace.ex b/lib/new_relic/distributed_trace.ex
index a80f91a..8a2cb0b 100644
--- a/lib/new_relic/distributed_trace.ex
+++ b/lib/new_relic/distributed_trace.ex
@@ -9,9 +9,20 @@ defmodule NewRelic.DistributedTrace do
   alias NewRelic.Harvest.Collector.AgentRun
   alias NewRelic.Transaction
 
+  def start(type, headers \\ %{})
+
   def start(:http, headers) do
-    determine_context(:http, headers)
-    |> track_transaction(transport_type: "HTTP")
+    if NewRelic.Config.feature?(:distributed_tracing) do
+      determine_context(:http, headers)
+      |> track_transaction(transport_type: "HTTP")
+    end
+  end
+
+  def start(:other, _) do
+    if NewRelic.Config.feature?(:distributed_tracing) do
+      generate_new_context()
+      |> track_transaction(transport_type: "Other")
+    end
   end
 
   defp determine_context(:http, headers) do
diff --git a/lib/new_relic/harvest/harvest_cycle.ex b/lib/new_relic/harvest/harvest_cycle.ex
index d90a589..a27818d 100644
--- a/lib/new_relic/harvest/harvest_cycle.ex
+++ b/lib/new_relic/harvest/harvest_cycle.ex
@@ -140,7 +140,10 @@ defmodule NewRelic.Harvest.HarvestCycle do
 
   defp stop_harvest_cycle(timer), do: timer && Process.cancel_timer(timer)
 
-  defp trigger_harvest_cycle(%{lookup_module: lookup_module, harvest_cycle_key: harvest_cycle_key}) do
+  defp trigger_harvest_cycle(%{
+         lookup_module: lookup_module,
+         harvest_cycle_key: harvest_cycle_key
+       }) do
     harvest_cycle = lookup_module.lookup(harvest_cycle_key) || 60_000
     Process.send_after(self(), :harvest_cycle, harvest_cycle)
   end
diff --git a/lib/new_relic/init.ex b/lib/new_relic/init.ex
index 81882b8..1d7cbfc 100644
--- a/lib/new_relic/init.ex
+++ b/lib/new_relic/init.ex
@@ -71,6 +71,11 @@ defmodule NewRelic.Init do
             "NEW_RELIC_DB_QUERY_COLLECTION_ENABLED",
             :db_query_collection_enabled
           ),
+      distributed_tracing:
+        determine_feature(
+          "NEW_RELIC_DISTRIBUTED_TRACING_ENABLED",
+          :distributed_tracing_enabled
+        ),
       ecto_instrumentation:
         determine_feature(
           "NEW_RELIC_ECTO_INSTRUMENTATION_ENABLED",
diff --git a/lib/new_relic/other_transaction.ex b/lib/new_relic/other_transaction.ex
index c2a5534..9ab043c 100644
--- a/lib/new_relic/other_transaction.ex
+++ b/lib/new_relic/other_transaction.ex
@@ -3,9 +3,7 @@ defmodule NewRelic.OtherTransaction do
 
   def start_transaction(category, name) do
     NewRelic.Transaction.Reporter.start_transaction(:other)
-
-    NewRelic.DistributedTrace.generate_new_context()
-    |> NewRelic.DistributedTrace.track_transaction(transport_type: "Other")
+    NewRelic.DistributedTrace.start(:other)
 
     NewRelic.add_attributes(
       pid: inspect(self()),
diff --git a/lib/new_relic/sampler/process.ex b/lib/new_relic/sampler/process.ex
index 2d7a7a7..d9c7564 100644
--- a/lib/new_relic/sampler/process.ex
+++ b/lib/new_relic/sampler/process.ex
@@ -69,21 +69,26 @@ defmodule NewRelic.Sampler.Process do
     {current_sample, stats}
   end
 
-  @kb 1024
   def take_sample(pid) do
     # http://erlang.org/doc/man/erlang.html#process_info-2
     info = Process.info(pid, [:message_queue_len, :memory, :reductions, :registered_name])
 
     %{
       pid: inspect(pid),
-      memory_kb: info[:memory] / @kb,
+      memory_kb: kb(info[:memory]),
       message_queue_length: info[:message_queue_len],
       name: parse(:name, info[:registered_name]) || inspect(pid),
       reductions: info[:reductions]
     }
   end
 
-  defp delta(previous, current), do: %{reductions: current.reductions - previous.reductions}
+  @kb 1024
+  defp kb(nil), do: nil
+  defp kb(bytes), do: bytes / @kb
+
+  defp delta(%{reductions: nil}, _), do: nil
+  defp delta(_, %{reductions: nil}), do: nil
+  defp delta(%{reductions: prev}, %{reductions: curr}), do: %{reductions: curr - prev}
 
   defp parse(:name, []), do: nil
   defp parse(:name, name), do: inspect(name)
diff --git a/lib/new_relic/span/reporter.ex b/lib/new_relic/span/reporter.ex
index 5a59d08..83f0a37 100644
--- a/lib/new_relic/span/reporter.ex
+++ b/lib/new_relic/span/reporter.ex
@@ -2,7 +2,9 @@ defmodule NewRelic.Span.Reporter do
   @moduledoc false
 
   def report_span(span) do
-    case NewRelic.Config.feature(:infinite_tracing) do
+    case NewRelic.Config.feature?(:distributed_tracing) &&
+           NewRelic.Config.feature(:infinite_tracing) do
+      false -> :ignore
       :sampling -> NewRelic.Harvest.Collector.SpanEvent.Harvester.report_span(span)
       :infinite -> NewRelic.Harvest.TelemetrySdk.Spans.Harvester.report_span(span)
     end
diff --git a/lib/new_relic/util.ex b/lib/new_relic/util.ex
index a34a02a..3534949 100644
--- a/lib/new_relic/util.ex
+++ b/lib/new_relic/util.ex
@@ -78,7 +78,7 @@ defmodule NewRelic.Util do
       {key, value} when is_bitstring(value) ->
         case String.valid?(value) do
           true -> [{key, value}]
-          false -> [bad_value(key, value)]
+          false -> [{key, "[BINARY_VALUE]"}]
         end
 
       {key, value} when is_reference(value) when is_pid(value) when is_port(value) ->


Test patch:
diff --git a/test/distributed_trace_test.exs b/test/distributed_trace_test.exs
index 30c7c6d..dee4a95 100644
--- a/test/distributed_trace_test.exs
+++ b/test/distributed_trace_test.exs
@@ -15,8 +15,10 @@ defmodule DistributedTraceTest do
     plug(:dispatch)
 
     get "/" do
-      [{_, outbound_payload} | _] = NewRelic.distributed_trace_headers(:http)
-      send_resp(conn, 200, outbound_payload)
+      case NewRelic.distributed_trace_headers(:http) do
+        [{_, outbound_payload} | _] -> send_resp(conn, 200, outbound_payload)
+        [] -> send_resp(conn, 200, "nothin")
+      end
     end
 
     get "/w3c" do
@@ -66,6 +68,35 @@ defmodule DistributedTraceTest do
     :ok
   end
 
+  test "can disable Distributed Tracing" do
+    reset_config =
+      TestHelper.update(:nr_features,
+        distributed_tracing: false
+      )
+
+    TestHelper.restart_harvest_cycle(Collector.TransactionEvent.HarvestCycle)
+
+    TestHelper.request(
+      TestPlugApp,
+      conn(:get, "/")
+      |> put_req_header(@dt_header, generate_inbound_payload(:app))
+    )
+
+    [[_, attrs] | _] = TestHelper.gather_harvest(Collector.TransactionEvent.Harvester)
+
+    assert attrs[:name]
+    refute attrs[:error_reason]
+
+    refute attrs[:"parent.app"]
+    refute attrs[:parentId]
+    refute attrs[:parentSpanId]
+    refute attrs[:traceId]
+
+    TestHelper.pause_harvest_cycle(Collector.TransactionEvent.HarvestCycle)
+
+    reset_config.()
+  end
+
   test "Annotate Transaction event with DT attrs" do
     TestHelper.restart_harvest_cycle(Collector.TransactionEvent.HarvestCycle)
 
@@ -135,7 +166,7 @@ defmodule DistributedTraceTest do
     TestHelper.pause_harvest_cycle(Collector.Metric.HarvestCycle)
   end
 
-  test "propigate the context through connected Elixir processes" do
+  test "propagate the context through connected Elixir processes" do
     response =
       TestHelper.request(
         TestPlugApp,
diff --git a/test/transaction_test.exs b/test/transaction_test.exs
index e7effef..4e703ea 100644
--- a/test/transaction_test.exs
+++ b/test/transaction_test.exs
@@ -207,7 +207,6 @@ defmodule TransactionTest do
            end)
   end
 
-  @bad "[BAD_VALUE]"
   test "Attribute coercion" do
     TestHelper.restart_harvest_cycle(Collector.TransactionEvent.HarvestCycle)
 
@@ -233,11 +232,13 @@ defmodule TransactionTest do
     assert {:ok, _} = Date.from_iso8601(event[:date])
     assert {:ok, _} = Time.from_iso8601(event[:time])
 
+    # Binary values
+    assert event[:binary] == "[BINARY_VALUE]"
+
     # Bad values
-    assert event[:binary] == @bad
-    assert event[:tuple] == @bad
-    assert event[:function] == @bad
-    assert event[:struct] == @bad
+    assert event[:tuple] == "[BAD_VALUE]"
+    assert event[:function] == "[BAD_VALUE]"
+    assert event[:struct] == "[BAD_VALUE]"
 
     # Don't report nil values
     refute Map.has_key?(event, :nilValue)