Move integration building logic from Grafana #60
Conversation
| @@ -96,3 +100,89 @@ func (e ReceiverInitError) Error() string { | |||
| } | |||
|
|
|||
| func (e ReceiverInitError) Unwrap() error { return e.Err } | |||
|
|
|||
| type IntegrationsBuilder struct { | |||
There was a problem hiding this comment.
Can you please explain what your plan is with this structure? I don't think this should be split into a separate struct to satisfy the interface and instead just remove the interface itself.
We already have one level of indirection with the receiver Factory, and now we're introducing one more - I find this makes the code hard to follow and diverges with how this is done in two other places (upstream and Mimir) if possible, I'd like to have fewer differences not more.
There was a problem hiding this comment.
I use structure to pass multiple services through 3 functions of the builder. Currently, they are part of the Configuration interface, which is much harder to understand and read than this. The alternative would be to add them to the Alertmanager struct but I do not think this is a good solution because 1. those services (images service, notifications, logger factory, etc) are used by notifiers only, 2. the Alertmanager struct already contains many fields.
The Factory you mentioned is basically useless everywhere else because it requires data that cannot be provided by anything but the builder. So it can be made private and then simplified as it won't be used by anything else.
If you have a better idea, I would appreciate it if you could explain it. The way it looks right now in Grafana with the cascade of closures is way less readable.
There was a problem hiding this comment.
The alternative would be to add them to the Alertmanager struct but I do not think this is a good solution because 1. those services (images service, notifications, logger factory, etc) are used by notifiers only, 2. the Alertmanager struct already contains many fields.
I'd rather see them there. I don't see how this is a bad idea and struct with many fields is a pretty common thing in Go e.g. https://github.com/grafana/mimir/blob/main/pkg/ingester/ingester.go#L219-L275
I'm a big fan of keeping indirection low - as it makes it easier to reason about the system as a whole.
e03b095 to
be496f5
Compare
|
Closing this PR as there is no agreement on a way forward, and will go without builder struct |
|
I don't understand why this PR is closed - Pull Requests are discussions, and this is exactly what we were having here. I apologise if I came across as abrasive with my words but that was not my intention here - code refactors are a bit subjective and what seems obvious to you might not be for me at first glance. For the future, I wish to not be shut down the moment a conversation gets though and instead that we work together to reach a consensus or outcome. |
This PR splits the building of the integration from the alertmanager configuration.
Configurationis updated to require a methodReceiversthat returns a collection ofApiReceiverNotifierfromGrafanaReceiverconfiguration. The code is moved from Grafana with minimal changes.Relates to PR grafana/grafana#63340