Repository: newrelic/elixir_agent
Pull request #31 (2018-10-01 17:25:42)

Extend the timeout on harvester send_harvest
This PR extends the time that a harvester Task is willing to wait for the `send_harvest` action. The action is essentially doing an HTTPoison request, which has it's own built-in timeout values of `8000` for connecting and `5000` for receiving a response.

The Task isn't blocking anything so this won't impact anything else in the agent.


Patch:
diff --git a/lib/new_relic/harvest/collector/custom_event/harvester.ex b/lib/new_relic/harvest/collector/custom_event/harvester.ex
index dc1b8fa..6e204d5 100644
--- a/lib/new_relic/harvest/collector/custom_event/harvester.ex
+++ b/lib/new_relic/harvest/collector/custom_event/harvester.ex
@@ -47,15 +47,6 @@ defmodule NewRelic.Harvest.Collector.CustomEvent.Harvester do
       |> Collector.HarvestCycle.current_harvester()
       |> GenServer.call(:gather_harvest)
 
-  def complete(nil), do: :ignore
-
-  def complete(harvester) do
-    Task.Supervisor.start_child(Collector.CustomEvent.TaskSupervisor, fn ->
-      GenServer.call(harvester, :send_harvest)
-      Supervisor.terminate_child(Collector.CustomEvent.HarvesterSupervisor, harvester)
-    end)
-  end
-
   # Server
 
   def handle_cast(_late_msg, :completed), do: {:noreply, :completed}
diff --git a/lib/new_relic/harvest/collector/data_supervisor.ex b/lib/new_relic/harvest/collector/data_supervisor.ex
index 98fa1cf..0a1cd13 100644
--- a/lib/new_relic/harvest/collector/data_supervisor.ex
+++ b/lib/new_relic/harvest/collector/data_supervisor.ex
@@ -15,7 +15,6 @@ defmodule NewRelic.Harvest.Collector.DataSupervisor do
     harvester_cycle = Module.concat(namespace, HarvestCycle)
 
     children = [
-      supervisor(Task.Supervisor, [[name: Module.concat(namespace, TaskSupervisor)]]),
       supervisor(Collector.HarvesterSupervisor, [
         [harvester: harvester, name: harvester_supervisor]
       ]),
@@ -23,7 +22,6 @@ defmodule NewRelic.Harvest.Collector.DataSupervisor do
         [
           name: harvester_cycle,
           harvest_cycle_key: harvest_cycle_key,
-          module: harvester,
           supervisor: harvester_supervisor
         ]
       ])
diff --git a/lib/new_relic/harvest/collector/error_trace/harvester.ex b/lib/new_relic/harvest/collector/error_trace/harvester.ex
index 81a4618..dd8f82b 100644
--- a/lib/new_relic/harvest/collector/error_trace/harvester.ex
+++ b/lib/new_relic/harvest/collector/error_trace/harvester.ex
@@ -35,15 +35,6 @@ defmodule NewRelic.Harvest.Collector.ErrorTrace.Harvester do
       |> Collector.HarvestCycle.current_harvester()
       |> GenServer.call(:gather_harvest)
 
-  def complete(nil), do: :ignore
-
-  def complete(harvester) do
-    Task.Supervisor.start_child(Collector.ErrorTrace.TaskSupervisor, fn ->
-      GenServer.call(harvester, :send_harvest)
-      Supervisor.terminate_child(Collector.ErrorTrace.HarvesterSupervisor, harvester)
-    end)
-  end
-
   # Server
 
   def handle_cast(_late_msg, :completed), do: {:noreply, :completed}
diff --git a/lib/new_relic/harvest/collector/harvest_cycle.ex b/lib/new_relic/harvest/collector/harvest_cycle.ex
index ff32113..937c5b5 100644
--- a/lib/new_relic/harvest/collector/harvest_cycle.ex
+++ b/lib/new_relic/harvest/collector/harvest_cycle.ex
@@ -14,7 +14,6 @@ defmodule NewRelic.Harvest.Collector.HarvestCycle do
   def init(
         name: name,
         harvest_cycle_key: harvest_cycle_key,
-        module: module,
         supervisor: supervisor
       ) do
     if NewRelic.Config.enabled?(), do: send(self(), :harvest_cycle)
@@ -23,7 +22,6 @@ defmodule NewRelic.Harvest.Collector.HarvestCycle do
      %{
        name: name,
        harvest_cycle_key: harvest_cycle_key,
-       module: module,
        supervisor: supervisor,
        harvester: nil,
        timer: nil
@@ -55,8 +53,8 @@ defmodule NewRelic.Harvest.Collector.HarvestCycle do
     {:reply, :ok, %{state | harvester: harvester, timer: timer}}
   end
 
-  def handle_call(:pause, _from, %{harvester: harvester, timer: old_timer} = state) do
-    stop_harvester(state, harvester)
+  def handle_call(:pause, _from, %{timer: old_timer} = state) do
+    stop_harvester(state)
     stop_harvest_cycle(old_timer)
     {:reply, :ok, %{state | harvester: nil, timer: nil}}
   end
@@ -88,18 +86,31 @@ defmodule NewRelic.Harvest.Collector.HarvestCycle do
 
   # Helpers
 
-  defp swap_harvester(%{supervisor: supervisor, name: name, module: harvester_module}) do
+  defp swap_harvester(%{supervisor: supervisor, name: name, harvester: harvester}) do
     {:ok, next} = Supervisor.start_child(supervisor, [])
     Process.monitor(next)
-    prev = Collector.HarvesterStore.current(name)
     Collector.HarvesterStore.update(name, next)
-    harvester_module.complete(prev)
+    send_harvest(supervisor, harvester)
     next
   end
 
-  defp stop_harvester(%{name: name, module: harvester_module}, harvester) do
+  defp stop_harvester(%{supervisor: supervisor, name: name, harvester: harvester}) do
     Collector.HarvesterStore.update(name, nil)
-    harvester_module.complete(harvester)
+    send_harvest(supervisor, harvester)
+  end
+
+  def send_harvest(_supervisor, nil), do: :no_harvester
+
+  @harvest_timeout 15_000
+  def send_harvest(supervisor, harvester) do
+    Task.Supervisor.start_child(
+      Collector.TaskSupervisor,
+      fn ->
+        GenServer.call(harvester, :send_harvest, @harvest_timeout)
+        Supervisor.terminate_child(supervisor, harvester)
+      end,
+      shutdown: @harvest_timeout
+    )
   end
 
   defp stop_harvest_cycle(timer), do: timer && Process.cancel_timer(timer)
diff --git a/lib/new_relic/harvest/collector/metric/harvester.ex b/lib/new_relic/harvest/collector/metric/harvester.ex
index ebad969..808d236 100644
--- a/lib/new_relic/harvest/collector/metric/harvester.ex
+++ b/lib/new_relic/harvest/collector/metric/harvester.ex
@@ -33,15 +33,6 @@ defmodule NewRelic.Harvest.Collector.Metric.Harvester do
       |> Collector.HarvestCycle.current_harvester()
       |> GenServer.call(:gather_harvest)
 
-  def complete(nil), do: :ignore
-
-  def complete(harvester) do
-    Task.Supervisor.start_child(Collector.Metric.TaskSupervisor, fn ->
-      GenServer.call(harvester, :send_harvest)
-      Supervisor.terminate_child(Collector.Metric.HarvesterSupervisor, harvester)
-    end)
-  end
-
   # Server
 
   def handle_cast(_late_msg, :completed), do: {:noreply, :completed}
diff --git a/lib/new_relic/harvest/collector/span_event/harvester.ex b/lib/new_relic/harvest/collector/span_event/harvester.ex
index 9708dd9..1553387 100644
--- a/lib/new_relic/harvest/collector/span_event/harvester.ex
+++ b/lib/new_relic/harvest/collector/span_event/harvester.ex
@@ -76,15 +76,6 @@ defmodule NewRelic.Harvest.Collector.SpanEvent.Harvester do
       |> Collector.HarvestCycle.current_harvester()
       |> GenServer.call(:gather_harvest)
 
-  def complete(nil), do: :ignore
-
-  def complete(harvester) do
-    Task.Supervisor.start_child(Collector.SpanEvent.TaskSupervisor, fn ->
-      GenServer.call(harvester, :send_harvest)
-      Supervisor.terminate_child(Collector.SpanEvent.HarvesterSupervisor, harvester)
-    end)
-  end
-
   # Server
 
   def handle_cast(_late_msg, :completed), do: {:noreply, :completed}
diff --git a/lib/new_relic/harvest/collector/transaction_error_event/harvester.ex b/lib/new_relic/harvest/collector/transaction_error_event/harvester.ex
index 60ecd25..a4827b2 100644
--- a/lib/new_relic/harvest/collector/transaction_error_event/harvester.ex
+++ b/lib/new_relic/harvest/collector/transaction_error_event/harvester.ex
@@ -38,15 +38,6 @@ defmodule NewRelic.Harvest.Collector.TransactionErrorEvent.Harvester do
       |> Collector.HarvestCycle.current_harvester()
       |> GenServer.call(:gather_harvest)
 
-  def complete(nil), do: :ignore
-
-  def complete(harvester) do
-    Task.Supervisor.start_child(Collector.TransactionErrorEvent.TaskSupervisor, fn ->
-      GenServer.call(harvester, :send_harvest)
-      Supervisor.terminate_child(Collector.TransactionErrorEvent.HarvesterSupervisor, harvester)
-    end)
-  end
-
   # Server
 
   def handle_cast(_late_msg, :completed), do: {:noreply, :completed}
diff --git a/lib/new_relic/harvest/collector/transaction_event/harvester.ex b/lib/new_relic/harvest/collector/transaction_event/harvester.ex
index 6bcfc72..a0d0ce5 100644
--- a/lib/new_relic/harvest/collector/transaction_event/harvester.ex
+++ b/lib/new_relic/harvest/collector/transaction_event/harvester.ex
@@ -39,15 +39,6 @@ defmodule NewRelic.Harvest.Collector.TransactionEvent.Harvester do
       |> Collector.HarvestCycle.current_harvester()
       |> GenServer.call(:gather_harvest)
 
-  def complete(nil), do: :ignore
-
-  def complete(harvester) do
-    Task.Supervisor.start_child(Collector.TransactionEvent.TaskSupervisor, fn ->
-      GenServer.call(harvester, :send_harvest)
-      Supervisor.terminate_child(Collector.TransactionEvent.HarvesterSupervisor, harvester)
-    end)
-  end
-
   # Server
 
   def handle_cast(_late_msg, :completed), do: {:noreply, :completed}
diff --git a/lib/new_relic/harvest/collector/transaction_trace/harvester.ex b/lib/new_relic/harvest/collector/transaction_trace/harvester.ex
index 494c9bc..26d7573 100644
--- a/lib/new_relic/harvest/collector/transaction_trace/harvester.ex
+++ b/lib/new_relic/harvest/collector/transaction_trace/harvester.ex
@@ -40,15 +40,6 @@ defmodule NewRelic.Harvest.Collector.TransactionTrace.Harvester do
       |> Collector.HarvestCycle.current_harvester()
       |> GenServer.call(:gather_harvest)
 
-  def complete(nil), do: :ignore
-
-  def complete(harvester) do
-    Task.Supervisor.start_child(Collector.TransactionTrace.TaskSupervisor, fn ->
-      GenServer.call(harvester, :send_harvest)
-      Supervisor.terminate_child(Collector.TransactionTrace.HarvesterSupervisor, harvester)
-    end)
-  end
-
   # Server
 
   def handle_cast(_late_msg, :completed), do: {:noreply, :completed}


Test patch:
diff --git a/test/custom_event_test.exs b/test/custom_event_test.exs
index 27c1afb..189af25 100644
--- a/test/custom_event_test.exs
+++ b/test/custom_event_test.exs
@@ -48,7 +48,7 @@ defmodule CustomEventTest do
 
     # Verify that the Harvester shuts down w/o error
     Process.monitor(harvester)
-    Collector.CustomEvent.Harvester.complete(harvester)
+    Collector.HarvestCycle.send_harvest(Collector.CustomEvent.HarvesterSupervisor, harvester)
     assert_receive {:DOWN, _ref, _, ^harvester, :shutdown}, 1000
   end
 
diff --git a/test/error_trace_test.exs b/test/error_trace_test.exs
index 51d0605..e1a6e49 100644
--- a/test/error_trace_test.exs
+++ b/test/error_trace_test.exs
@@ -42,7 +42,7 @@ defmodule ErrorTraceTest do
 
     # Verify that the Harvester shuts down w/o error
     Process.monitor(harvester)
-    Collector.ErrorTrace.Harvester.complete(harvester)
+    Collector.HarvestCycle.send_harvest(Collector.ErrorTrace.HarvesterSupervisor, harvester)
     assert_receive {:DOWN, _ref, _, ^harvester, :shutdown}, 1000
   end
 
diff --git a/test/metric_harvester_test.exs b/test/metric_harvester_test.exs
index 1f20031..b828db5 100644
--- a/test/metric_harvester_test.exs
+++ b/test/metric_harvester_test.exs
@@ -19,7 +19,7 @@ defmodule MetricHarvesterTest do
 
     # Verify that the Harvester shuts down w/o error
     Process.monitor(harvester)
-    Collector.Metric.Harvester.complete(harvester)
+    Collector.HarvestCycle.send_harvest(Collector.Metric.HarvesterSupervisor, harvester)
     assert_receive {:DOWN, _ref, _, ^harvester, :shutdown}, 1000
   end
 
diff --git a/test/span_event_test.exs b/test/span_event_test.exs
index f8d35ee..bac6cdb 100644
--- a/test/span_event_test.exs
+++ b/test/span_event_test.exs
@@ -62,7 +62,7 @@ defmodule SpanEventTest do
 
     # Verify that the Harvester shuts down w/o error
     Process.monitor(harvester)
-    Collector.SpanEvent.Harvester.complete(harvester)
+    Collector.HarvestCycle.send_harvest(Collector.SpanEvent.HarvesterSupervisor, harvester)
     assert_receive {:DOWN, _ref, _, ^harvester, :shutdown}, 1000
 
     Application.delete_env(:new_relic_agent, :span_event_reservoir_size)
diff --git a/test/transaction_error_event_test.exs b/test/transaction_error_event_test.exs
index c5de97c..3e10905 100644
--- a/test/transaction_error_event_test.exs
+++ b/test/transaction_error_event_test.exs
@@ -102,7 +102,12 @@ defmodule TransactionErrorEventTest do
 
     # Verify that the Harvester shuts down w/o error
     Process.monitor(harvester)
-    Collector.TransactionErrorEvent.Harvester.complete(harvester)
+
+    Collector.HarvestCycle.send_harvest(
+      Collector.TransactionErrorEvent.HarvesterSupervisor,
+      harvester
+    )
+
     assert_receive {:DOWN, _ref, _, ^harvester, :shutdown}, 1000
   end
 
diff --git a/test/transaction_event_test.exs b/test/transaction_event_test.exs
index f5fb893..43e22fa 100644
--- a/test/transaction_event_test.exs
+++ b/test/transaction_event_test.exs
@@ -64,7 +64,7 @@ defmodule TransactionEventTest do
 
     # Verify that the Harvester shuts down w/o error
     Process.monitor(harvester)
-    Collector.TransactionEvent.Harvester.complete(harvester)
+    Collector.HarvestCycle.send_harvest(Collector.TransactionEvent.HarvesterSupervisor, harvester)
     assert_receive {:DOWN, _ref, _, ^harvester, :shutdown}, 1000
 
     Application.delete_env(:new_relic_agent, :transaction_event_reservoir_size)
diff --git a/test/transaction_trace_test.exs b/test/transaction_trace_test.exs
index 8e35509..3b1587f 100644
--- a/test/transaction_trace_test.exs
+++ b/test/transaction_trace_test.exs
@@ -81,7 +81,7 @@ defmodule TransactionTraceTest do
 
     # Verify that the Harvester shuts down w/o error
     Process.monitor(harvester)
-    Collector.TransactionTrace.Harvester.complete(harvester)
+    Collector.HarvestCycle.send_harvest(Collector.TransactionTrace.HarvesterSupervisor, harvester)
     assert_receive {:DOWN, _ref, _, ^harvester, :shutdown}, 1000
   end