beam-telemetry / telemetry_metrics

Collect and aggregate Telemetry events over time
https://hexdocs.pm/telemetry_metrics
Apache License 2.0
207 stars 33 forks source link

Extra metric options #33

Closed arkgil closed 4 years ago

arkgil commented 5 years ago

Sometimes it might be beneficial to have some extra information included in the metric definition. This data could be then used by a reporter. For example, a StatsD reporter could allow users to configure a sampling rate.

We could also move current distribution's :bucket option to these extra properties. Again, in the example of StatsD, the buckets are configured on the server side, so it doesn't make sense to pass them as a part of metric definition. The downside to that is that every reporter which needs the buckets to be defined on the reporting side would need to validate the buckets on its own.

I was thinking that these extra options could be passed in a keyword being the second argument of each metric definition:

counter("http.request.count", sampling_rate: 0.1)

and then we would store them in a new field of metric definition struct:

%Counter{
  ...
  extra_options: [sampling_rate: 0.1]
  ...
}

Thoughts?

josevalim commented 5 years ago
  1. counter("http.request.count", sampling_rate: 0.1) looks great but it means we won't be able to catch typos on key names and what not

  2. counter("http.request.count", extra_options: [sampling_rate: 0.1]) (or reporter_options) is more verbose, but provides better error messages and makes the rationale between those options clearer

Thoughts?

arkgil commented 5 years ago

Also, if we go with 1 and we decide to add a new key that some reporter already uses, that would break people's code.

josevalim commented 5 years ago

Yeah, I would go with :reporter_options then! --

José Valimwww.plataformatec.com.br http://www.plataformatec.com.br/Founder and Director of R&D

bryannaegele commented 5 years ago

Histograms in Statsd / Datadog are equivalent to Summaries in Prometheus and they're all distributions. The main difference for calculating Summaries in Prometheus seems to be you don't define the buckets, but you do define the quantiles to report. With Statsd, those are defined on the agent.

I feel the simplest and non-breaking option is to simply leave it the way it is but accept nil or something for the buckets definition. To calculate distributions, bucketing is always involved someplace, so it makes sense to me to leave it. For the Prometheus reporter, I was thinking there should be a way to take the quantiles to report and a summary: true option in the existing options and ignore the buckets.

It seems like all options in the definitions are reporter options in some regard. We could extend the current option validation to validate sample_rate if it's present.