-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(alertmanager): add alertmanager package #6860
base: main
Are you sure you want to change the base?
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
👍 Looks good to me! Reviewed everything up to 0688b72 in 2 minutes and 32 seconds
More details
- Looked at
2583
lines of code in22
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. pkg/alertmanager/alertmanagerserver/server.go:173
- Draft comment:
Check the error returned byNewAlertsFromPostableAlerts
before proceeding to put alerts. This ensures that any conversion errors are handled appropriately. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code appears to be deliberately structured to prioritize sending notifications over validation errors. The comment on line 175 explicitly states this is intentional behavior. The suggestion to check errors first would actually go against this design choice. The current implementation matches the stated intent.
I could be wrong if there are serious validation errors that should never be ignored, or if sending invalid alerts could cause system issues.
The code comment clearly documents this as an intentional design choice to prioritize notification sending. If this was dangerous, it likely wouldn't be explicitly documented as intended behavior.
The comment should be deleted as it suggests changing intentionally designed behavior that is already documented in the code comments.
2. pkg/alertmanager/alertmanagertypes/config.go:76
- Draft comment:
Validate the config after unmarshalling to ensure it is correct and complete. This can prevent issues with invalid configurations being used. - Reason this comment was not posted:
Comment did not seem useful.
3. pkg/alertmanager/alertmanagertypes/config.go:158
- Draft comment:
Handle the case where the receiver or route to be deleted does not exist. This can prevent unexpected behavior when attempting to delete non-existent entries. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment points out a legitimate issue - the Delete method doesn't indicate whether the deletion was successful or if the item didn't exist. However, this could be an intentional design choice to make deletion idempotent. Without more context about the intended behavior or system requirements, we can't be certain that this is actually a problem that needs fixing.
I might be overvaluing the importance of explicit error handling. Silent failure could be the desired behavior for idempotent operations.
While idempotency is valuable, providing feedback about operation success can help with debugging and user experience. The comment raises a valid point worth considering.
The comment raises a valid concern about error handling, but without more context about the intended behavior, we can't be certain it's an issue that needs fixing.
4. pkg/alertmanager/alertmanagerstore/memoryalertmanagerstore/provider.go:19
- Draft comment:
Consider adding parameters to theNew
function to allow setting initial values forSilences
,NFLog
, andConfig
. This can be useful if these need to be pre-populated. - Reason this comment was not posted:
Confidence changes required:50%
TheNew
function inprovider.go
initializes a provider with default values but does not provide any mechanism to set initial values forSilences
,NFLog
, orConfig
. This could lead to issues if these need to be pre-populated.
5. pkg/alertmanager/alertmanagerserver/server.go:1
- Draft comment:
Avoid using thecomponent/index.tsx
file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_jKduOyH0wG9QMlCa
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Summary
Add a dedicated package for alertmanager
Related Issues / PR's
Closes https://github.com/SigNoz/platform-pod/issues/404
Important
Introduces a new alert management package with server setup, configuration, and state management using Prometheus Alertmanager.
Server
inserver.go
to manage alerts, silences, and notification logs using Prometheus Alertmanager.Start
,Stop
,SetConfig
, andPutAlerts
methods for server lifecycle and alert management.Config
,RouteConfig
,SMTPConfig
,AlertsConfig
,SilencesConfig
, andNFLogConfig
inconfig.go
for alertmanager settings.memoryalertmanagerstore
for in-memory storage of silences and notification logs.State
,Config
,Alert
, andReceiver
types inalertmanagertypes
for alert and config handling.errors.go
andcode.go
.server_test.go
.render_test.go
.This description was created by for 0688b72. It will automatically update as commits are pushed.