Repository: newrelic/elixir_agent (elixir)

Issue:
Error Reporter Optional
**Is your feature request related to a problem? Please describe.**
Error reports that contain session information can be considered sensitive and often not desired to be stored in any third party.

**Describe the solution you'd like**
Either: disable the error reporter from starting **or** be able to filter out certain error types from the error.

2 This could potentially be difficult depending on the type of error. For instance, an unmatched clause in a phoenix controller will raise an error with the entire plug contents (including session)

**Describe alternatives you've considered**
The sasl logger could be manually removed at application boot time. This may actually be an acceptable answer, but could end up re-initializing the error handler if the new relic application crashes and restarts.


Pull request description:
Optionally disable error reporting
This PR adds the capability to opt-in to disabling error reporting. For example, you may have secure info in the Plug `conn`.

Thanks to @sb8244 for contributing this!

Note, I made a few changes from the contributed PR:
* **Changed the config option to be `error_collector_enabled`** in order to align with the same option in other New Relic agents
* Tweaked the testing to avoid fully restarting the application
* Made the config checking a little more flexible for future additions...

closes #33

Patch:
diff --git a/lib/new_relic/config.ex b/lib/new_relic/config.ex
index 2545bba..8bf7f69 100644
--- a/lib/new_relic/config.ex
+++ b/lib/new_relic/config.ex
@@ -59,6 +59,19 @@ defmodule NewRelic.Config do
     end)
   end
 
+  @doc """
+  Some Agent features can be controlled via configuration
+
+  * `:error_collector_enabled` (default `true`)
+  """
+  def feature?(:error_collector) do
+    case System.get_env("NEW_RELIC_ERROR_COLLECTOR_ENABLED") do
+      "true" -> true
+      "false" -> false
+      _ -> Application.get_env(:new_relic_agent, :error_collector_enabled, true)
+    end
+  end
+
   @doc false
   def enabled?, do: (harvest_enabled?() && app_name() && license_key() && true) || false
 
diff --git a/lib/new_relic/error/supervisor.ex b/lib/new_relic/error/supervisor.ex
index 4e86080..429fe2e 100644
--- a/lib/new_relic/error/supervisor.ex
+++ b/lib/new_relic/error/supervisor.ex
@@ -14,8 +14,11 @@ defmodule NewRelic.Error.Supervisor do
       supervisor(Task.Supervisor, [[name: NewRelic.Error.TaskSupervisor]])
     ]
 
-    :error_logger.delete_report_handler(NewRelic.Error.ErrorHandler)
-    :error_logger.add_report_handler(NewRelic.Error.ErrorHandler)
+    if NewRelic.Config.feature?(:error_collector) do
+      :error_logger.delete_report_handler(NewRelic.Error.ErrorHandler)
+      :error_logger.add_report_handler(NewRelic.Error.ErrorHandler)
+    end
+
     supervise(children, strategy: :one_for_one)
   end
 end
diff --git a/lib/new_relic/transaction/reporter.ex b/lib/new_relic/transaction/reporter.ex
index 321fd81..87c862a 100644
--- a/lib/new_relic/transaction/reporter.ex
+++ b/lib/new_relic/transaction/reporter.ex
@@ -58,14 +58,21 @@ defmodule NewRelic.Transaction.Reporter do
   end
 
   def stop(%{kind: _kind} = error) do
-    add_attributes(
-      end_time_mono: System.monotonic_time(),
-      error: true,
-      transaction_error: error,
-      error_kind: error.kind,
-      error_reason: inspect(error.reason),
-      error_stack: inspect(error.stack)
-    )
+    if NewRelic.Config.feature?(:error_collector) do
+      add_attributes(
+        end_time_mono: System.monotonic_time(),
+        error: true,
+        transaction_error: error,
+        error_kind: error.kind,
+        error_reason: inspect(error.reason),
+        error_stack: inspect(error.stack)
+      )
+    else
+      add_attributes(
+        end_time_mono: System.monotonic_time(),
+        error: true
+      )
+    end
 
     complete()
   end


Test patch:
diff --git a/test/config_test.exs b/test/config_test.exs
index ca4fcb4..f836569 100644
--- a/test/config_test.exs
+++ b/test/config_test.exs
@@ -38,4 +38,19 @@ defmodule ConfigTest do
     Application.put_env(:new_relic_agent, :automatic_attributes, [])
     System.delete_env("ENV_VAR_NAME")
   end
