Add support for sampling rate #28
Conversation
There was a problem hiding this comment.
Hey, thanks for the PR! ⚡️
Apart from specific comments there is one missing piece here, meaning pushing the sampling rate down to the formatter. Using a sampling rate without reporting it upstream may lead to invalid data, e.g. a counter with sampling rate of 0.1 bumped up 100 times would show up as 10 (on average) in the system we report to.
Fortunately, both stock StatsD and Datadog support providing the sampling rate. See https://docs.datadoghq.com/developers/dogstatsd/datagram_shell?tab=metrics and https://github.com/statsd/statsd/blob/master/docs/metric_types.md#sampling.
mix.lock
Outdated
| @@ -1,21 +1,21 @@ | |||
| %{ | |||
| "certifi": {:hex, :certifi, "2.5.1", "867ce347f7c7d78563450a18a6a28a8090331e77fa02380b4a21962a65d36ee5", [:rebar3], [{:parse_trans, "~>3.3", [hex: :parse_trans, repo: "hexpm", optional: false]}], "hexpm"}, | |||
| "dialyxir": {:hex, :dialyxir, "0.5.1", "b331b091720fd93e878137add264bac4f644e1ddae07a70bf7062c7862c4b952", [:mix], [], "hexpm"}, | |||
| "earmark": {:hex, :earmark, "1.3.2", "b840562ea3d67795ffbb5bd88940b1bed0ed9fa32834915125ea7d02e35888a5", [:mix], [], "hexpm"}, | |||
| "dialyxir": {:hex, :dialyxir, "0.5.1", "b331b091720fd93e878137add264bac4f644e1ddae07a70bf7062c7862c4b952", [:mix], [], "hexpm", "6c32a70ed5d452c6650916555b1f96c79af5fc4bf286997f8b15f213de786f73"}, | |||
There was a problem hiding this comment.
Can you revert the dependency update change? This is unrelated to the PR, we can do it in another one 🙂
|
|
||
| @spec sample(Metrics.t()) :: Metrics.measurement() | nil | ||
| defp sample(metric) do | ||
| rate = Keyword.get(metric.reporter_options, :sample_rate, 1.0) |
There was a problem hiding this comment.
| rate = Keyword.get(metric.reporter_options, :sample_rate, 1.0) | |
| rate = Keyword.get(metric.reporter_options, :sampling_rate, 1.0) |
Can we use that name, or is sample_rate the correct wording?
There was a problem hiding this comment.
Renamed. I wondered about that, but went with Bryan's comment from beam-telemetry/telemetry_metrics#33 since he was the last to say anything.
|
I've updated both the standard and datadog formatters to include sampling rate. Not terribly happy with the duplicate use of |
lib/telemetry_metrics_statsd.ex
Outdated
| ] | ||
| ) | ||
|
|
||
| In this example, we are captureing 100% of the measurements for the counter, |
There was a problem hiding this comment.
Thaks. Speling is hard.
|
Ah, @samullen, it is failing on OTP 19, because |
|
For whatever the reason, I thought |
|
Thanks! 🎉 |
telemetry_metrics recently added
:reporter_options(telemetry_metrics#52) initiated from a request to add extra metrics options.This PR allows developers to pass
[sample_rate: <rate>](where "" is a value between 0.0 and 1.0) to:reporter_optionsto limit how many measurements to capture.