Refactor alerting package #49

yuri-tceretian · 2023-01-31T21:06:15Z

All code moved from alerting sub-package to root. This removes the duplicated path component, e.g "github.com/grafana/alerting/alerting/models" turns to "github.com/grafana/alerting/models"
Refactored channels package.

Package renamed to receivers
All receivers are moved into their own sub-packages
Logging interface is moved to logging package in the root
Code for working with images is moved to images package in the root
Templates, structs and functions to work with them are moved to templates package in the root

All receivers renamed to Notifier. So reference to receivers will look like email.Notifier and constructor is email.New
Removed a factory function *Factory that wrapped New function with a ReceiverInitError and replaced by a generic function in the Factory.
Copied receivers factory from Grafana.

Fixes #46

gotjosh

LGTM, but please look at my comments.

I was hoping this was done per package as opposed to in one go so that can we have isolated, bike shedable conversations (as this is a good time to bike shed).

images/images.go

images/testing.go

images/utils.go

gotjosh · 2023-02-01T17:34:05Z

notify/factory.go

+	"github.com/grafana/alerting/receivers/wecom"
+)
+
+var receiverFactories = map[string]func(receivers.FactoryConfig) (NotificationChannel, error){


NotificationChannel is a legacy Grafana concept, is it possible to get rid of it? I'm fine with this part of the separate PR as this PR is already kind of hard to review as it is right now.

It's just a composition of two interfaces. I think we can but not in this PR. We need to move code that builds integrations from Grafana first.

gotjosh · 2023-02-01T17:36:23Z

notify/factory.go

+	notify.ResolvedSender
+}
+
+func Factory(receiverType string) (func(receivers.FactoryConfig) (NotificationChannel, error), bool) {


Do these functions warrant tests?

It probably does but this code is mostly moved as is from Grafana where it did not have the tests either. I am not sure what we can test here. Probably that all receivers are covered. Each receiver is covered by its own tests.

I'd test what happens when a receiver exists and when it does not.

gotjosh · 2023-02-01T17:38:20Z

notify/factory.go

@@ -0,0 +1,98 @@
+package notify


I gotta admit, I'm not a fan of this pattern at all. I spent a big portion of my career writing this type of code in ruby and 10 years later I continue to find this hard to read/understand.

The layer of abstraction for the sake of saving an initialisation of an error seems too much.

That being said, I consider this a nit - if you all agree this is OK feel free to ignore my comment.

Not sure to which line of code you refer to. Do you mean wrap function? If yes, I am not a fan of it either, this was made as a workaround for providing a validation of the configuration prior to saving it to the database.
Currently, there is a similar factory method for each receiver and it looks exactly the same. I just made it DRY-er.

Not sure to which line of code you refer to

The whole design within this file. It's called factory so given I cannot comment on files I tend to typically pick the first line of the file when referring to the whole file.

Ah, definitely, I think we should change it to the way it is implemented in Alertmanager!

gotjosh · 2023-02-01T17:44:38Z

receivers/line/config.go

+	Description string `json:"description,omitempty" yaml:"description,omitempty"`
+}
+
+func BuildConfig(fc receivers.FactoryConfig) (*Config, error) {


nit: This is validating the configuration but it's called BuildConfig in all receivers - this feels misleading, I understand that this "technically" returning a configuration because it's doing the marshalling but the common pattern I see in most cases is to simply execute the unmarshall call inline and call Validate() on top of the built object.

I do not have a strong opinion on this name. BuildConfig and Validate have pros and cons. Ideally, that should be boiled down to custom marshaller implementation for each struct but for now, decryption functionality does not let us do that (I have a ddoc for that).

I renamed BuildConfig to ValidateConfig.

yuri-tceretian · 2023-02-01T19:02:16Z

I was hoping this was done per package as opposed to in one go so that can we have isolated, bike shedable conversations (as this is a good time to bike shed).

I tried to do so by package but when I touched something it just affected all other files. So, I decided to do it in kinda one change. I did not touch notify package though, as I am not sure what the end structure will be because we still have some code in the Grafana repository.

alexweav · 2023-02-01T20:44:33Z

receivers/alertmanager/alertmanager.go

 )

