RFC: support clearing of metric objects for custom collectors#1175
RFC: support clearing of metric objects for custom collectors#1175calestyo wants to merge 1 commit into
Conversation
When not doing “direct instrumentation” but rather collecting metrics and thus using the `*MetricFamily`-classes, the general approach seems to be: • Defining a custom collector (that is: a class which is derived from `prometheus_client.registry.Collector`) and at least a `collect`-method. • Create instances of `*MetricFamily` within `collect`. • Add metrics to these via their `add_metric`-method. • Either `return` or `yield` those, for use by the collector registry. The last two steps will be done for every invocation of `collect`. In many cases however, the `*MetricFamily`-objects themselves (that is: their name, documentation, labels, et cetera – basically all the values set via their respective `__init__`-method) except for any metrics added to them will always stay the same and be needlessly re-defined/created on every call to `collect`. This adds `clear`-method to their base class `prometheus_client.metrics_core.Metric`, which simply clears the instance’s `samples`-attribute, so that the `*MetricFamily`-objects that are static in the above sense, can be defined/created for example in `__init__`, and `collect` would merely need to call `clear` for every metric. That could be done either in the beginning of `clear` or perhaps right after `yield`ing the respective object(s) (which would allow Python to reclaim the memory faster, but might perhaps lead to the clearing not happening if an exception is raised. So perhaps the best practise would to do both, clear all in the beginning of `collect` and – for exporters who have so many metrics that this might be an issue – right after `yield`ing. Another idea was, that the clearing could actually be done by the registry after it has collected the metrics, but this seems to rather just limit the freedom and would possibly also not be backwards compatible. This change of course still allows for the “old” usage (that is: re-defining/ creating the `*MetricFamily`-objects in `collect` on every call. Last but not least, currently, `samples` is cleared via the `list.clear`-method rather than simply assigning a new empty list to it. The former is probably considerably slower (if there are many elements in the list), but chosen for now, as `samples` is (per naming conventions) “public” and thus references to it might be held by users. Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
|
btw: I think we could perhaps even tell people that if they would want to use the "new" way (i.e. define metrics in And perhaps one more rationale point: |
|
Oh and one more point to decide upon: How should the function be named? I went for |
|
Agree on the memory point, the cost is the samples (the label-value data), not the |
2 participants
Hey.
This is based on my second point in #1173 (comment).
Please see there and also the commit message which explains the rationale.
I think the biggest open point right now would be whether to use
self.samples.clear()or ratherself.samples = [], which would probably be way faster (see commit message for the reasoning).If
.sampleswas actually be considered non-public, it should however be renamed (and I think the same goes for e.g.add_sample, whose doc-string already says it would be internal only... so just renamed it to_add_sample, or maybe warp around it and add a deprecation warning to the public one.Cheers,
Chris.
PS: @csmarchbanks and @k1chik might want to have a look. For the latter, if this PR would be considered worthy to add... it would probably make sense to present that way of use as an alternative to the current schema in the docs (which you've so greatly worked on recently)