Is the godoc for MetricVec.GetMetricWithLabelValues() wrong?
notrobpike opened this issue · 8 comments
This does not feel like a user mailing list/group question so I am opening this issue.
GetMetricWithLabelValues claims that a Delete retains the metric being deleted, and it's just that it's not exported any longer. That is a straightforward statement, that I think might be wrong, but the statement is straightforward anyway. I'm more concerned that the next bit is confusing, that the metric is no longer exportable, "even if created again later".
For the latter claim, I've done a unit test where I create/delete/add a counter in a countervec, and it works as one would expect. The godoc is hinting to me that it wouldn't work -- that the final add would not export the metric.
For the former claim, Metric will still exist, I'm not sure how to test for this but if I trace through the code it appears to me that it is in fact truly deleted. Here and in an earlier commit here
A final clue I have about this is that this doc is attached to GetMetricWithLabelValues. This is just as, if not more important, for users of Delete to know that the metric is not being deleted. So if this statement were correct I'd expect it to be attached to the Delete method.
I'm reading the code again and I think you might be correct...
I'm not sure what we want to do here though, we could 1) fix the comment to reflect reality or 2) do what we currently say we do.
I don't think we want to prevent people from recreating a metric that was deleted before, but maybe we can have some performance improvement in this recreation operation if we don't delete the metric completely like we do here. Keeping it hidden from exposure might save some CPU cycles when recreating it... a benchmark would answer our questions :)
For soft-delete to be valuable, delete/create cycles probably have to be rapid.
I understand that the deletion of a big-M "Metric" here is really deletion of a time series, not the entire small-m metric -- Metric here refers to the struct, part of a MetricVec. Just want to clarify what we're talking about as these terms get confusing -- to me anyway!
I will venture a guess that for most use cases, deletion of such is somewhat rare, and rapid delete/recreate is even rarer. If you soft-delete, then you have to check the soft-delete status, or maintain a side cache that then has to be walked on any re-create. Feels annoying to maintain.
In my particular use case, I need the metric to be deleted. The entire point of my use case is that I want to release some memory for no longer useful time series (Metrics). I'm exporting a zillion time series and need to cull them periodically. Which is why I got looking into this.
So I favor calling it working as intended, and change the godoc. :) And if someone does want soft-delete, they can go ahead and ask or write that code as a separate issue.
Thanks for the detailed explanation of your use case! I agree that what you described is probably more common in reality
Hey @ArthurSens
Based on the discussion above, I believe adding an auxiliary function that can soft delete the value can be useful. If this sounds useful, may I proceed with working on this issue?
GetMetricWithLabelValues claims that a Delete retains the metric being deleted
I cannot read that from the doc comment. Maybe the wording is misleading, but at least the word "retain" doesn't show up anywhere.
I assume you are talking about this sentence:
In that case, the Metric will still exist, but it will not be exported anymore, even if a Metric with the same label values is created later.
This is merely referring to the pattern where you use GetMetricWithLabelValues to get a Metric and keep it for later use (i.e. not Delete retains the Metric, your code retains the Metric). If you Delete a Metric from a vector, it's gone from the vector for good. So your version of Metric is now not exported anymore, and while you can still use it, e.g. you could increment it if it is a Counter, it wouldn't do anything for the exposed metrics.
From how I understand the discussion, we only need to word this sentence in a less misleading way.
@beorn7 my complaint here is centered on memory consumption of "deleted" metrics.
Yes, "still exist" -> retained ... in memory (given that it can still be incremented and has a memory of the previous value). I realize now that my method of testing this ... by looking at exported metrics ... was faulty, since that is already documented as not being done.
I have these very high cardinality metrics that I purge periodically by deleting label values. This reduces total cardinality at the prom server, but also I've been doing this for months and months and my application memory stays constrained by using Delete. So I am not sure that even the claim that it is kept but not exported is correct, from my observation. I will have to look at the code to verify that. But I can see in my heap size metrics that when my periodic trigger to do a Delete run occurs, the app heap size decreases.
If you delete a metric from a vec, and you don't keep a reference to the metric, it will be garbage collected.
As said, the sentence in question is referring to the case where you keep a reference to the metric around for "direct usage".
Thanks I get it now. On a new reading with that guidance, I can see now how it works, and also why the mention of Delete is part of Get.... As an interface, I think it's not a given that the Metric will still exist after a Delete. I'll think about how the godoc might be updated for clarity.