Repository: beam-telemetry/telemetry_metrics_statsd (elixir)

Issue:
Crash when a tag is empty
Given the following test:

```
  test "it doesn't crash when tag values are empty" do
    {socket, port} = given_udp_port_opened()

    counter = given_counter("http.request.count", tags: [:method, :status])

    start_reporter(
      metrics: [counter],
      port: port,
      formatter: :standard
    )

    handlers_before = :telemetry.list_handlers([])

    :telemetry.execute([:http, :request], %{latency: 172}, %{method: "GET", status: ""})
    assert_reported(socket, "http.request.count.GET.nil:1|c")

    handlers_after = :telemetry.list_handlers([])
    assert handlers_after == handlers_before
  end
```

The handler crashes with the following logs:

```
02:33:41.200 [error] Handler {TelemetryMetricsStatsd.EventHandler, #PID<0.245.0>
, [:http, :request]} has failed and has been detached. Class=:error
Reason=:badarg
Stacktrace=[
  {:erlang, :iolist_to_binary,
   [[["http", 46, "request", 46, "count"], [46, "GET" | nil], 58, "1|c", ""]],
   [error_info: %{module: :erl_erts_errors}]},
  {TelemetryMetricsStatsd.Formatter, :format, 5,
   [file: 'lib/telemetry_metrics_statsd/formatter.ex', line: 25]},
  {TelemetryMetricsStatsd.EventHandler, :"-handle_event/4-fun-2-", 7,
   [file: 'lib/telemetry_metrics_statsd/event_handler.ex', line: 67]},
  {Enum, :"-reduce/3-lists^foldl/2-0-", 3, [file: 'lib/enum.ex', line: 2468]},
  {TelemetryMetricsStatsd.EventHandler, :handle_event, 4,
   [file: 'lib/telemetry_metrics_statsd/event_handler.ex', line: 58]},
  {:telemetry, :"-execute/3-fun-0-", 4,
   [
     file: '/home/alex/elixir/telemetry_metrics_statsd/deps/telemetry/src/teleme
try.erl',
     line: 150
   ]},
  {:lists, :foreach_1, 2, [file: 'lists.erl', line: 1442]},
  {TelemetryMetricsStatsdTest,
   :"test it doesn't crash when tag values are empty", 1,
   [file: 'test/telemetry_metrics_statsd_test.exs', line: 180]},
  {ExUnit.Runner, :exec_test, 1, [file: 'lib/ex_unit/runner.ex', line: 512]},
  {:timer, :tc, 1, [file: 'timer.erl', line: 235]},
  {ExUnit.Runner, :"-spawn_test_monitor/4-fun-1-", 4,
   [file: 'lib/ex_unit/runner.ex', line: 463]}
]
```

If I understand correctly, it crashes because the standard format doesn't support an empty string. However, the crash is inconvenient because the handler is detached, and future metrics will be lost.

A possible solution would be to omit it when a tag is empty, and log a warning message. Or at least a better error message would be interesting if you still think crashing is the way to go. It can be challenging to understand what is happening. It returns a `nil` in the middle of a list, and `:erlang.iolist_to_binary` raises an exception.

Thank you for your work.

Pull request description:
Fix handling of empty and missing tag values with standard formatter
Fixes #81

