-
Notifications
You must be signed in to change notification settings - Fork 569
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
Alertmanager: Initialize skipped Grafana Alertmanagers receiving requests #10691
Open
santihernandezc
wants to merge
22
commits into
main
Choose a base branch
from
santihernandezc/initialize_skipped_grafana_alertmanagers_receiving_requests
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
e63e411
(WIP) Alertmanager: Initialize skipped Grafana Alertmanagers receivin…
santihernandezc 8bbf5de
remove unnecessary lines, refactor,
santihernandezc eefbcbb
use sync.Map instead of map + mutex
santihernandezc 7883412
add gauge for number of Alertmanagers skipped during the last config …
santihernandezc 40572a2
make doc, make reference-help
santihernandezc d7bd126
reduce the amount of store operations by only storing a zero-value ti…
santihernandezc 395cf74
remove unnecessary zeroTimeUnix var
santihernandezc 7699941
wording in logs
santihernandezc e23a766
Merge branch 'main' of https://github.com/grafana/mimir into santiher…
santihernandezc 2201e08
use LoadOrStore()
santihernandezc 83d8c88
receivingRequests -> lastRequestTime
santihernandezc 67cafb6
fix receiving alerts -> receiving requests
santihernandezc 51524d0
Merge branch 'main' of https://github.com/grafana/mimir into santiher…
santihernandezc 79c89f8
improve redability in computeConfig()
santihernandezc 373336b
Add counter for on-request initializations
santihernandezc 45246b6
fix custom mimir config being ignored in grafana tenants, tests
santihernandezc 39da214
fix order of expects in tests
santihernandezc 5ba975f
make test diff smaller
santihernandezc 01303a9
Merge branch 'main' of https://github.com/grafana/mimir into santiher…
santihernandezc c688474
handle errNotUploadingFallback errors
santihernandezc 8e88697
delete tenant from skipped list if it's not owned by the instance, al…
santihernandezc c3eb098
prevent race conditions when starting Alertmanagers, refactor
santihernandezc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concern: This method is doing a lot and appears to have code that doesn't match our current
-grafana
tenant strategy. For example, it's not clear to me what the following is meant to be doing anymore and why we don't need it in the newstartAlertmanager
code:If it is in fact, not needed in
startAlertmanager
we might want to start cleaning up some of the redundant code that doesn't fit the current strategy. Or at least extract it somewhere where it is clear it's not part of the functional flow.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That bit of code gets executed after we've checked for usable (promoted, non-default, non-empty) Grafana configuration. If we reach this far, the tenant has a Grafana configuration we can use to start their Alertmanager.
It does indeed not match our current approach. It's part of the original one, where Grafana and non-Grafana tenants were the same, and they couldn't be distinguished by a suffix.
It's not necessary to add
computeConfig()
to thestartAlertmanager()
function as it will only be called for skipped Grafana Alertmanagers. If a Grafana Alertmanager was skipped, it had no usable configuration, so we can just use a default config.