Reset metric name lookup table when another worker deletes a metric #171
Conversation
This file contains 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
Make it easier to define multiple tests and run them concurrently.
This test verifies that: - a gauge metric that has been reset is no longer reported; - a previously-reset metric that is used again correctly reports its values.
The metric key index now accepts a function that gets called every time a key is deleted from the index, which happens at the next sync after another worker had deleted a key. The callback function finds the same metric in the local worker's registry and cleans up its lookup table. To make sure this happens deterministically for all workers, the key index sync is now also scheduled to run every `sync_interval` (1s by default). The goals are: - to prevent unbounded growth of the per-metric lookup tables; - to ensure that previously reset metrics can be used again.
Size based cleanup was added to avoid unbounded growth of lookup tables as metrics get deleted/reset. This should not be necessary now when lookup tables are being reset on metric deletion. This reverts eb1876d
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.
A few commits here that are easier to review individually, adding a test to reproduce #168 and fixing the issue by emtying the per-metric lookup table every time a metric is deleted by one of the other nginx workers. (cc @Ashion who reported this)
Since we already have a way to propagate metric key deletion to other workers via KeyIndex, I am piggy-backing on the same mechanism to trigger lookup table cleanup. (cc @dolik-rce who implemented the KeyIndex)
This also addresses the issue of unbounded memory growth when there is metric churn (#151), so I am removing the static lookup table size limit that was introduced as a mitigation. (cc @kingluo who reported that memory leak issue)
I will wait for a few days before merging this just in case someone has any feedback.
Fixes #168