Allow to add labels inside a context manager

[witsch]

This is an attempt to use time() as a context manager, but still be able to add labels with values that depend on the result of the timed operation. In our case we'd like to monitor the duration of a number of web requests, but still distinguish/label them by the returned HTTP error code. It didn't seem possible to use the provided time() helper in this case, but instead we would have to measure the time ourselves and then use something like metric.labels(status=status_code).observe(duration).

Being able to add the label inside the context manager seemed a bit more convenient:

from prometheus_client import Histogram
from requests import get

teapot = Histogram('teapot', 'A teapot', ['status'])
with teapot.time() as metric:
    response = get('https://httpbin.org/status/418')
    metric.labels(status=response.status_code)

This should now work with all three time() helpers. However, the 'observability' checks for 'gauge' and 'summary' metrics had to be deferred (to their respective __exit__ methods; see 2b57f24), because there is no way to tell upfront if a label will in fact be added, making the metric 'observable'.

I hope that's an acceptable change. If so, the track_inprogress() helper should probably also be changed accordingly.

[csmarchbanks]

Thanks for the PR! I think the new behavior is certainly helpful, but I am not sure of the implementation yet. Happy to brainstorm on some ways to avoid __self__ and __name__ if possible.

[csmarchbanks]

I don't love the references back to __self__ and __name__. I wonder if we could avoid that, but haven't come up with a great way yet. Perhaps a second variable to __init__ could help?

[witsch]

Thanks for the PR! I think the new behavior is certainly helpful, but I am not sure of the implementation yet. Happy to brainstorm on some ways to avoid __self__ and __name__ if possible.

I suppose an easy change would be to pass both the metric instance and the name of the callback method when initializing Timer, i.e. return Timer(self, 'observe') instead of return Timer(self.observe) as it is now. This would only affect three places in the code (afaics), but I was trying to keep the changes at a minimum.

Alternatively, an alias like callback = observe (in Summary and Histogram) and callback = set (in Gauge) would also work. Timer would only have to remember the metric instance and call .callback(…) on it.

Then again, it's only names. Slightly funny looking perhaps, but we use __init__ and __call__ all the time as well… 😉

[csmarchbanks]

I definitely think passing the metric instance is nicer, and will work nicely with types hints as they are added. Perhaps it should be called with return Timer(self, self.observe), that way it is clear we are passing in both a metric and a callback?

[witsch]

I definitely think passing the metric instance is nicer, and will work nicely with types hints as they are added. Perhaps it should be called with return Timer(self, self.observe), that way it is clear we are passing in both a metric and a callback?

Agreed, but I think passing self.observe instead of the callback name would still require us to keep Timer.labels() almost the same, except for the self._callback.__self__, of course. We'd still have to getattr(…, self._callback.__name__), though.

If that's okay with you, I can happily adjust the code tomorrow? 🙂

[csmarchbanks]

Ahh, right that makes sense. I think the callback name for now sounds good, we can always change it if it becomes a problem to maintain and the new functionality will be nice.

[witsch]

If that's okay with you, I can happily adjust the code tomorrow? 🙂

Yesterday was too "full", but I think now it's really a bit clearer. And I suppose I should configure my git to sign off by default… 😅

[csmarchbanks]

One small comment otherwise 👍.

You mentioned updating track_inprogress as well, I don't think that would make sense as we inc on __enter__ which means we would need to know any labels upfront. Am I missing something though?

[csmarchbanks]

I actually don't think this is necessary as set or observe will already call _raise_if_not_observable.

[witsch]

Good point, I was trying to "make up" for the call on the way in, essentially making sure that it's still called at all, but I didn't realize that the callbacks already do so anyway… 🙂

[witsch]

You mentioned updating track_inprogress as well, I don't think that would make sense as we inc on __enter__ which means we would need to know any labels upfront. Am I missing something though?

True. I had just spotted that (other) context manager, but not looked into the details yet. But yes, that wouldn't work, of course.

[csmarchbanks]

Thanks!