feat(sqlmigration): update the alertmanager tables

[vikrantgupta25]

Summary

• update the alert manager tables according to the new schema design

---

Important

Update alert manager tables to new schema with UUID primary keys and implement migration logic.

• Migration:
  • Add 021_update_alertmanager.go to migrate alert manager tables to new schema with UUID primary keys.
  • Implement CopyOldChannelToNewChannel, CopyOldConfigToNewConfig, and CopyOldStateToNewState for data migration.
  • Add UpdatePrimaryKey function in dialect.go for primary key migration.
• Schema Changes:
  • Change primary key from int to valuer.UUID in Channel, StoreableConfig, and StoreableState.
  • Update GetChannelByID, UpdateChannelByReceiverAndID, and DeleteChannelByID to use valuer.UUID.
• Miscellaneous:
  • Update NewChannelFromReceiver to generate UUIDs.
  • Modify GetColumnType in dialect.go to handle new schema.

^{This description was created by for f4202c6. It will automatically update as commits are pushed.}

[vikrantgupta25]

contributes to - https://github.com/SigNoz/platform-pod/issues/583

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 6d138bd in 3 minutes and 36 seconds

More details

• Looked at 880 lines of code in 14 files
• Skipped 0 files when reviewing.
• Skipped posting 14 drafted comments based on config settings.

1. pkg/alertmanager/alertmanager.go:37

• Draft comment:
  Good update: switched GetChannelByID and related methods to use valuer.UUID instead of int. This enhances type-safety.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative, explaining a change that was made. It does not provide a suggestion, ask for confirmation, or point out a potential issue. According to the rules, purely informative comments should be removed.

2. pkg/alertmanager/api.go:143

• Draft comment:
  Using valuer.NewUUID to convert the channel id is correct. Error messages now use '%s' for uuid string representation.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative, as it only states that the use of valuer.NewUUID is correct and that error messages now use '%s'. It doesn't provide any actionable feedback or suggestions for improvement.

3. pkg/alertmanager/legacyalertmanager/provider.go:269

• Draft comment:
  Updated GetChannelByID signature to use valuer.UUID; consistent type usage improves 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/alertmanager/signozalertmanager/provider.go:108

• Draft comment:
  Consistent update: the UpdateChannelByReceiverAndID now accepts valuer.UUID. All conversion handling appears correct.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative, stating that a function now accepts a certain type and that conversion handling appears correct. It doesn't provide any actionable feedback or suggestions for improvement.

5. pkg/signoz/provider.go:67

• Draft comment:
  New migration factory (NewUpdateAlertmanagerFactory) correctly added. Ensures the updated alertmanager schema is migrated.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative, as it only states that a new migration factory was correctly added. It doesn't provide any actionable feedback or suggestions for improvement.

6. pkg/sqlmigration/020_update_alertmanager.go:221

• Draft comment:
  Using valuer.GenerateUUID() for new channel IDs is a proper approach for schema migration. Transaction handling looks correct.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative, praising the use of a function and transaction handling. It doesn't provide any actionable feedback or suggestions for improvement.

7. pkg/types/alertmanagertypes/channel.go:129

• Draft comment:
  Updated GetChannelByID now uses valuer.UUID and updated error formatting with '%s', which is appropriate for a UUID string.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative, describing what changes were made without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest improvements.

8. pkg/types/alertmanagertypes/config.go:41

• Draft comment:
  StoreableConfig is refactored to use types.Identifiable and TimeAuditable; this enhances consistency across models.
• 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/types/alertmanagertypes/state.go:60

• Draft comment:
  Consider adding a default case in the switch over stateName in Set() to handle unexpected values instead of silently doing nothing.
• 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/types/alertmanagertypes/state.go:82

• Draft comment:
  For consistency and improved formatting, consider using errors.Newf instead of errors.New when returning the 'state not found' error in Get().
• 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/sqlmigration/020_update_alertmanager.go:70

• Draft comment:
  Typo: The type name 'newAlertManagerState' uses an inconsistent capitalization. Consider renaming it to 'newAlertmanagerState' to maintain consistency with other alertmanager types in the file.
• Reason this comment was not posted:
  Marked as duplicate.

12. pkg/sqlstore/postgressqlstore/dialect.go:31

• Draft comment:
  The comment on line 31 says "if the columns is integer then do this". Consider updating it to "if the column is integer, then do this" 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.

13. pkg/sqlstore/sqlitesqlstore/dialect.go:30

• Draft comment:
  Typo in comment: "if the columns is integer then do this" should be "if the column is integer then do this".
• 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.

14. pkg/types/alertmanagertypes/config.go:236

• Draft comment:
  Typographical error: The error message in GetReceiver uses 'channel with name %q not found' but it should refer to a 'receiver' since that's what is being handled. Please change it to 'receiver with name %q not found'.
• 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_q0c6LNkIJLyy7QPX

---

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.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 99020ab in 1 minute and 38 seconds

More details

• Looked at 60 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 6 drafted comments based on config settings.

1. pkg/alertmanager/alertmanagerstore/sqlalertmanagerstore/config.go:116

• Draft comment:
  Good catch on formatting: changed %d to %s to match the string type from id.StringValue().
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative, acknowledging a change in formatting from %d to %s. It doesn't provide any actionable feedback or suggestions for improvement.

2. pkg/sqlmigration/020_update_alertmanager.go:59

• Draft comment:
  Renamed types (e.g., existingAlertManagerState -> existingAlertmanagerState) for consistency. Verify all references are implemented consistently.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to verify consistency in renaming, which falls under the rule of not asking the author to double-check things. It doesn't provide a specific suggestion or point out a specific issue.