-type AlertmanagerConfig struct {
-	*NotificationChannelConfig
+type Config struct {


The package-per-receiver is an amazing change 👍

alexweav · 2023-02-01T20:45:54Z

receivers/alertmanager/alertmanager.go

+// Notifier sends alert notifications to the alert manager
+type Notifier struct {


We called the package receivers but generalized all the notifier types Notifier - WDYT on aligning this? I.e. calling the package two layers up notifiers or calling these structs Receiver?

Do "notifier" and "receiver" always mean the same thing in our context?

The reason I chose package receivers and struct name Notifier is that:

we have notify package, and having notifiers would look awkward. If we want to rename to notifiers then probably we need to rename notify package.

Notifier implements upstream's interface Notifier that has method Notify. Receiver in Alertmanager terminology is a set of Integrations, where each Integration wraps a Notifier.

gotjosh

LGTM

notify/factory.go

Co-authored-by: gotjosh <josue.abreu@gmail.com>

1. All code moved from alerting sub-package to root. This removes the duplicated path component, e.g "github.com/grafana/alerting/alerting/models" turns to "github.com/grafana/alerting/models" 2. Refactored channels package. - Package renamed to `receivers` - All receivers are moved into their own sub-packages - Logging interface is moved to `logging` package in the root - Code for working with images is moved to `images` package in the root - Templates, structs and functions to work with them are moved to `templates` package in the root 3. All receivers are renamed to Notifier. So reference to receivers will look like `email.Notifier` and the constructor is `email.New` 4. Removed a factory function *Factory that wrapped New function with a `ReceiverInitError` and replaced by a generic function in the Factory. 5. Copied receivers factory from Grafana. Co-authored-by: gotjosh <josue.abreu@gmail.com>

yuri-tceretian added 6 commits January 31, 2023 15:26

refactor

cd30034

split utils

6aee737

rename structs and methods to pass linter + remove factory methods

3225189

implement a generic wrapper for factory and use New

96c9ba2

move image utils to images

f5c0cb3

move ReceiverInitError to where it is used

853f6f6

yuri-tceretian requested a review from gotjosh January 31, 2023 21:06

yuri-tceretian added 2 commits January 31, 2023 16:13

missing renames

bae0515

nolint for type alias because there is already Receiver

8159bc9

yuri-tceretian self-assigned this Jan 31, 2023

yuri-tceretian added the kata:am_unify label Feb 1, 2023

gotjosh reviewed Feb 1, 2023

View reviewed changes

cleanup

2df3075

alexweav reviewed Feb 1, 2023

View reviewed changes

rename BuildConfig to ValidateConfig

dc0c7a5

gotjosh approved these changes Feb 2, 2023

View reviewed changes

notify/factory.go Outdated Show resolved Hide resolved

Update notify/factory.go

43f0b89

Co-authored-by: gotjosh <josue.abreu@gmail.com>

yuri-tceretian merged commit 173c836 into main Feb 2, 2023

yuri-tceretian deleted the yuri-tceretian/notify-refactor branch February 2, 2023 17:29

This was referenced Feb 2, 2023

Alerting: Update to alerting 20230203015918-0e4e2675d7aa (after refactoring) grafana/grafana#62823

Merged

Fix formatting of the error in receveris factory #51

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor alerting package #49

Refactor alerting package #49

yuri-tceretian commented Jan 31, 2023

gotjosh left a comment

gotjosh Feb 1, 2023

yuri-tceretian Feb 1, 2023

gotjosh Feb 1, 2023

yuri-tceretian Feb 1, 2023

gotjosh Feb 2, 2023

gotjosh Feb 1, 2023

yuri-tceretian Feb 1, 2023

gotjosh Feb 2, 2023

yuri-tceretian Feb 2, 2023

gotjosh Feb 1, 2023

yuri-tceretian Feb 1, 2023

yuri-tceretian Feb 1, 2023

yuri-tceretian commented Feb 1, 2023

alexweav Feb 1, 2023

alexweav Feb 1, 2023

yuri-tceretian Feb 1, 2023

gotjosh left a comment

		// Notifier sends alert notifications to the alert manager
		type Notifier struct {

Refactor alerting package #49

Refactor alerting package #49

Conversation

yuri-tceretian commented Jan 31, 2023

gotjosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuri-tceretian commented Feb 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotjosh left a comment

Choose a reason for hiding this comment