Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add alerter to node and cli #1605

Closed
wants to merge 10 commits into from
Closed

feat: add alerter to node and cli #1605

wants to merge 10 commits into from

Conversation

MSevey
Copy link
Contributor

@MSevey MSevey commented Mar 19, 2024

Overview

closes #1600

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

Summary by CodeRabbit

  • New Features
    • Introduced a comprehensive alerting system for monitoring and reporting issues at various severity levels.
    • Added a new AlertsCmd command for fetching and displaying node alerts.
    • Implemented alert management capabilities across different node types (FullNode, LightNode) and their clients.
  • Enhancements
    • Enhanced block submission error handling with alert registration and logging.
  • Tests
    • Added tests for new alerting functionality and block submission error handling.

@MSevey MSevey marked this pull request as ready for review March 19, 2024 01:33
@MSevey MSevey enabled auto-merge March 19, 2024 01:40
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 9

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 553e399 and 59098f4.
Files selected for processing (13)
  • block/alerts.go (1 hunks)
  • block/manager.go (6 hunks)
  • block/manager_test.go (2 hunks)
  • cmd/rollkit/commands/alerts.go (1 hunks)
  • cmd/rollkit/main.go (1 hunks)
  • node/full.go (1 hunks)
  • node/full_client.go (1 hunks)
  • node/light.go (2 hunks)
  • node/node.go (2 hunks)
  • rpc/json/service.go (3 hunks)
  • rpc/json/types.go (2 hunks)
  • types/alerter.go (1 hunks)
  • types/alerter_test.go (1 hunks)
Additional comments: 13
cmd/rollkit/main.go (1)
  • 19-19: The addition of AlertsCmd to the CLI's root command is correctly implemented and integrates the new alerts functionality into the CLI.
node/node.go (1)
  • 24-24: Adding the Alerts() method to the Node interface is a significant enhancement for alerting capabilities. Ensure all implementations of the Node interface are updated to include this new method.
cmd/rollkit/commands/alerts.go (1)
  • 22-22: Acknowledging the TODO comment about making the HTTP request more generic as a positive indication of planned improvements for maintainability.
types/alerter_test.go (1)
  • 9-118: The tests for marshaling/unmarshaling AlertSeverity and the functionality of the Alerter interface are well-implemented and provide comprehensive coverage. Good job ensuring the new functionality is thoroughly tested.
types/alerter.go (1)
  • 1-203: The introduction of the Alerter interface, Alert and GenericAlerter types, and related constants and methods are well-implemented and align with the PR's objectives to enhance alerting capabilities across the system. The design and implementation of these components provide a robust framework for managing alerts.
rpc/json/service.go (1)
  • 89-89: The addition of the "alerts" method to the service's method map follows the established pattern and is correctly implemented.
block/manager_test.go (2)
  • 36-36: The initialization of the alerter field in the getManager function is correctly implemented and follows the expected pattern for adding alerting capabilities.
  • 483-550: The implementation of the TestSubmitBlocksToDAAlerts test function is well-structured and effectively tests the alerting behavior during block submission to the DA layer. It correctly simulates submission failures and successes and verifies the generated alerts.
block/manager.go (5)
  • 65-66: The addition of the daSubmissionFailureLog constant is a good practice for consistent logging of DA layer submission failures.
  • 125-126: The addition of the alerter field to the Manager struct is crucial for integrating the alerter system. Ensure that the alerter is properly initialized and utilized throughout the component.
  • 266-266: Initialization of the alerter in the NewManager function is correctly done. Ensure that the NewAlerter function properly handles any initialization errors and that the "block_manager" identifier is used appropriately.
  • 957-960: Handling of DA layer submission failures in submitBlocksToDA by registering an alert is a critical update for enhancing monitoring and alerting capabilities. Ensure that the alert severity and logging are appropriate for the failure event.
  • 911-913: Unregistration of alerts upon successful submission in submitBlocksToDA is important for maintaining the accuracy of the monitoring system. Verify that the unregistration does not inadvertently affect other unrelated alerts.

node/full_client.go Show resolved Hide resolved
node/full.go Outdated Show resolved Hide resolved
block/alerts.go Outdated Show resolved Hide resolved
cmd/rollkit/commands/alerts.go Outdated Show resolved Hide resolved
cmd/rollkit/commands/alerts.go Outdated Show resolved Hide resolved
cmd/rollkit/commands/alerts.go Outdated Show resolved Hide resolved
rpc/json/types.go Outdated Show resolved Hide resolved
node/light.go Outdated Show resolved Hide resolved
rpc/json/service.go Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 59098f4 and 026a371.
Files selected for processing (2)
  • cmd/rollkit/commands/alerts.go (1 hunks)
  • rpc/json/types.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • cmd/rollkit/commands/alerts.go
  • rpc/json/types.go

@rollkit rollkit deleted a comment from coderabbitai bot Mar 19, 2024
@MSevey
Copy link
Contributor Author

MSevey commented May 3, 2024

closing until needed

@MSevey MSevey closed this May 3, 2024
auto-merge was automatically disabled May 3, 2024 16:16

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Alerts Manager
1 participant