Repository: beam-telemetry/telemetry_metrics_statsd (elixir)

Issue:
Crash at event_handler.ex:63 if tag value is missing - intentional?
I have a question regarding `TelemetryMetricsStatsd` and its `EventHandler`. In our environments we very subtly lose all of our metrics due to a crash which we suspect is caused by `Map.fetch!/2` as a tag value is missing.

Is it intentional to crash here if the value does not exist for a given tag? If that is the case, is this documented?

Our metrics.ex is currently very long and messy due to how tricky it is to manage the tags for different kinds of metrics, and I was hoping that it would be fine to configure the metrics such that if a value for the key exists, just use that, so that we can reuse a lot of functions. This means that more of the tag handling is common for all metrics, and the exceptions are handled more cleanly.

Currently this behaviour is not the same as the `ConsoleReporter` that I tend to use when I develop as we do not want to report any metrics to anywhere during development, but if it looks this way I either need to do that or to implement some kind of tests that copy this behaviour such that we catch the crash before deployment. Currently this process dies upon deployment and then we're blind.

This is from the tag 0.4.0 I think, but the line still exists in latest.

https://github.com/beam-telemetry/telemetry_metrics_statsd/blob/cda8cca8b204c8c870b2da691817bab7bd6dc4f1/lib/telemetry_metrics_statsd/event_handler.ex#L63

Pull request description:
Allow empty and missing tag values
With standard StatsD formatter, if the tag value is empty or missing,
the metric update is not sent at all, because the formatted metric name
would be malformed.

With DataDog formatter, an empty tag value is sent.

Closes #43 

Patch:
diff --git a/lib/telemetry_metrics_statsd/event_handler.ex b/lib/telemetry_metrics_statsd/event_handler.ex
index 7eed000..18ac873 100644
--- a/lib/telemetry_metrics_statsd/event_handler.ex
+++ b/lib/telemetry_metrics_statsd/event_handler.ex
@@ -63,7 +63,7 @@ defmodule TelemetryMetricsStatsd.EventHandler do
             |> Map.new()
             |> Map.merge(metric.tag_values.(metadata))
 
-          tags = Enum.map(metric.tags, &{&1, Map.fetch!(tag_values, &1)})
+          tags = Enum.map(metric.tags, &{&1, Map.get(tag_values, &1, "")})
           Formatter.format(formatter_mod, metric, prefix, value, tags)
         else
           :nopublish
diff --git a/lib/telemetry_metrics_statsd/formatter/standard.ex b/lib/telemetry_metrics_statsd/formatter/standard.ex
index 1165160..302d1b3 100644
--- a/lib/telemetry_metrics_statsd/formatter/standard.ex
+++ b/lib/telemetry_metrics_statsd/formatter/standard.ex
@@ -9,18 +9,24 @@ defmodule TelemetryMetricsStatsd.Formatter.Standard do
 
   @impl true
   def format(metric, value, tags) do
-    case format_metric_value(metric, value) do
-      [] ->
+    case format_metric_tags(tags) do
+      nil ->
         []
 
-      val ->
-        [
-          format_metric_name(metric.name),
-          format_metric_tags(tags),
-          ?:,
-          val,
-          format_sampling_rate(metric.reporter_options)
-        ]
+      formatted_tags ->
+        case format_metric_value(metric, value) do
+          nil ->
+            []
+
+          formatted_value ->
+            [
+              format_metric_name(metric.name),
+              formatted_tags,
+              ?:,
+              formatted_value,
+              format_sampling_rate(metric.reporter_options)
+            ]
+        end
     end
   end
 
@@ -32,15 +38,26 @@ defmodule TelemetryMetricsStatsd.Formatter.Standard do
     [:erlang.atom_to_binary(segment, :utf8), ?. | format_metric_name(segments)]
   end
 
-  defp format_metric_tags([]) do
+  defp format_metric_tags(tags) do
+    do_format_metric_tags(tags)
+  catch
+    :throw, :empty ->
+      nil
+  end
+
+  defp do_format_metric_tags([]) do
     []
   end
 
-  defp format_metric_tags([{_, nil} | tags]) do
+  defp do_format_metric_tags([{_, ""} | _tags]) do
+    throw(:empty)
+  end
+
+  defp do_format_metric_tags([{_, nil} | tags]) do
     [?., "nil" | format_metric_tags(tags)]
   end
 
-  defp format_metric_tags([{_, tag_value} | tags]) do
+  defp do_format_metric_tags([{_, tag_value} | tags]) do
     [?., to_string(tag_value) | format_metric_tags(tags)]
   end
 
@@ -70,7 +87,7 @@ defmodule TelemetryMetricsStatsd.Formatter.Standard do
       "Unable to format negative value: #{inspect(value)} for reporting to StatsD Counter"
     )
 
-    []
+    nil
   end
 
   defp format_sum_metric_value(%Metrics.Sum{}, value) when value >= 0,


Test patch:
diff --git a/test/telemetry_metrics_statsd/formatter/datadog_test.exs b/test/telemetry_metrics_statsd/formatter/datadog_test.exs
index 6764387..1a1edd1 100644
--- a/test/telemetry_metrics_statsd/formatter/datadog_test.exs
+++ b/test/telemetry_metrics_statsd/formatter/datadog_test.exs
@@ -55,6 +55,13 @@ defmodule TelemetryMetricsStatsd.Formatter.DatadogTest do
              "my.awesome.metric:131|g|#method:nil,status:200"
   end
 
+  test "empty tag values are included in the formatted metric" do
+    m = given_last_value("my.awesome.metric", tags: [:method, :status])
+
+    assert format(m, 131, method: "", status: 200) ==
+             "my.awesome.metric:131|g|#method:,status:200"
+  end
+
   test "tags passed as explicit argument are used for the formatted metric" do
     m = given_last_value("my.awesome.metric", tags: [:whatever])
 
diff --git a/test/telemetry_metrics_statsd/formatter/standard_test.exs b/test/telemetry_metrics_statsd/formatter/standard_test.exs
index 3c844d5..30a8d51 100644
--- a/test/telemetry_metrics_statsd/formatter/standard_test.exs
+++ b/test/telemetry_metrics_statsd/formatter/standard_test.exs
@@ -68,6 +68,12 @@ defmodule TelemetryMetricsStatsd.Formatter.StandardTest do
              "my.awesome.metric.nil.200:131|g"
   end
 
+  test "empty string tag values are dropped" do
+    m = given_last_value("my.awesome.metric", tags: [:method, :status])
+
+    assert format(m, 131, method: "", status: 200) == ""
+  end
+
   test "tags passed as explicit argument are used for the formatted metric" do
     m = given_last_value("my.awesome.metric", tags: [:whatever])
 
diff --git a/test/telemetry_metrics_statsd_test.exs b/test/telemetry_metrics_statsd_test.exs
index f05b160..a5160dd 100644
--- a/test/telemetry_metrics_statsd_test.exs
+++ b/test/telemetry_metrics_statsd_test.exs
@@ -142,6 +142,26 @@ defmodule TelemetryMetricsStatsdTest do
     end
   end
 
+  test "it doesn't crash when tag values are missing" do
+    {socket, port} = given_udp_port_opened()
+
+    counter = given_counter("http.request.count", tags: [:method, :status])
+
+    start_reporter(
+      metrics: [counter],
+      port: port,
+      formatter: :datadog
+    )
+
+    handlers_before = :telemetry.list_handlers([])
+
+    :telemetry.execute([:http, :request], %{latency: 172}, %{method: "GET"})
+    assert_reported(socket, "http.request.count:1|c|#method:GET,status:")
+
+    handlers_after = :telemetry.list_handlers([])
+    assert handlers_after == handlers_before
+  end
+
   test "measurement function is taken into account when getting the value for the metric" do
     {socket, port} = given_udp_port_opened()
     last_value = given_last_value("vm.memory.total", measurement: fn m -> m.total * 2 end)