Honorning buckets passed as part of PrometheusMetrics initialization … #174

[Rdgarg]

Honoring buckets passed as part of PrometheusMetrics initialization in Histogram metrics.

[Rdgarg]

#174

[coveralls]

coverage: 84.835% (+1.2%) from 83.617%
when pulling a461ff4 on Rdgarg:HonoringBucketsInHistogram
into 9e20f3f on rycus86:master.

[rycus86]

OK, I think the change is good, but I'm not sure this should hang off self.buckets.
The doc on that one says

:param buckets: the time buckets for request latencies (will use the default when `None`)

So up until now, it would only affect the default request latency metrics the library adds, not the manually decorated histogram metrics (you can add buckets=(...) on those specifically).
If you'd like to avoid duplicating the buckets, you can either make it a variable, or we could look at adding a default_histogram_buckets parameter here instead.

What do you think?

[Rdgarg]

Hi,

I dont think I fully got it. What do you mean by

If you'd like to avoid duplicating the buckets, you can either make it a variable, or we could look at adding a default_histogram_buckets parameter here instead.

[rycus86]

I meant, from your example, you could do this:

buckets = (0.005, 0.1, 0.2, INF)
metrics = PrometheusMetrics(app, buckets=buckets)
shorten_url_duration = metrics.histogram('shorten_url_duration', 'Duration of shorten_url call', buckets=buckets)

[Rdgarg]

I was already doing that :P
But shouldn't the buckets which got passed during PrometheusMetrics initialization be honored while creating a histogram metrics.

[rycus86]

We could do that, but we would change what that buckets parameter does currently on PrometheusMetrics where the docs say the time buckets for request latencies, which is why I suggested that perhaps a different variable could work there as some consumers might use this parameter currently and expect it to only apply to request latencies but not to custom histograms.
At that point though, manually declaring the buckets there may be cleaner?