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.

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

Is the godoc for MetricVec.GetMetricWithLabelValues() wrong? #1470

Open
notrobpike opened this issue Mar 14, 2024 · 3 comments
Open

Is the godoc for MetricVec.GetMetricWithLabelValues() wrong? #1470

notrobpike opened this issue Mar 14, 2024 · 3 comments

Comments

@notrobpike
Copy link

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.

@ArthurSens
Copy link
Member

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 :)

@notrobpike
Copy link
Author

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.

@ArthurSens
Copy link
Member

Thanks for the detailed explanation of your use case! I agree that what you described is probably more common in reality

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants