Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Honorning buckets passed as part of PrometheusMetrics initialization … #174 #175

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Honorning buckets passed as part of PrometheusMetrics initialization … #174 #175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions prometheus_flask_exporter/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,10 @@ def _track(self, metric_type, metric_call, metric_kwargs, name, description, lab

labels = self._get_combined_labels(labels)

if metric_type is Histogram:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

if metric_kwargs.get('buckets') is None and self.buckets is not None:
metric_kwargs['buckets'] = self.buckets

parent_metric = metric_type(
name, description, labelnames=labels.keys(), registry=registry,
**metric_kwargs
Expand Down
37 changes: 37 additions & 0 deletions tests/test_defaults.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@ def decorate_metrics(f):
def decorated(*args):
invocations.append('metrics')
return f(*args)

return decorated

self.metrics(metrics_decorator=decorate_metrics)
Expand Down Expand Up @@ -639,6 +640,42 @@ def test():
('le', '5.0'), ('method', 'GET'), ('path', '/test'), ('status', 200)
)

def test_custom_buckets_honored_in_histogram(self):
metrics = self.metrics(buckets=(0.2, 2, 4))

@self.app.route('/test')
@metrics.histogram('hist_1', 'Histogram 1')
def test1():
return 'OK'

self.client.get('/test')
self.client.get('/test')

self.assertMetric('hist_1_bucket', '2.0', ('le', '0.2'))

self.assertMetric('hist_1_bucket', '2.0', ('le', '2.0'))

self.assertMetric('hist_1_bucket', '2.0', ('le', '4.0'))

self.assertMetric('hist_1_bucket', '2.0', ('le', '+Inf'))

self.assertAbsent(
'hist_1_bucket',
('le', '0.1'), ('method', 'GET'), ('path', '/test'), ('status', 200)
)
self.assertAbsent(
'hist_1',
('le', '0.3'), ('method', 'GET'), ('path', '/test'), ('status', 200)
)
self.assertAbsent(
'hist_1',
('le', '1.2'), ('method', 'GET'), ('path', '/test'), ('status', 200)
)
self.assertAbsent(
'hist_1',
('le', '5.0'), ('method', 'GET'), ('path', '/test'), ('status', 200)
)

def test_invalid_labels(self):
metrics = self.metrics()

Expand Down
Loading