Allow returning stale data from tenants cache #665
Merged
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
In the case where an entry in the cache was expired, if the data could not be refreshed by requesting it to the API, an error was returned. This implied a blocking behavior for various actions, such as: - Check executions: If limits could not be verified. - Publish events: If tenant remotes could not be obtained. These actions should continue to run even if the agents lose connection with the API, or the API is behaving abnormally. Therefore, this change favours returning stale tenants data from the cache in case it can not be refreshed by requesting it to the API. The worst case scenario for each mentioned case after this change is: - Check execution: The check will be executed with limits that might have changed. - Publish: The metrics and logs will be pushed using remotes that might have changed. Both cases can lead to metrics and logs backends to reject our requests. But these changes happen infrequently, so most often the data returned by the cache will still be valid and checks will be able to run even if the agent is disconnected from the API.
The-9880
reviewed
Mar 26, 2024
Comment on lines
+151
to
+152
| // wait for tenants to expire | ||
| time.Sleep(cacheExpirationTime) |
There was a problem hiding this comment.
Just a side thought: we could maybe use something like a TestClock to test cache expirations in general - instead of calling time.Sleep we would do something like testClock.Advance(200 ms) and it would validate that these expirations work without having to pause execution.
Then you don't have to consider reducing the timeout from 500ms -> 200ms in tests as well 😄
There was a problem hiding this comment.
You are absolutely right that sleeping in a test is not ideal, but mocking time is not so straightforward, as we would have to define an interface for it, have two different implementations, one for std and one for our "test time", and have an instance for it in the cache, so it gets kind of cumbersome. For other cases where it's required, such as when using a ticker, I usually do this, but for this case sleeping some time seems reasonable 😕
mem
added a commit
that referenced
this pull request
Apr 8, 2024
* dependabot: group prometheus updates (#664) * Chore(deps): Bump the prometheus-go group with 2 updates (#668) * Allow returning stale data from tenants cache (#665) * Chore(deps): Bump the prometheus-go group with 2 updates (#669) * Chore(deps): Bump golang.org/x/net from 0.22.0 to 0.23.0 (#670) * Chore(deps): Bump golang.org/x/net from 0.23.0 to 0.24.0 (#672) * Chore(deps): Bump golang.org/x/sync from 0.6.0 to 0.7.0 (#673) * Update grafana-build-tools to v0.10.0 (#676) Signed-off-by: Marcelo E. Magallon <[email protected]>
Merged
mem
added a commit
that referenced
this pull request
Apr 8, 2024
* dependabot: group prometheus updates (#664) * Chore(deps): Bump the prometheus-go group with 2 updates (#668) * Allow returning stale data from tenants cache (#665) * Chore(deps): Bump the prometheus-go group with 2 updates (#669) * Chore(deps): Bump golang.org/x/net from 0.22.0 to 0.23.0 (#670) * Chore(deps): Bump golang.org/x/net from 0.23.0 to 0.24.0 (#672) * Chore(deps): Bump golang.org/x/sync from 0.6.0 to 0.7.0 (#673) * Update grafana-build-tools to v0.10.0 (#676) Signed-off-by: Marcelo E. Magallon <[email protected]>
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.
In the case where an entry in the cache was expired, if the data could not be refreshed by requesting it to the API, an error was returned. This implied a blocking behavior for various actions, such as:
These actions should continue to run even if the agents lose connection with the API, or the API is behaving abnormally. Therefore, this change favours returning stale tenants data from the cache in case it can not be refreshed by requesting it to the API.
The worst case scenario for each mentioned case after this change is:
Both cases can lead to metrics and logs backends to reject our requests. But these changes happen infrequently, so most often the data returned by the cache will still be valid and checks will be able to run even if the agent is disconnected from the API. And even in the case on which these errors happen, is better than blocking preventively.