beam-telemetry/telemetry_metrics

Extra metric options

Closed this issue · 4 comments

arkgil commented

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

  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

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
bryannaegele commented

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.