3. pkg/sqlmigration/020_update_alertmanager.go:256

• Draft comment:
  Updated function signature and type names in CopyOldStateToNewState to reflect the renaming changes.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative, as it only states what was done without providing any actionable feedback or suggestions. It does not align with the rules for useful comments.

4. pkg/alertmanager/alertmanagerstore/sqlalertmanagerstore/config.go:115

• Draft comment:
  Good fix: using '%s' instead of '%d' to format the UUID string is correct.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative, as it is simply acknowledging a correct change without providing any actionable feedback or suggestions. It does not align with the rules, which prohibit purely informative comments.

5. pkg/sqlmigration/020_update_alertmanager.go:57

• Draft comment:
  Rename types for consistency: use 'existingAlertmanagerState' and 'newAlertmanagerState' (all lowercase 'manager').
• Reason this comment was not posted:
  Comment looked like it was already resolved.

6. pkg/sqlmigration/020_update_alertmanager.go:256

• Draft comment:
  Update 'CopyOldStateToNewState' to reflect the new type names for clarity and consistency.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

Workflow ID: wflow_BeFFl0ancgtVp67e

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

❌ Changes requested. Incremental review on f4202c6 in 1 minute and 47 seconds

More details

• Looked at 283 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 6 drafted comments based on config settings.

1. pkg/sqlmigration/021_update_alertmanager.go:119

• Draft comment:
  The check 'if err == nil' after scanning is redundant since if err != nil, it would have returned earlier. Consider simplifying the error handling.
• 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 reviewer missed that sql.ErrNoRows is handled specially and doesn't cause an early return. The err == nil check is actually important because err could be sql.ErrNoRows at that point. Removing it would change the behavior to potentially process rows even when there was an ErrNoRows error.
  Could there be some other error handling pattern that would make this clearer? Maybe the code could be restructured to avoid this confusion?
  While the code could potentially be clearer, the current error handling is correct and the err == nil check is necessary for proper behavior.
  The comment should be deleted because its suggestion is incorrect - the err == nil check is necessary due to the special handling of sql.ErrNoRows.

2. pkg/sqlmigration/021_update_alertmanager.go:214

• Draft comment:
  Down migration is a no-op. If intentional, add a comment explaining why Down is empty.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to explain why the Down migration is a no-op. This is a request for clarification, which violates the rule against asking the author to confirm their intention or explain their changes. Therefore, this comment should not be approved.

3. pkg/sqlmigration/021_update_alertmanager.go:21

• Draft comment:
  Ensure table naming consistency. The old table 'notification_channels' (plural) is renamed to 'notification_channel' (singular). Confirm if this inconsistency 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 = 0% vs. threshold = 50%
  This comment violates several rules: 1) It starts with "Ensure that..." which is explicitly discouraged 2) It's asking the author to confirm their intention rather than pointing out a clear issue 3) It's not suggesting a clear action - just questioning consistency 4) Table naming conventions would be a cross-file concern that requires broader context about the codebase's standards.
  The change in naming convention could reflect a broader standardization effort across the codebase. Without seeing other table names, we can't be certain this is actually inconsistent.
  Even if there is an inconsistency, the comment doesn't provide actionable feedback and instead just asks for confirmation, which our rules explicitly discourage.
  The comment should be deleted as it's asking for confirmation rather than pointing out a clear issue requiring action.

4. pkg/sqlmigration/021_update_alertmanager.go:145

• Draft comment:
  Verify similar table renaming conventions for alertmanager config and state. Consistent naming across tables will prevent future confusion.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to verify naming conventions, which is against the rules as it asks for confirmation. It doesn't provide a specific suggestion or point out a clear issue.

5. pkg/sqlmigration/021_update_alertmanager.go:119

• Draft comment:
  The error handling when scanning into slices checks for sql.ErrNoRows, but many ORMs return an empty slice instead. Consider simplifying this check.
• 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%

1. The code is using Bun ORM which may have its own error handling patterns. 2. Without knowing Bun's specific behavior around empty results, we can't be certain if this error check is unnecessary. 3. The pattern is repeated in multiple places suggesting it may be intentional. 4. The current code works defensively by handling both cases.
   I don't have concrete evidence that Bun ORM definitely returns empty slices instead of sql.ErrNoRows. The comment makes assumptions about ORM behavior without specific knowledge of Bun.
   Since we can't be certain about Bun's exact behavior and the current code works safely by handling both cases, suggesting a simplification could introduce risks without clear benefits.
   The comment should be deleted since it makes assumptions about ORM behavior without concrete evidence, and the current defensive error handling is not clearly problematic.

6. pkg/sqlmigration/021_update_alertmanager.go:114

• Draft comment:
  If the tables contain many records, loading all data into memory could impact performance. Consider processing in batches.
• 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%
  This is a database migration script that copies data between tables. Loading all records at once could be problematic if tables are very large. However, these appear to be config/state tables that likely don't contain massive amounts of data. The notification channels, alertmanager configs and states are typically configuration entities that have relatively few records. The transaction ensures data consistency during migration.
  I could be wrong about the typical data volume in these tables. In some large deployments, there could potentially be many notification channels or alert states.
  While possible, these are configuration tables that by their nature tend to have limited records. The benefit of a simple, atomic migration likely outweighs the complexity of batching for these specific tables.
  The comment should be deleted. While technically correct, batching would add complexity for likely minimal benefit given these are config/state tables that typically don't have huge record counts.

Workflow ID: wflow_CPPZhsseNiDSxz9u

---

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.

[ellipsis-dev]

Down migration is a no-op. Consider documenting that this migration is irreversible or implement rollback logic if needed.