Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

statMetricsActive is incorrect because we prune recursively. #797

Closed
Dieterbe opened this issue Dec 21, 2017 · 0 comments · Fixed by #799
Closed

statMetricsActive is incorrect because we prune recursively. #797

Dieterbe opened this issue Dec 21, 2017 · 0 comments · Fixed by #799

Comments

@Dieterbe
Copy link
Contributor

Dieterbe commented Dec 21, 2017

see #787 more specifically #787 (review)

copying here because I don't trust GH enough to retain the comments long after the fact.

in our implementation, a path can be both a leaf and have children.
e.g. foo.bar can have data/metricdef (making it a leaf), while foo.bar.baz also exists (making foo.bar a branch)
In graphite this doesn't work, but some of our customers wanted this behavior, and it seemed fairly easy to support, so that's why we do.

the m.delete call is recursive: it deletes the path, and any sub-paths (I think i actually just discovered a related bug here), so what we should do is track the actual amount of paths that got deleted by the delete call, or I think better: the pruning process shouldn't recursively delete, but rather only delete leaves (and clean up stale branch nodes that no longer have children nor defs)

woodsaj pushed a commit that referenced this issue Dec 21, 2017
-  this issue was discovered in discussions of issue #714
-  this also fixes #797
woodsaj pushed a commit that referenced this issue Dec 21, 2017
-  this issue was discovered in discussions of issue #714
-  this also fixes #797
woodsaj pushed a commit that referenced this issue Dec 21, 2017
-  this issue was discovered in discussions of issue #714
-  this also fixes #797
woodsaj pushed a commit that referenced this issue Dec 21, 2017
-  this issue was discovered in discussions of issue #714
-  this also fixes #797
Aergonus pushed a commit to bloomberg/metrictank that referenced this issue Jan 19, 2018
-  this issue was discovered in discussions of issue grafana#714
-  this also fixes grafana#797
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant