-
Notifications
You must be signed in to change notification settings - Fork 1.6k
chore: added custom dispatcher for custom routing rules #8686
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
Open
aniketio-ctrl
wants to merge
1
commit into
main
Choose a base branch
from
chore/custom_dispatcher
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.
Conversation
This file contains hidden or 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
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.
Important
Looks good to me! 👍
Reviewed everything up to 6d5a767 in 1 minute and 36 seconds. Click for details.
- Reviewed
617
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
7
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. pkg/alertmanager/alertmanagerserver/server.go:55
- Draft comment:
Dispatcher field now uses signozDispatcher.Dispatcher. Ensure this change is consistent across the module and that any upstream dependencies expecting the original type are updated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure consistency and check dependencies, which violates the rule against asking the author to confirm or ensure things. It doesn't provide a specific suggestion or point out a specific issue.
2. pkg/alertmanager/alertmanagerserver/server.go:191
- Draft comment:
Using signozDispatcher.NewDispatcherMetrics with 'false' for registering limit metrics means the 'aggrGroupLimitReached' counter won’t be registered. Please confirm this is intentional. - Reason this comment was not posted:
Comment was on unchanged code.
3. pkg/alertmanager/alertmanagerserver/server.go:296
- Draft comment:
The custom dispatcher is instantiated via signozDispatcher.NewDispatcher with several parameters (e.g., timeout function, marker). Verify that these parameters meet the expectations for the custom routing rules implementation. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to verify that the parameters meet expectations, which falls under the rule of not asking the author to confirm or ensure behavior. It doesn't provide a specific suggestion or point out a specific issue, making it not useful according to the guidelines.
4. pkg/alertmanager/dispatch/dispatcher.go:439
- Draft comment:
When setting up the aggregation group logger with logger.With("aggrGroup", ag), consider adding a more descriptive identifier (like the group key) instead of the raw pointer value to aid debugging. - Reason this comment was not posted:
Comment looked like it was already resolved.
5. pkg/alertmanager/dispatch/dispatcher.go:358
- Draft comment:
In processAlert, when the aggregation group limit is reached the alert is dropped and an error is logged. Confirm that dropping alerts in this scenario is acceptable or if a fallback mechanism is needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to confirm if dropping alerts when the aggregation group limit is reached is acceptable. This falls under asking the author to confirm their intention, which is against the rules. Therefore, this comment should be removed.
6. pkg/alertmanager/dispatch/dispatcher.go:293
- Draft comment:
The Stop() method waits on the d.done channel. Ensure that the dispatcher’s Run loop always terminates to avoid potential blocking in Stop(). - Reason this comment was not posted:
Comment looked like it was already resolved.
7. pkg/alertmanager/dispatch/dispatcher.go:544
- Draft comment:
Typo: In the comment "Ensure that alerts don't resolve as time move forwards.", consider rephrasing it to "Ensure that alerts don't resolve as time moves forward." - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The rules state not to make purely informative comments and to only comment if there is clearly a code change required. While this is technically about changed code (since this is a new file), fixing grammar in comments is not a functional issue and doesn't affect the code's behavior. The rules emphasize focusing on actual code issues rather than documentation/style nitpicks. The grammar error could potentially cause confusion for future developers reading the code. Documentation clarity is important for maintainability. While clear documentation is valuable, this particular grammar error is minor and doesn't significantly impact code understanding. The meaning is still clear despite the grammatical error. Delete this comment as it's purely about documentation style and doesn't affect code functionality or quality.
Workflow ID: wflow_hXhiMH9QIT9o8JnT
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Added custom dispatcher to test on staging environment
Important
Introduces a custom dispatcher for SigNoz alert manager with specific routing rules and metrics.
signozDispatcher
inserver.go
to replace Prometheusdispatch.Dispatcher
.dispatcher.go
for SigNoz, based on Prometheus Alertmanager.DispatcherMetrics
indispatcher.go
for custom metrics likeaggrGroups
,processingDuration
, andaggrGroupLimitReached
.dispatcher.go
withprocessAlert()
andrun()
methods.unlimitedLimits
.New()
andSetConfig()
inserver.go
to initialize and configure the custom dispatcher.This description was created by
for 6d5a767. You can customize this summary. It will automatically update as commits are pushed.