feat(alertmanager): integrate with ruler #7222
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
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to b6be22c in 2 minutes and 46 seconds
More details
- Looked at
1415lines of code in22files - Skipped
0files when reviewing. - Skipped posting
28drafted comments based on config settings.
1. pkg/query-service/rules/manager.go:360
- Draft comment:
Ensure tx.Commit() is called only after all dependent operations succeed. Consider adding comments that explain why a rollback is deferred and commit is called after alertmanager config updates. - 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 comment raises a valid point about transaction ordering - committing before dependent operations could leave the system in an inconsistent state if the alertmanager updates fail. However, the comment's phrasing as "Ensure that..." makes it speculative rather than definitive. The suggestion to add comments is not a critical code change.
I may be undervaluing the importance of transaction ordering. An inconsistent state between the DB and alertmanager could cause serious issues.
While transaction ordering is important, the comment's phrasing makes it speculative rather than pointing out a definite issue. Per the rules, we should avoid "Ensure that..." type comments.
The comment should be deleted because it is phrased as a speculative "Ensure that..." suggestion rather than pointing out a definite issue, even though the underlying concern may be valid.
2. pkg/query-service/rules/manager.go:391
- Draft comment:
After editing the rule, verify that the in-memory task state is in sync with the new rule – syncing with alertmanager configuration is critical. Ensure error handling in syncRuleStateWithTask covers all edge cases. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. pkg/query-service/rules/manager.go:517
- Draft comment:
When deleting tasks, be cautious: ensure that removal from both tasks and rules maps is thread-safe. Use comments to clarify why both maps are updated. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. pkg/sqlmigration/013_add_alertmanager.go:37
- Draft comment:
Ensure that the deferred tx.Rollback() is acceptable even if tx.Commit() is later called (based on bun docs). It might be clearer to explicitly nil out the tx after successful commit to avoid accidental rollback calls. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This is a common SQL transaction pattern. The comment starts with "Ensure that..." which is a red flag. The behavior of Rollback after Commit is well-documented in SQL drivers and bun. The suggestion to nil out tx wouldn't help since the deferred call is already registered. This seems like unnecessary speculation about a standard pattern.
The comment does raise a valid concern about transaction safety. Maybe there are edge cases where this pattern could cause issues?
This is an extremely common pattern in Go database code. If there were issues with deferred rollback after commit, it would be well-known and documented. The bun library specifically supports this pattern.
Delete this comment. It's speculative, starts with "Ensure that...", and questions a standard Go database transaction pattern without strong evidence of an actual issue.
5. pkg/signoz/config.go:108
- Draft comment:
Deprecated flag messages should be eventually removed; consider adding references to update your environment variable names in the documentation. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. pkg/signoz/provider.go:79
- Draft comment:
Make sure that when passing a nil sqlstore to NewAlertmanagerProviderFactories, the underlying factories handle nil appropriately to avoid panic. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to ensure that a nil sqlstore is handled appropriately, which violates the rule against asking the author to ensure behavior is intended or tested. It does not provide a specific suggestion or point out a specific issue.
7. pkg/signoz/provider_test.go:19
- Draft comment:
The test for NewAlertmanagerProviderFactories passes nil. Ensure that providers can handle a nil SQLStore, or add a comment/documentation if nil is allowed only for test scenarios. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The test's purpose is clearly to verify non-panicking behavior, as shown by assert.NotPanics and the comment on the similar test above. The comment is asking for confirmation about nil handling, which falls under the "do not ask to confirm intention" rule. It's also somewhat speculative about whether nil should be handled.
Maybe the nil handling is actually a serious issue that could cause runtime problems in production? Maybe we should keep comments about potential null pointer issues?
The test explicitly verifies that nil doesn't cause panics, and the similar test above has a comment explaining this is intentional behavior. If nil wasn't meant to be handled, the test would be written differently.
Delete the comment. It violates rules about asking for confirmation and making speculative comments. The test itself demonstrates that nil handling is intended behavior.
8. pkg/signoz/signoz.go:108
- Draft comment:
After initializing alertmanager via provider, consider documenting which provider is chosen (legacy vs. SigNoz) based on configuration for clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment suggests documenting which provider is chosen after initializing alertmanager. This is a suggestion for documentation, which is not allowed according to the rules. The comment does not provide a specific code suggestion or ask for a test to be written. Therefore, it should be removed.
9. pkg/signoz/config.go:109
- Draft comment:
For deprecation warnings, consider using a logger (e.g. zap) instead of fmt.Println to maintain consistent logging and to enable log redirection. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. pkg/signoz/provider_test.go:20
- Draft comment:
TestNewAlertmanagerProviderFactories passes nil to NewAlertmanagerProviderFactories. Verify that the downstream alertmanager factories safely handle a nil SQLStore in production. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify that a nil SQLStore is safely handled in production, which violates the rule against asking the author to ensure behavior is intended or tested. It does not provide a specific code suggestion or ask for a specific test to be written.
11. pkg/query-service/rules/manager.go:360
- Draft comment:
The pattern of deferring tx.Rollback() before committing in EditRule() is good; ensure that it does not obscure errors in case tx.Commit() fails. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that a specific behavior does not obscure errors. This falls under the rule of not asking the author to ensure behavior is intended or tested. Therefore, this comment should be removed.
12. pkg/query-service/rules/test_notification.go:55
- Draft comment:
The TestNotification function effectively sets up a test alert rule. Consider adding assertions to verify the contents of the test alert (e.g. checking the generated alert name or message) to strengthen this test. - Reason this comment was not posted:
Comment was on unchanged code.
13. pkg/signoz/provider.go:79
- Draft comment:
The NewAlertmanagerProviderFactories function constructs factories using the provided SQLStore. Ensure that in production a valid, non-nil SQLStore is always passed to avoid potential nil pointer dereferences. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to ensure that a valid, non-nilSQLStoreis passed in production to avoid nil pointer dereferences. This falls under the category of asking the author to ensure behavior, which is against the rules. Therefore, this comment should be removed.
14. ee/query-service/rules/manager.go:40
- Draft comment:
It looks like there's a typographical error in the comment at line 40; 'evalution' should be corrected to 'evaluation' to improve clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. ee/query-service/rules/manager.go:61
- Draft comment:
There's a typographical error in the comment at line 61; please change 'evalution' to 'evaluation'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
16. ee/query-service/rules/manager.go:81
- Draft comment:
Please correct the typographical error in the comment at line 81: 'evalution' to 'evaluation'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
17. pkg/query-service/app/http_handler.go:5240
- Draft comment:
Typographical error: The variable name 'errQuriesByName' appears in several locations. Consider renaming it to 'errQueriesByName' for consistency and clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
18. pkg/query-service/app/http_handler.go:4449
- Draft comment:
Typographical error: The variable 'lastestConfig' should likely be renamed to 'latestConfig' to correctly reflect its meaning. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
19. pkg/query-service/rules/api_params.go:42
- Draft comment:
Consider updating the comment on line 42 from "this file contains api request and responses to be" to "this file contains API requests and responses to be served over HTTP" for clarity and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
20. pkg/query-service/rules/api_params.go:304
- Draft comment:
It appears there might be typos in the JSON field tags of GettableRule: "createAt", "createBy", "updateAt", and "updateBy". Consider using "createdAt", "createdBy", "updatedAt", and "updatedBy" for consistency and clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
21. pkg/query-service/rules/base_rule.go:69
- Draft comment:
Typo: In the comment for sendUnmatched, 'dont' should be written as "don't". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
22. pkg/query-service/rules/base_rule.go:73
- Draft comment:
Typo: In the comment for sendAlways, 'irresepective' should be corrected to 'irrespective'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
23. pkg/query-service/rules/db.go:361
- Draft comment:
Typo: In the SQL query (line 361) for GetChannel, the column 'data' is repeated as 'data data'. This might be unintentional and should be corrected. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
24. pkg/query-service/rules/db.go:398
- Draft comment:
Typographical error: In the DeleteChannel function (line 398), the error message incorrectly refers to 'INSERT to notification_channels' instead of 'DELETE from notification_channels'. Please update the message to accurately reflect the SQL operation. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
25. pkg/query-service/rules/db.go:493
- Draft comment:
Typographical error: In the EditChannel function (line 493), the error message states 'Error in committing transaction for INSERT to notification_channels' which is confusing since the operation is an UPDATE. Please change 'INSERT' to 'UPDATE' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
26. pkg/query-service/rules/test_notification.go:33
- Draft comment:
Consider reviewing the spelling of 'TestAlertPostFix' in the fmt.Sprintf call. Typically, 'postfix' is written with a lowercase 'f' (i.e., 'TestAlertPostfix') for consistency and clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
27. pkg/query-service/rules/test_notification.go:82
- Draft comment:
Consider correcting the error message string 'failed to derive ruletype with given information' to 'failed to derive rule type with given information' for improved readability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
28. pkg/signoz/provider.go:13
- Draft comment:
The import path 'go.signoz.io/signoz/pkg/sqlstore/postgressqlstore' looks like it may contain a typographical error. Consider verifying if 'postgressqlstore' should be 'postgresqlstore' (with only one 's' after 'postgre'). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_BZxNFAubeE6vCf8L
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5bd60d3 in 2 minutes and 30 seconds
More details
- Looked at
1679lines of code in8files - Skipped
0files when reviewing. - Skipped posting
12drafted comments based on config settings.
1. pkg/query-service/rules/db.go:164
- Draft comment:
Avoid constructing SQL with fmt.Sprintf; even though id is parsed as an integer, it's safer to use parameterized queries. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. pkg/query-service/rules/manager.go:66
- Draft comment:
Consider defining a constant for the '-groupname' suffix to avoid magic strings. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. pkg/query-service/rules/manager.go:855
- Draft comment:
In PatchRule, ensure that syncing rule state uses the latest patched rule instead of restoring the old stored state if that is the intended behavior. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. pkg/query-service/rules/manager.go:568
- Draft comment:
The addTask function returns an error if a task with the same name exists, but consider whether tasks should be uniquely identified by rule ID or if additional disambiguation is needed. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
5. pkg/query-service/rules/manager.go:871
- Draft comment:
In PatchRule, the callback passed to EditRule calls m.syncRuleStateWithTask with the original stored rule (&storedRule) instead of the updated patched rule. This may revert the intended patch changes. Consider syncing with the updated rule. - 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 comment misunderstands the purpose of this callback. This is an error handler that runs if EditRule fails, to restore the previous state - which is exactly what we want. We want to revert back to the original stored rule if the edit fails. Using the original stored rule here is correct. The comment's suggestion to use the patched rule would actually be wrong, as it would apply the failed changes.
Could there be cases where we want to keep the patched changes even if EditRule fails? Maybe the error is unrelated to the rule changes themselves?
No, if EditRule fails we should revert all changes to maintain consistency. The error could be from DB constraints or validation that we want to respect. A partial update would leave things in an inconsistent state.
The comment should be deleted. The code correctly uses the original stored rule to restore state after a failed edit. This is proper error handling behavior.
6. pkg/query-service/rules/db.go:164
- Draft comment:
GetStoredRule uses fmt.Sprintf to inject the ID into the SQL query. Although the ID is converted to an integer, it is best practice to use parameterized queries to mitigate any potential injection risks. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. pkg/query-service/rules/db.go:263
- Draft comment:
The SQL query in getChannels uses an alias 'data data'. Verify if this alias is intentional or if it is a typo. - Reason this comment was not posted:
Comment was on unchanged code.
8. pkg/query-service/rules/manager.go:388
- Draft comment:
Several parts of the manager code use zap.L() directly. For consistency and easier testing/configuration, consider using the injected logger (m.logger) throughout. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. pkg/query-service/rules/api_params.go:300
- Draft comment:
The JSON tags for the fields in GettableRule (CreatedAt, CreatedBy, UpdatedAt, and UpdatedBy) use keys 'createAt', 'createBy', 'updateAt', and 'updateBy', which might be typographical errors. Typically, these should be 'createdAt', 'createdBy', 'updatedAt', and 'updatedBy' to maintain consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. pkg/query-service/rules/db.go:23
- Draft comment:
The documentation in the RuleDB interface is inconsistent with the updated method signatures. For example, the comment for CreateRule mentions returning 'tx and group name' but the implementation now returns a task name (int64) and error. Similar mismatches exist for EditRule and DeleteRule. Please update the comments to accurately reflect the current implementation. - Reason this comment was not posted:
Comment looked like it was already resolved.
11. pkg/query-service/rules/manager.go:172
- Draft comment:
Typo: The comment 'create ch rule task for evalution' should read 'create ch rule task for evaluation'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. pkg/query-service/rules/manager.go:193
- Draft comment:
Typo: The comment 'create promql rule task for evalution' should be corrected to 'create promql rule task for evaluation'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_WVgQ2SHxxQCqxWwC
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 8cc15c7 in 1 minute and 57 seconds
More details
- Looked at
136lines of code in3files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. pkg/query-service/rules/manager.go:345
- Draft comment:
Avoid duplicating the logic to fetch preferred channels when ParsedRule.PreferredChannels is empty; consider extracting this into a helper function for reusability. - Reason this comment was not posted:
Marked as duplicate.
2. pkg/query-service/rules/manager.go:743
- Draft comment:
Consider extracting the duplicate channel fetching logic in prepareTestNotifyFunc into a common helper for consistency. - Reason this comment was not posted:
Marked as duplicate.
3. pkg/query-service/rules/manager.go:345
- Draft comment:
Duplicate logic for populating preferredChannels is repeated in EditRule, CreateRule, and prepareTestNotifyFunc. Consider extracting this into a helper function to avoid duplication. - Reason this comment was not posted:
Marked as duplicate.
4. pkg/query-service/rules/manager.go:368
- Draft comment:
The PR description mentions fixing the usage explorer graph x-axis scaling, but the changes here focus on alertmanager integration. Please verify that the intended UI/graph changes are included. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. pkg/types/alertmanagertypes/matcher.go:79
- Draft comment:
Using matcher.Matches(ruleID) in matcherContainsRuleID may introduce subtle differences compared to the previous literal-comparison (splitting the matcher value). Ensure that ruleID values are properly escaped or that exact matching is desired. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment raises a valid point about behavior change, but several things make me lean towards removing it: 1) The code consistently uses labels.NewMatcher throughout to create matchers, so the format should be consistent 2) The Matches() method is the intended way to use these matchers 3) The comment is speculative ("may introduce") rather than pointing out a definite issue 4) It asks for verification rather than making a clear suggestion.
I could be underestimating the risk of different matching behavior. The matcher's regexp implementation might handle edge cases differently than string splitting.
While there could be edge cases, the code consistently uses the official matcher APIs throughout, and asking for verification without specific examples isn't actionable.
Delete the comment as it's speculative, asks for verification rather than making concrete suggestions, and the code is consistently using the intended matcher APIs.
6. pkg/types/alertmanagertypes/matcher.go:47
- Draft comment:
The use of slices.Replace for updating the matcher in addRuleIDToRoute is correct, but it may be less immediately clear than a direct assignment. Consider adding a comment to explain this choice for future maintainers. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. pkg/query-service/rules/manager.go:173
- Draft comment:
Typo detected in the comment: 'create ch rule task for evalution'. Please change 'evalution' to 'evaluation'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. pkg/query-service/rules/manager.go:194
- Draft comment:
Typo detected in the comment: 'create promql rule task for evalution'. Please change 'evalution' to 'evaluation'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_DILzeFVYB2DLmpcN
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 6965f42 in 1 minute and 40 seconds
More details
- Looked at
207lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. pkg/types/alertmanagertypes/matcher_test.go:23
- Draft comment:
Consider extracting the common logic that marshals the route and extracts matchers (lines 63–70 and 127–134) into a helper to reduce duplication and improve readability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. pkg/types/alertmanagertypes/matcher_test.go:148
- Draft comment:
Test cases for matcherContainsRuleID look comprehensive. Ensure edge cases are fully covered if the underlying regexp formulation changes in future updates. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. pkg/types/alertmanagertypes/matcher_test.go:16
- Draft comment:
Consider using a []string instead of []any for expectedMatchers to improve type safety, since matcher values are always strings. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. pkg/types/alertmanagertypes/matcher_test.go:63
- Draft comment:
Using gjson to extract the 'matchers' field works, but consider unmarshalling the JSON into a struct for improved clarity and maintainability if the JSON schema is stable. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. pkg/types/alertmanagertypes/matcher_test.go:148
- Draft comment:
TestMatcherContainsRuleID is thorough. Consider adding additional edge-case tests—such as scenarios with an empty matcher set and/or an empty ruleID—to further ensure robust behavior. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
Workflow ID: wflow_WVS4cMWHfyT8D0b9
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
5 tasks
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2298e9c in 2 minutes and 2 seconds
More details
- Looked at
100lines of code in4files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. pkg/alertmanager/service.go:188
- Draft comment:
Typo fix in comment: changed 'with a the lock held' to 'with the lock held'. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. pkg/sqlmigration/014_add_alertmanager.go:182
- Draft comment:
Ensure fallback logic in populateAlertmanagerConfig is intended: if no preferred channels are specified, default receivers from all channels are appended. Consider adding a comment clarifying this behavior. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. pkg/types/alertmanagertypes/receiver.go:24
- Draft comment:
NewReceiver function: Consider adding reasoning for marshaling to YAML instead of using JSON for default population. This improves maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. pkg/types/alertmanagertypes/receiver_test.go:11
- Draft comment:
TestNewReceiver: Test case for TelegramConfig looks good. Ensure that additional test cases cover error scenarios for invalid configuration inputs to improve test coverage. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. pkg/sqlmigration/014_add_alertmanager.go:153
- Draft comment:
Consider renaming 'receiversFromChannels' to 'defaultReceivers' (or a similar descriptive name) for clarity in its purpose. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. pkg/sqlmigration/014_add_alertmanager.go:182
- Draft comment:
When no preferredChannels are specified, the code appends the default receivers from channels. Ensure that these channel names exactly match the expected alertmanager receiver identifiers. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. pkg/types/alertmanagertypes/receiver.go:24
- Draft comment:
The approach of marshaling to YAML and then unmarshaling to apply default values is clever, but may be less efficient if NewReceiver is called frequently. Consider documenting the performance impact or refactoring if needed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
8. pkg/types/alertmanagertypes/receiver_test.go:32
- Draft comment:
This test compares the JSON string output directly. Since JSON key ordering can be non-deterministic, consider comparing the parsed object structure to avoid potential flakiness. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
9. pkg/types/alertmanagertypes/receiver.go:25
- Draft comment:
Typo in the comment: consider changing 'missed' to 'missing' in the sentence 'The only default value which is missed issend_resolved(as it is a bool)...'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While the suggestion is technically correct (missing is more grammatically appropriate than missed), this is an extremely minor issue that doesn't affect code functionality or clarity. The meaning is perfectly clear either way. The rules state not to make comments that are obvious or unimportant.
The comment is technically correct and does improve the documentation quality slightly. Poor grammar in documentation could be seen as unprofessional.
While correct, this is too minor to warrant a PR comment. Documentation grammar fixes like this could be handled in a separate documentation pass or mentioned in code review comments rather than inline PR comments.
Delete this comment as it's too minor and doesn't affect code functionality or clarity.
Workflow ID: wflow_KtvvRPYrcJAav8ai
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on bc3c7e1 in 1 minute and 46 seconds
More details
- Looked at
120lines of code in4files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. pkg/types/alertmanagertypes/config.go:381
- Draft comment:
Setting 'model.NameValidationScheme = model.UTF8Validation' globally in init() may have side effects. Ensure this change is intentional and documented. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%
The comment is pointing out a potential side effect of setting a global variable, which is a valid concern. However, it asks the author to ensure the change is intentional and documented, which violates the rule against asking for confirmation or documentation. The comment could be improved by suggesting a specific action or change instead.
2. pkg/types/alertmanagertypes/route.go:17
- Draft comment:
In the non-nil branch, the route's fields are overwritten with new RouteConfig values. Confirm that overriding existing settings (including GroupByStr and durations) is intended. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment directly violates the rule "Do NOT ask the PR author to confirm their intention". The function name and implementation clearly show this is the intended behavior - it's meant to apply RouteConfig values to a Route object, whether new or existing. The comment isn't pointing out a bug or suggesting an improvement, it's just asking for confirmation.
Maybe there could be a legitimate concern about overwriting existing values without preserving them, which could cause data loss?
The function name and signature make it clear this is a configuration application function - overwriting values is its explicit purpose. If preserving old values was needed, it would be handled at the caller level.
Delete the comment as it violates the rule about asking for confirmation of intended behavior and doesn't identify any actual issues.
3. pkg/types/alertmanagertypes/receiver_test.go:20
- Draft comment:
Test JSON inputs are tightly coupled to formatting. Consider comparing unmarshaled objects instead of raw JSON strings to improve robustness. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. pkg/types/alertmanagertypes/config.go:381
- Draft comment:
Setting model.NameValidationScheme to UTF8Validation improves label name validation. Please confirm that this change aligns with legacy behavior expectations. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to confirm their intention, which violates the rule against asking for confirmation of intentions. It does not provide a specific code suggestion or highlight a potential issue with the code itself.
5. pkg/types/alertmanagertypes/config.go:87
- Draft comment:
Using mergo.Merge() to overlay GlobalConfig defaults may override provided fields unintentionally. Please review the merge strategy to ensure it meets requirements. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. pkg/types/alertmanagertypes/receiver_test.go:19
- Draft comment:
The hardcoded expected JSON in TestNewReceiver may be brittle against future default changes. Consider referencing constants or helper functions for default formatting. - Reason this comment was not posted:
Comment was on unchanged code.
7. pkg/types/alertmanagertypes/route.go:17
- Draft comment:
In NewRouteFromRouteConfig, when an existing route is provided, only group-related fields are updated while the Receiver remains unchanged. Please confirm that preserving the existing Receiver is the intended behavior. - 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 their intention regarding the behavior of preserving the existing Receiver. This violates the rule against asking for confirmation of intention.
8. pkg/types/alertmanagertypes/route.go:24
- Draft comment:
The call to UnmarshalYAML with a no-op function is used for validation. It might be beneficial to document why a no-op callback is sufficient and if additional validation should be considered. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking for documentation on why a no-op function is used for validation and if additional validation should be considered. This is not a direct code suggestion or a request for a test, but rather a request for explanation and consideration of additional validation. It violates the rule against asking the PR author to explain or ensure behavior is intended.
Workflow ID: wflow_tI0WDG9FnJdrfRKV
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 74edc94 in 1 minute and 51 seconds
More details
- Looked at
268lines of code in7files - Skipped
0files when reviewing. - Skipped posting
18drafted comments based on config settings.
1. frontend/src/api/channels/createMsTeams.ts:13
- Draft comment:
Renamed property to 'msteamsv2_configs'; ensure backend API expects this new key. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This comment violates our rules because it's asking the author to verify/ensure something. We should assume the author has already verified API compatibility since they made the change. If the backend wasn't ready, it would be caught by tests or builds. The comment doesn't point out a clear required code change.
Maybe the backend API change isn't deployed yet, and this is a valid concern about timing of deployments?
Even if there are deployment timing concerns, that's a deployment coordination issue, not a code review issue. The code itself is fine and will work once both changes are deployed.
Delete this comment as it violates our rule about not asking authors to verify/ensure things, and doesn't suggest any specific code changes.
2. frontend/src/api/channels/editMsTeams.ts:13
- Draft comment:
Updated property name to 'msteamsv2_configs' in edit endpoint payload; verify consistency with API spec. - 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 consistency with the API spec, which is against the rules. It does not provide a specific suggestion or point out a specific issue with the code.
3. frontend/src/api/channels/testMsTeams.ts:13
- Draft comment:
Ensure the test endpoint payload uses 'msteamsv2_configs' consistently; recheck API integration for tests. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold50%
The comment is asking the author to recheck the API integration for tests, which violates the rule against asking the author to double-check things. However, it also suggests ensuring consistency in usingmsteamsv2_configs, which is a specific suggestion. The comment is partially useful but needs to be rephrased to focus on the specific suggestion.
4. pkg/alertmanager/legacyalertmanager/provider.go:192
- Draft comment:
Using MSTeamsV2ReceiverToMSTeamsReceiver for marshalling receiver data ensures compatibility; ensure tests cover conversion errors. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure tests cover conversion errors, which violates the rule against asking the author to ensure testing. It doesn't provide a specific suggestion or point out a specific issue with the code.
5. pkg/alertmanager/legacyalertmanager/provider.go:263
- Draft comment:
Converting channels with MSTeamsV2ToMSTeams() provides legacy support; ensure proper error handling during conversion. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure proper error handling, which violates the rule against asking the author to ensure behavior is intended or tested. It doesn't provide a specific suggestion or point out a specific issue.
6. pkg/alertmanager/legacyalertmanager/provider.go:299
- Draft comment:
Conversion via MSTeamsV2ReceiverToMSTeamsReceiver in update payload ensures consistency; double-check integration tests. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to double-check integration tests, which violates the rule against asking for confirmation or ensuring testing. It doesn't provide a specific code suggestion or point out a specific issue.
7. pkg/sqlmigration/014_add_alertmanager.go:237
- Draft comment:
Migration function converts 'msteams' to 'msteamsv2' channels; ensure no data loss during conversion. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure no data loss during conversion, which violates the rule against asking the author to ensure behavior is intended or tested. It doesn't provide a specific suggestion or point out a specific issue.
8. pkg/sqlmigration/014_add_alertmanager.go:260
- Draft comment:
Conversion in msTeamsReceiverToMSTeamsV2Receiver correctly maps legacy to v2 configs; verify configuration mappings. - 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 the configuration mappings, which falls under the category of asking them to double-check or ensure something is correct. This violates the rule against asking the author to confirm or verify their changes.
9. pkg/types/alertmanagertypes/channel.go:159
- Draft comment:
MSTeamsV2ToMSTeams conversion function ensures legacy compatibility; ensure bi-directional conversion tests are in place. - 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 that tests are in place for a specific function. This violates the rule against asking the author to ensure that their change is tested. Therefore, this comment should be removed.
10. pkg/types/alertmanagertypes/receiver.go:95
- Draft comment:
MSTeamsV2ReceiverToMSTeamsReceiver conversion function correctly maps v2 config fields to legacy format; check for consistency with migration logic. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states that the conversion function works correctly, which is not necessary for the PR author to know.
11. frontend/src/api/channels/createMsTeams.ts:13
- Draft comment:
Renamed property from 'msteams_configs' to 'msteamsv2_configs'. Ensure that the backend now expects the new key and that the front‐end and back‐end remain in sync. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
12. frontend/src/api/channels/editMsTeams.ts:13
- Draft comment:
Updated property key to 'msteamsv2_configs'. Confirm that the API endpoint accepts the new naming convention. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
13. frontend/src/api/channels/testMsTeams.ts:13
- Draft comment:
Changed property key to 'msteamsv2_configs' in the test endpoint. Verify that testChannel endpoint correctly processes this new key. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
14. pkg/alertmanager/legacyalertmanager/provider.go:192
- Draft comment:
Conversion of receiver configuration using MSTeamsV2ReceiverToMSTeamsReceiver ensures legacy compatibility. Confirm that downstream systems expecting the legacy ms teams format are handled consistently. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
15. pkg/sqlmigration/014_add_alertmanager.go:237
- Draft comment:
The helper function msTeamsChannelToMSTeamsV2Channel correctly converts channels from type 'msteams' to 'msteamsv2'. Ensure that all legacy channels are appropriately migrated. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
16. pkg/sqlmigration/014_add_alertmanager.go:260
- Draft comment:
Conversion function msTeamsReceiverToMSTeamsV2Receiver transforms v1 configs to the new v2 format. Verify that all required fields are mapped correctly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
17. pkg/types/alertmanagertypes/channel.go:159
- Draft comment:
MSTeamsV2ToMSTeams function converts v2 channels back to the legacy format. Confirm that this reverse conversion is consistent with the forward migration. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
18. pkg/types/alertmanagertypes/receiver.go:95
- Draft comment:
MSTeamsV2ReceiverToMSTeamsReceiver correctly maps MSTeamsV2Configs to MSTeamsConfigs. Make sure that all necessary fields are accurately translated. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
Workflow ID: wflow_FmAnkpOsXEKs1Xln
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2565773 in 46 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. frontend/src/pages/ChannelsEdit/index.tsx:56
- Draft comment:
Using 'msteamsv2_configs' here as specified in the integration design is correct. Ensure that backend responses and tests are updated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is purely informative and asks the PR author to ensure that backend responses and tests are updated, which violates the rules against asking for confirmation or ensuring testing.
2. frontend/src/pages/ChannelsEdit/index.tsx:56
- Draft comment:
Good update: the key was changed from 'msteams_configs' to 'msteamsv2_configs' per migration. Make sure the backend now sends 'msteamsv2_configs' and that any legacy handling for 'msteams_configs' is cleaned up. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
Workflow ID: wflow_3aAU3NeGrPF2KMTb
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2a4891f in 44 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. pkg/sqlmigration/014_add_alertmanager.go:219
- Draft comment:
Using Model(channel) is correct since 'channel' is already a pointer. Passing &channel would create a double pointer error. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, explaining why the current implementation is correct. It doesn't suggest any changes or improvements, nor does it highlight any potential issues.
2. pkg/sqlmigration/014_add_alertmanager.go:219
- Draft comment:
Using Model(channel) is correct here because channel is already a pointer. The previous usage (Model(&channel)) would have passed a pointer to a pointer, which could cause update issues. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, explaining why the current code is correct. It doesn't suggest any changes or improvements, nor does it ask for confirmation or testing. According to the rules, purely informative comments should be removed.
Workflow ID: wflow_iYm5dsBjcR3ozuRf
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 47c4483 in 1 minute and 48 seconds
More details
- Looked at
135lines of code in3files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. pkg/alertmanager/config_test.go:38
- Draft comment:
Good use of the unified Unmarshal API with tag 'yaml' to avoid double decoding the config response. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. pkg/config/conf.go:60
- Draft comment:
The new Unmarshal method loops over multiple tags. Please add a comment clarifying the purpose and expected behavior of sequential unmarshalling with different tags. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking for clarification on the purpose and expected behavior of a method, which violates the rule against asking the PR author to explain or confirm their intention. It does not provide a specific code suggestion or identify a potential issue with the code itself.
3. pkg/signoz/config.go:80
- Draft comment:
Updated unmarshal call to use the new API with tag 'yaml' is appropriate; ensure all configuration factories are compatible with the new variant. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. pkg/alertmanager/config_test.go:38
- Draft comment:
Using a single Unmarshal call with the 'yaml' tag consolidates the previous two calls. Ensure that this approach correctly handles both YAML and any prior format (if JSON is also expected) and that nothing is lost with the removal of the separate UnmarshalYaml call. - Reason this comment was not posted:
Comment looked like it was already resolved.
5. pkg/config/conf.go:60
- Draft comment:
The modified Unmarshal function now iterates over multiple tags (defaulting to 'mapstructure' and any extra, e.g. 'yaml'). This loop can lead to successive decodings that may override earlier values. Verify that this merging behavior is intended, and consider breaking out once a decode succeeds or merging results explicitly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment violates several rules: 1) It starts with "Verify that..." which is explicitly discouraged 2) It's speculative about potential issues rather than pointing out a definite problem 3) It asks the author to confirm their intention 4) Without seeing how this function is used in practice, we can't be certain this is actually problematic.
The successive decoding behavior could genuinely cause subtle bugs that are hard to debug. The comment identifies a real technical concern.
While the concern might be valid, the comment's form violates our rules by being speculative and asking for verification rather than making a concrete suggestion for improvement.
The comment should be removed because it's phrased as a verification request rather than a concrete issue, and we don't have strong evidence that this is definitely problematic in practice.
6. pkg/config/conf.go:126
- Draft comment:
In the YamlV2UnmarshalHookFunc, using reflect.Indirect on &data to obtain a string may be fragile if data is not strictly a string or is nil. Consider adding checks to ensure safe conversion and avoid potential panics. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The code already has a guard against non-string types with f.Kind() != reflect.String check. The reflection is only used as a fallback after a safer type assertion. While reflection can be dangerous, this seems like a reasonable use case in a decode hook function. The comment doesn't provide a clear alternative approach.
I might be underestimating the risk of panics in edge cases. The code assumes data can be safely referenced and converted to string.
The function is specifically a decode hook for string conversions, and the primary type check ensures we only process strings. Any panic would indicate a deeper issue with the decode hook contract.
Delete the comment. While it raises a valid concern about reflection safety, the code has reasonable guards and the comment doesn't provide clear actionable improvements.
7. pkg/signoz/config.go:80
- Draft comment:
The updated call to conf.Unmarshal now passes the 'yaml' tag. Ensure that this change preserves backward compatibility with configurations that were previously decoded via separate YAML unmarshalling. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_fh6Jz6m7802MGWm9
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2735550 in 2 minutes and 40 seconds
More details
- Looked at
85lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. conf/example.yaml:113
- Draft comment:
Typo in comment: 'exahustive' should be 'exhaustive'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While the comment is technically correct about the typo, fixing typos in comments is a very minor issue that doesn't affect functionality. The rules state not to make comments that are obvious or unimportant. A typo in a comment, while incorrect, doesn't warrant a PR comment as it's not a significant issue requiring code changes.
The typo could potentially confuse readers of the documentation, and fixing it would improve the quality of the codebase. Documentation quality is important.
While documentation quality matters, this is an extremely minor issue. The meaning is still clear despite the typo, and raising it as a PR comment creates unnecessary noise in the review process.
The comment should be deleted as it addresses a trivial typo that doesn't impact functionality and creates unnecessary review noise.
2. conf/example.yaml:101
- Draft comment:
The alertmanager configuration block is added here. Ensure the new settings (e.g., poll_interval, maintenance_interval) align with the overall system defaults and usage explorer graph behavior as per issue 404. This diff only affects config; verify that the x-axis issue is resolved elsewhere. - 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 alignment with system defaults and verify an issue resolution, which violates the rule against asking for confirmation or verification. It doesn't provide a specific code suggestion or ask for a specific test to be written.
3. conf/example.yaml:113
- Draft comment:
Typo in comment: 'exahustive' should be 'exhaustive'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While the typo correction is accurate, the rules state not to make purely informative comments. A typo in a comment doesn't affect functionality and fixing it provides minimal value. The comment is technically about a changed line, but it's an extremely minor issue that doesn't impact code quality or functionality.
The typo could potentially confuse readers looking for documentation. Also, maintaining high quality even in comments shows attention to detail.
However, this is exactly the kind of minor, purely informative comment that the rules explicitly tell us to avoid. The meaning is still clear despite the typo.
The comment should be deleted as it's purely informative and doesn't require any meaningful code changes.
4. conf/example.yaml:101
- Draft comment:
The diff only updates configuration (Alertmanager, TelemetryStore, etc.) and does not include any changes addressing the usage explorer graph's x-axis scaling issue described in issue Usage explorer graph should occopy the whole hr in x-axis rather than just being centred on the specific point of time #404. Please ensure that the intended frontend modifications are included or update the description 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 that the intended frontend modifications are included or to update the description. This violates the rule against asking the PR author to update the PR description or to ensure that changes are included. Therefore, this comment should be removed.
5. conf/example.yaml:113
- Draft comment:
Typo: 'exahustive' should be corrected to 'exhaustive'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While the comment is technically correct about the typo, it's just a spelling error in a documentation comment. It doesn't affect functionality, code quality, or understanding. The link to the upstream documentation still works, and readers can still understand the meaning. The rules suggest we should only keep comments that require clear code changes or affect code quality.
The typo could potentially confuse non-native English speakers or make the documentation look unprofessional. Documentation quality is also important.
While documentation quality matters, this is a minor typo that doesn't impact understanding or functionality. The rules specifically say not to make purely informative comments or unimportant changes.
Delete the comment as it points out a minor documentation typo that doesn't impact code quality or functionality, falling into the category of unimportant changes.
Workflow ID: wflow_fZDPKu9c0DI4lNVE
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 8f903cc in 2 minutes and 29 seconds
More details
- Looked at
25lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. pkg/types/alertmanagertypes/config.go:112
- Draft comment:
Avoid variable shadowing: the loop variable 'receiver' is re-declared inside the loop. Consider renaming the inner variable (e.g. 'newReceiver') for clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. pkg/types/alertmanagertypes/config.go:112
- Draft comment:
The loop re-marshals and then re-parses each receiver via NewReceiver. If this double conversion isn’t strictly required (i.e. if the receiver is already in the desired structure), consider refactoring to avoid potential performance overhead. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. pkg/types/alertmanagertypes/config.go:229
- Draft comment:
There is a potential typographical error: the error code ErrCodeAlertmanagerChannelNotFound is used in the GetReceiver function, but no such constant is defined anywhere. It is likely that ErrCodeAlertmanagerConfigNotFound was intended. Please verify and correct this, to ensure consistency with the rest of the error codes. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_Z25MxAmybuGSlP5f
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e1be10c in 25 minutes and 51 seconds
More details
- Looked at
246lines of code in9files - Skipped
0files when reviewing. - Skipped posting
13drafted comments based on config settings.
1. pkg/sqlmigration/014_add_alertmanager.go:112
- Draft comment:
When scanning for an organization (lines 99–110), the code checks for sql.ErrNoRows but then uses an if (err == nil). Consider making the behavior explicit when no org is found (e.g. logging or returning a clear error) so that the migration doesn’t silently skip populating data. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This is a database migration script that adds alertmanager functionality. The behavior of skipping population when no org exists seems intentional - it's a valid case where a fresh installation might not have any orgs yet. The code handles this gracefully by not erroring. Adding logging here might not be valuable since migrations typically run during startup/deployment.
The comment raises a valid point about explicit error handling. Silent failure could make debugging harder if something goes wrong.
However, this is a migration script where silent skipping is acceptable when no orgs exist. The code correctly handles both error cases - real errors are returned, while no-rows is handled gracefully.
The comment should be deleted. The current error handling is appropriate for a migration script, and adding logging or errors for the no-rows case would not improve the code.
2. pkg/sqlmigration/014_add_alertmanager.go:167
- Draft comment:
In the populateAlertmanagerConfig function (lines 167–185), the code extracts 'preferredChannels' from the rules table using gjson. It would be beneficial to add extra validation/logging if the expected JSON structure changes or if the field is missing, to prevent misconfiguration in rule-to-receiver mapping. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. pkg/sqlmigration/014_add_alertmanager.go:259
- Draft comment:
The msTeamsReceiverToMSTeamsV2Receiver conversion (lines 259–280) transforms MSTeamsConfigs to MSTeamsV2Configs. It would be useful to include unit tests ensuring this mapping is correct and preserves all necessary fields, as this conversion is crucial during migration. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. pkg/sqlmigration/014_add_alertmanager.go:236
- Draft comment:
In msTeamsChannelToMSTeamsV2Channel, the conversion of a channel from type "msteams" to "msteamsv2" is implemented correctly. Consider adding a comment clarifying that any errors from alertmanagertypes.NewReceiver are wrapped and that the new receiver conversion guarantees that required fields are preserved. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
5. pkg/sqlmigration/014_add_alertmanager.go:167
- Draft comment:
The logic that builds the matchersMap using gjson on rule data is clever. Consider adding a comment (or additional error handling) to note assumptions about the JSON structure (i.e. the presence of 'preferredChannels') so that future schema changes in alerting rules do not break this migration unexpectedly. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
6. pkg/sqlmigration/014_add_alertmanager.go:100
- Draft comment:
When retrieving an orgID with a SELECT id FROM organizations LIMIT 1, the code currently checks for err != nil and then if err == nil calls population functions. It might be more robust to explicitly handle the case where no rows are returned (sql.ErrNoRows) to make the intent clearer. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
7. pkg/sqlmigration/014_add_alertmanager.go:142
- Draft comment:
Overall, the migration sets up the alertmanager_config and alertmanager_state tables with proper foreign key constraints and ensures existing notification channels get an org_id. The conversion of msteams to msteamsv2 channels is applied to all channels before creating the global config. This transactional approach seems solid and meets the PR requirements. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%
None
8. pkg/query-service/auth/auth.go:540
- Draft comment:
In the Register function, after creating the first user, calling alertmanager.SetDefaultConfig(ctx, user.OrgID) ensures the alertmanager default config is established. This integration meets the PR requirements for incorporating the alertmanager into the ruler flow. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
9. pkg/query-service/auth/auth.go:542
- Draft comment:
The RegisterInvitedUser flow correctly validates the invite token, creates the user, and then deletes the invitation. This ensures that non-first users must be invited, which aligns with the design requirements for user registration in an enterprise setup. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
10. ee/query-service/app/api/auth.go:288
- Draft comment:
Typographical error: The log message on line 288 currently uses '[receiveML]' as its tag. For clarity and consistency with the rest of the code (e.g., '[receiveSAML]'), please update this to '[receiveSAML]'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. pkg/query-service/app/http_handler.go:4821
- Draft comment:
Typographical error: In the comment starting at line 4821, 'experssion' should be corrected to 'expressions'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. pkg/query-service/auth/auth.go:498
- Draft comment:
Typo: 'utils.GeneratePassowrd()' should likely be corrected to 'utils.GeneratePassword()' to avoid any confusion or errors. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. pkg/query-service/auth/auth.go:257
- Draft comment:
Typo/clarity: The error message 'failed to org id from context' seems to be missing a verb. Consider changing it to 'failed to extract org id from context' or similar for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_srY4jIYNWBHXXTEa
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
YounixM
approved these changes
Mar 9, 2025
srikanthccv
approved these changes
Mar 9, 2025
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.
Summary
Integrate the new implementations of the alertmanager along with changes to the ruler. This change can be broadly categoried into 3 parts:
Frontend
/api/v1/alertsapi was double encoding the response in json and sending it to the frontend. This PR fixes the json response object.For instance, we have gone from the response
{ "status": "success", "data": "{\"status\":\"success\",\"data\":[{\"labels\":{\"alertname\":\"[platform][consumer] consumer is above 100% memory utilization\",\"bu\":\"platform\",\"...... }to the response{"status":"success","data":[{"labels":{"alertname":"[Metrics] Pod CP......msteamshas been changed tomsteamsv2wherever applicableRuler
The following changes have been done in the ruler component:
Create,EditandDeletehave been made transactionaltestPrepareNotifyFuncfor sending test notificationsAlertmanager
Although a huge chunk of the alertmanagers have been merged in previous PRs (the list can be found at https://github.com/SigNoz/platform-pod/issues/404), this PR takes care of changes needed in order to incorporate it with the ruler
Related Issues / PR's
Closes https://github.com/SigNoz/platform-pod/issues/404
Closes https://github.com/SigNoz/platform-pod/issues/176
Screenshots
After change:
Affected Areas and Manually Tested Areas
Affected Areas are the Triggered alerts page.
Important
Integrates Alertmanager with the existing ruler, updating configurations, database schema, and adding tests for enhanced alerting capabilities.
Alertmanagerwithrulerinapi.go,server.go, andmanager.go.AlertmanagerAPItoAPIHandlerinapi.go.makeRulesManagerto includeAlertmanagerinserver.go.Alertmanagerconfiguration toconfig.goandprovider.go.NewAlertmanagerProviderFactoriesinprovider.go.ruleDBto handleAlertmanagerconfigurations indb.go.alertmanager_configandalertmanager_statetables in014_add_alertmanager.go.Alertmanagerintegration inprovider_test.go,config_test.go, andmatcher_test.go.alertManagerpackage and related code.manager.goto handle alert rules withAlertmanager.This description was created by
for 6965f42. It will automatically update as commits are pushed.
Important
Integrates Alertmanager with the existing ruler, updating configurations, database schema, and adding tests for enhanced alerting capabilities.
Alertmanagerwithrulerinapi.go,server.go, andmanager.go.AlertmanagerAPItoAPIHandlerinapi.go.makeRulesManagerto includeAlertmanagerinserver.go.Alertmanagerconfiguration toconfig.goandprovider.go.NewAlertmanagerProviderFactoriesinprovider.go.ruleDBto handleAlertmanagerconfigurations indb.go.alertmanager_configandalertmanager_statetables in014_add_alertmanager.go.Alertmanagerintegration inprovider_test.go,config_test.go, andmatcher_test.go.alertManagerpackage and related code.manager.goto handle alert rules withAlertmanager.This description was created by
for e1be10c. It will automatically update as commits are pushed.