+
+  test "Can configure error collecting via ENV and Application" do
+    System.put_env("NEW_RELIC_ERROR_COLLECTOR_ENABLED", "false")
+
+    refute NewRelic.Config.feature?(:error_collector)
+
+    System.delete_env("NEW_RELIC_ERROR_COLLECTOR_ENABLED")
+
+    assert NewRelic.Config.feature?(:error_collector)
+
+    System.put_env("NEW_RELIC_ERROR_COLLECTOR_ENABLED", "true")
+    Application.get_env(:new_relic_agent, :error_collector_enabled, false)
+
+    assert NewRelic.Config.feature?(:error_collector)
+  end
 end
diff --git a/test/error_trace_test.exs b/test/error_trace_test.exs
index e1a6e49..913d067 100644
--- a/test/error_trace_test.exs
+++ b/test/error_trace_test.exs
@@ -103,8 +103,6 @@ defmodule ErrorTraceTest do
   @tag :capture_log
   test "record an expected error" do
     TestHelper.restart_harvest_cycle(Collector.ErrorTrace.HarvestCycle)
-    TestHelper.restart_harvest_cycle(Collector.Metric.HarvestCycle)
-    TestHelper.restart_harvest_cycle(Collector.TransactionErrorEvent.HarvestCycle)
     start_supervised({Task.Supervisor, name: TestSupervisor})
 
     {:exit, {_exception, _stacktrace}} =
@@ -131,8 +129,31 @@ defmodule ErrorTraceTest do
              | _
            ] = traces
 
-    TestHelper.pause_harvest_cycle(Collector.TransactionErrorEvent.HarvestCycle)
     TestHelper.pause_harvest_cycle(Collector.ErrorTrace.HarvestCycle)
-    TestHelper.pause_harvest_cycle(Collector.Metric.HarvestCycle)
+  end
+
+  test "Doesn't report an error if the handler is not installed" do
+    TestHelper.restart_harvest_cycle(Collector.ErrorTrace.HarvestCycle)
+    :error_logger.delete_report_handler(NewRelic.Error.ErrorHandler)
+
+    :proc_lib.spawn(fn ->
+      raise "RAISE"
+    end)
+
+    :timer.sleep(100)
+
+    traces = TestHelper.gather_harvest(Collector.ErrorTrace.Harvester)
+    assert length(traces) == 0
+
+    :error_logger.add_report_handler(NewRelic.Error.ErrorHandler)
+
+    :proc_lib.spawn(fn ->
+      raise "RAISE"
+    end)
+
+    :timer.sleep(100)
+
+    traces = TestHelper.gather_harvest(Collector.ErrorTrace.Harvester)
+    assert length(traces) == 1
   end
 end
diff --git a/test/transaction_test.exs b/test/transaction_test.exs
index 1defddc..e9cd4f4 100644
--- a/test/transaction_test.exs
+++ b/test/transaction_test.exs
@@ -129,10 +129,30 @@ defmodule TransactionTest do
 
     assert Enum.find(events, fn [_, event] ->
              event[:status] == 500 && event[:query] =~ "query{}" &&
-               event[:error_reason] =~ "TransactionError"
+               event[:error_reason] =~ "TransactionError" && event[:error_kind] == :error &&
+               event[:error_stack] =~ "test/transaction_test.exs"
            end)
   end
 
+  test "Allow disabling error collection" do
+    Application.put_env(:new_relic_agent, :error_collector_enabled, false)
+
+    TestHelper.restart_harvest_cycle(Collector.TransactionEvent.HarvestCycle)
+
+    assert_raise RuntimeError, fn ->
+      TestPlugApp.call(conn(:get, "/error"), [])
+    end
+
+    events = TestHelper.gather_harvest(Collector.TransactionEvent.Harvester)
+
+    assert Enum.find(events, fn [_, event] ->
+             event[:status] == 500 && event[:error] == true && event[:error_reason] == nil &&
+               event[:error_kind] == nil && event[:error_stack] == nil
+           end)
+
+    Application.delete_env(:new_relic_agent, :error_collector_enabled)
+  end
+
   test "Transaction with traced external service call" do
     TestHelper.restart_harvest_cycle(Collector.TransactionEvent.HarvestCycle)
     TestHelper.restart_harvest_cycle(Collector.CustomEvent.HarvestCycle)