Replace notify.Factory with a build function that accepts typed configuration #63

yuri-tceretian · 2023-02-14T21:51:54Z

Based on changes made in #61 and #62.
This PR introduces a factory function notify.BuildReceiverIntegrations that accepts parsed receiver configuration and creates Integrations.

Now, this looks pretty much closer to how upstream Alertmanager creates integrations.

Some notable changes:

All notifier constructors are replaced to accept only arguments they need instead of FactoryConfig.
FactoryConfig and the structs that it uses are deleted. Function GetDecryptedValueFn moved to notify package where it is used only once, which will make it simpler to delete in future.
Instead if NotificationSender there are two interfaces WebhookSender and EmailSender. In vanilla Alertmanager they are different for each notifier (actually, each notifier creates one by itself). Therefore, instead of accepting a single instance of senders BuildReceiverIntegrations now accepts factory functions, which can return an error. For example, lin in upstream Alertmanager https://github.com/prometheus/alertmanager/blob/41eb1213bb1c7ce0aa9e6464e297976d9c81cfe5/notify/webhook/webhook.go#L46

yuri-tceretian · 2023-03-15T18:48:30Z

logging/log.go

@@ -1,6 +1,6 @@
 package logging

-type LoggerFactory func(ctx ...interface{}) Logger
+type LoggerFactory func(loggerName string, ctx ...interface{}) Logger


this makes it clear that first element is always logger name whereas the remaining go to context (that's how it is implemented in Grafana)

I like this change, but wouldn't it mean that we'll be using a different interface than the one defined in Grafana's infra package? In order to use the logger, we would have to add this string as the first item in the ctx slice.
I don't think it's a big deal, just want to make sure we're aware of this.

Regularly, in the logging context is a slice of tuples. go-kit adds {Missing} if slice size is odd. However, In Grafana logger assumes that the first element is not a tuple's label but a logger name and automatically adds the logger label. However, this package can (and will) use a different logger implementation but usages of this interface still assume that the first element of the context is a logger. This change makes this interface more explicit.

I like this change, but wouldn't it mean that we'll be using a different interface than the one defined in Grafana's infra package?

There is no interface for New method in infra. What I do here is basically define it.

https://github.com/grafana/grafana/blob/862a6a2fa625100b690c5a48a9b12e7a7dd42041/pkg/infra/log/log.go#L231-L238

yuri-tceretian · 2023-03-15T18:49:29Z

notify/factory.go

 			}
+			createIntegration(idx, cfg, func(logger logging.Logger) notificationChannel {
+				return f(logger, w)
+			})


these to anonymous methods help reduce duplicated code below.

yuri-tceretian · 2023-03-15T18:50:25Z

notify/receivers.go

+// GetDecryptedValueFn is a function that returns the decrypted value of
+// the given key. If the key is not present, then it returns the fallback value.
+type GetDecryptedValueFn func(ctx context.Context, sjd map[string][]byte, key string, fallback string) string
+


This function is moved from the receivers package because it is not used there anymore.

santihernandezc

I like the idea of passing explicit values to the factory function instead of a FactoryConfig struct.
I requested a few comments to be added, but apart from that I think it's good to go!

santihernandezc · 2023-03-16T18:58:15Z

logging/log.go

@@ -1,6 +1,6 @@
 package logging

-type LoggerFactory func(ctx ...interface{}) Logger
+type LoggerFactory func(loggerName string, ctx ...interface{}) Logger


I like this change, but wouldn't it mean that we'll be using a different interface than the one defined in Grafana's infra package? In order to use the logger, we would have to add this string as the first item in the ctx slice.
I don't think it's a big deal, just want to make sure we're aware of this.

notify/factory.go

Co-authored-by: Santiago <[email protected]>

gotjosh

LGTM

gotjosh · 2023-03-23T17:09:02Z

receivers/wecom/wecom.go

+		Base:     receivers.NewBase(meta),
+		tmpl:     template,
+		log:      logger,
+		ns:       sender,


nit: This is called sender but the variable is installed called ns - this comment applies to all the others that use the webhook sender.

I will address it in a follow up PR as this rename will cause changes in other places.

receivers/slack/slack.go

receivers/discord/discord.go

gotjosh · 2023-03-23T17:27:08Z

receivers/alertmanager/alertmanager.go

-		settings: settings,
-		logger:   fc.Logger,
-	}, nil
+		Base:     receivers.NewBase(meta),


we filed #70 due to some problems we saw while reviewing this.

notify/factory.go

gotjosh · 2023-03-23T17:45:30Z

notify/factory.go

+// It returns a slice of Integration objects, one for each notification channel, along with any errors that occurred.
+func BuildReceiverIntegrations(
+	receiver GrafanaReceiverConfig,
+	tmpl *template.Template,


Suggested change

tmpl *template.Template,

tmpl *alerting.Template,

you need the local types otherwise you'll import the alertmanage directly.

I will address it in a follow-up PR as it touches many lines.

gotjosh · 2023-03-23T17:48:14Z

notify/factory.go

+	newWebhookSender func(n receivers.Metadata) (receivers.WebhookSender, error),
+	newEmailSender func(n receivers.Metadata) (receivers.EmailSender, error),


I think these functions should use primitives, such as:

func(ctx context.Context, client *http.Client, url string, body io.Reader) (*http.Response, error)

Instead of whatever interface was defined by Grafana with the WebhookSender and the EmailSender.

notify/factory.go

santihernandezc

All of my previous comments have been addressed 👍

yuri-tceretian self-assigned this Feb 14, 2023

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

yuri-tceretian mentioned this pull request Feb 14, 2023

Move integration building logic from Grafana #60

Closed

yuri-tceretian mentioned this pull request Feb 23, 2023

Parse notifier configs without creation of notifiers #62

Merged

yuri-tceretian force-pushed the yuri-tceretian/typed-factory branch 4 times, most recently from 95dc30b to 4684ec2 Compare March 8, 2023 00:03

yuri-tceretian requested review from gotjosh and santihernandezc March 8, 2023 00:18

yuri-tceretian marked this pull request as ready for review March 8, 2023 00:18

yuri-tceretian force-pushed the yuri-tceretian/typed-factory branch from 4684ec2 to bc56f06 Compare March 8, 2023 00:31

yuri-tceretian commented Mar 15, 2023

View reviewed changes

yuri-tceretian added 9 commits March 15, 2023 15:14

create alternative constructors for each notifier

2a18a67

replace factory with BuildReceiverIntegrations

c9f0b98

add notifier uid to log context

d5c03e8

remove old constructors

8144826

remove unused structs

cd4a081

hide interface

809ab30

make build accept factory of webhook and email senders and return error

94e6d30

tests

0c24bd1

remove factory.go

dbee25f

yuri-tceretian force-pushed the yuri-tceretian/typed-factory branch from c6f6f90 to dbee25f Compare March 15, 2023 19:14

santihernandezc reviewed Mar 16, 2023

View reviewed changes

yuri-tceretian and others added 4 commits March 22, 2023 11:39

Update notify/factory.go

9cc9790

Co-authored-by: Santiago <[email protected]>

Update notify/factory.go

4db293d

Co-authored-by: Santiago <[email protected]>

Update notify/factory.go

ddaf024

Co-authored-by: Santiago <[email protected]>

Update notify/factory.go

f6f6b7a

Co-authored-by: Santiago <[email protected]>

yuri-tceretian and others added 2 commits March 22, 2023 11:41

Update notify/factory.go

ff741c1

Co-authored-by: Santiago <[email protected]>

lint

f6d5712

yuri-tceretian mentioned this pull request Mar 22, 2023

Alerting: use alerting GrafanaReceiver and BuildReceiverConfiguration in Grafana grafana/grafana#65224

Merged

5 tasks

gotjosh approved these changes Mar 23, 2023

View reviewed changes

santihernandezc approved these changes Mar 23, 2023

View reviewed changes

yuri-tceretian added 2 commits March 28, 2023 11:18

misc fix

8ea2fed

simplify builder

7657484

yuri-tceretian merged commit c7c80f3 into main Mar 28, 2023

yuri-tceretian deleted the yuri-tceretian/typed-factory branch March 28, 2023 15:52

yuri-tceretian mentioned this pull request Mar 28, 2023

Use type alias for Template #71

Merged

yuri-tceretian mentioned this pull request Apr 14, 2023

Alerting: Update alerting module to 20230418161049-5f374e58cb32 + refactoring grafana/grafana#66622

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace notify.Factory with a build function that accepts typed configuration #63

Replace notify.Factory with a build function that accepts typed configuration #63

yuri-tceretian commented Feb 14, 2023 •

edited

Loading

yuri-tceretian Mar 15, 2023

santihernandezc Mar 16, 2023

yuri-tceretian Mar 22, 2023

yuri-tceretian Mar 15, 2023

yuri-tceretian Mar 15, 2023

santihernandezc left a comment

santihernandezc Mar 16, 2023

gotjosh left a comment

gotjosh Mar 23, 2023

yuri-tceretian Mar 28, 2023

gotjosh Mar 23, 2023

gotjosh Mar 23, 2023

yuri-tceretian Mar 28, 2023

gotjosh Mar 23, 2023

santihernandezc left a comment

		newWebhookSender func(n receivers.Metadata) (receivers.WebhookSender, error),
		newEmailSender func(n receivers.Metadata) (receivers.EmailSender, error),

Replace notify.Factory with a build function that accepts typed configuration #63

Replace notify.Factory with a build function that accepts typed configuration #63

Conversation

yuri-tceretian commented Feb 14, 2023 • edited Loading

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

santihernandezc left a 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

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

santihernandezc left a comment

Choose a reason for hiding this comment

yuri-tceretian commented Feb 14, 2023 •

edited

Loading