{{ message }}
RFC: support clearing of metric objects for custom collectors#1175
Open
calestyo wants to merge 1 commit into
Open
RFC: support clearing of metric objects for custom collectors#1175calestyo wants to merge 1 commit into
calestyo 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>
Author
Author
|
Oh and one more point to decide upon: How should the function be named? I went for |
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

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)