-
Notifications
You must be signed in to change notification settings - Fork 433
Adding log as destination for alerting #87
Conversation
@@ -33,7 +33,7 @@ type alertManager struct { | |||
} | |||
|
|||
// NewManager ... Instantiates a new alert manager | |||
func NewManager(ctx context.Context, sc client.SlackClient) Manager { | |||
func NewSlackManager(ctx context.Context, sc client.SlackClient) Manager { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious to know the nature/rationale behind changing this name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially, there's a new NewLogManager
, keeping NewManager
would indicate it can take in configuration of which type. Since that isn't the case now, specifying what the function can handle would be useful.
func (am *alertManager) handleLogPost(alert core.Alert) { | ||
logger := logging.WithContext(am.ctx) | ||
|
||
logger.Error("Pessimism Alert", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if a warning would be more appropriate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, i contemplated that. It essentially depends on how downstream monitoring of logs are setup. Figured usually errors are monitored and consumed, not so much warnings. But we can make the log type as an option in the future.
makes sense. Co-authored-by: Ethen Pociask <[email protected]>
sure, works. Co-authored-by: Ethen Pociask <[email protected]>
Current Test Coverage
|
Fixes Issue
Closes #62
Changes proposed
Adding log as the destination for alerting.