Patch:
diff --git a/lib/telemetry_metrics_statsd.ex b/lib/telemetry_metrics_statsd.ex
index 5ed7327..c165419 100644
--- a/lib/telemetry_metrics_statsd.ex
+++ b/lib/telemetry_metrics_statsd.ex
@@ -244,7 +244,7 @@ defmodule TelemetryMetricsStatsd do
 
   The summary metric is reported as [DataDog
   histogram](https://docs.datadoghq.com/developers/metrics/types/?tab=histogram), as that is the
-  metric that provides a set of statistics about gathered measurments on the DataDog side.
+  metric that provides a set of statistics about gathered measurements on the DataDog side.
 
   The distribution is flushed as [DataDog
   distribution](https://docs.datadoghq.com/developers/metrics/types/?tab=distribution) metric, which
diff --git a/lib/telemetry_metrics_statsd/formatter/standard.ex b/lib/telemetry_metrics_statsd/formatter/standard.ex
index 8ac5540..df1b32d 100644
--- a/lib/telemetry_metrics_statsd/formatter/standard.ex
+++ b/lib/telemetry_metrics_statsd/formatter/standard.ex
@@ -58,11 +58,11 @@ defmodule TelemetryMetricsStatsd.Formatter.Standard do
   end
 
   defp do_format_metric_tags([{_, nil} | tags]) do
-    [?., "nil" | format_metric_tags(tags)]
+    [?., "nil" | do_format_metric_tags(tags)]
   end
 
   defp do_format_metric_tags([{_, tag_value} | tags]) do
-    [?., to_string(tag_value) | format_metric_tags(tags)]
+    [?., to_string(tag_value) | do_format_metric_tags(tags)]
   end
 
   defp format_metric_value(%Metrics.Counter{}, _value), do: "1|c"
diff --git a/lib/telemetry_metrics_statsd/options.ex b/lib/telemetry_metrics_statsd/options.ex
index 5039132..b950355 100644
--- a/lib/telemetry_metrics_statsd/options.ex
+++ b/lib/telemetry_metrics_statsd/options.ex
@@ -6,7 +6,7 @@ defmodule TelemetryMetricsStatsd.Options do
       type: {:list, :any},
       required: true,
       doc:
-        "A lits of `Telemetry.Metrics` metric definitions that will be published by the reporter."
+        "A list of `Telemetry.Metrics` metric definitions that will be published by the reporter."
     ],
     host: [
       type: {:custom, __MODULE__, :host, []},
@@ -41,7 +41,7 @@ defmodule TelemetryMetricsStatsd.Options do
       default: [],
       doc:
         "Additional tags published with every metric. " <>
-          "Global tags are overriden by the tags specified in the metric definition."
+          "Global tags are overridden by the tags specified in the metric definition."
     ],
     prefix: [
       type: {:or, [:string, :atom]},


Test patch:
diff --git a/test/telemetry_metrics_statsd/formatter/standard_test.exs b/test/telemetry_metrics_statsd/formatter/standard_test.exs
index 30a8d51..70efa63 100644
--- a/test/telemetry_metrics_statsd/formatter/standard_test.exs
+++ b/test/telemetry_metrics_statsd/formatter/standard_test.exs
@@ -64,14 +64,14 @@ defmodule TelemetryMetricsStatsd.Formatter.StandardTest do
   test "nil tags are included in the formatted metric" do
     m = given_last_value("my.awesome.metric", tags: [:method, :status])
 
-    assert format(m, 131, method: nil, status: 200) ==
-             "my.awesome.metric.nil.200:131|g"
+    assert format(m, 131, method: "GET", status: nil) ==
+             "my.awesome.metric.GET.nil: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) == ""
+    assert format(m, 131, method: "GET", status: "") == ""
   end
 
   test "tags passed as explicit argument are used for the formatted metric" do
diff --git a/test/telemetry_metrics_statsd_test.exs b/test/telemetry_metrics_statsd_test.exs
index 40bd74c..ca80d08 100644
--- a/test/telemetry_metrics_statsd_test.exs
+++ b/test/telemetry_metrics_statsd_test.exs
@@ -144,7 +144,45 @@ defmodule TelemetryMetricsStatsdTest do
              "invalid value for :formatter option: expected :formatter be either :standard or :datadog, got :my_formatter"
   end
 
-  test "it doesn't crash when tag values are missing" do
+  test "it doesn't crash when tag values are missing with standard formatter" do
+    {socket, port} = given_udp_port_opened()
+
+    counter = given_counter("http.request.count", tags: [:method, :status])
+
+    start_reporter(
+      metrics: [counter],
+      port: port
+    )
+
+    handlers_before = :telemetry.list_handlers([])
+
+    :telemetry.execute([:http, :request], %{latency: 172}, %{method: "GET"})
+    assert_reported(socket, "")
+
+    handlers_after = :telemetry.list_handlers([])
+    assert handlers_after == handlers_before
+  end
+
+  test "it doesn't crash when tag values are nil with standard formatter" do
+    {socket, port} = given_udp_port_opened()
+
+    counter = given_counter("http.request.count", tags: [:method, :status])
+
+    start_reporter(
+      metrics: [counter],
+      port: port
+    )
+
+    handlers_before = :telemetry.list_handlers([])
+
+    :telemetry.execute([:http, :request], %{latency: 172}, %{method: "GET", status: nil})
+    assert_reported(socket, "http.request.count.GET.nil:1|c")
+
+    handlers_after = :telemetry.list_handlers([])
+    assert handlers_after == handlers_before
+  end
+
+  test "it doesn't crash when tag values are missing with DataDog formatter" do
     {socket, port} = given_udp_port_opened()
 
     counter = given_counter("http.request.count", tags: [:method, :status])