Separate and export BuildReceiverIntegrations #3553

alexweav · 2023-10-09T22:48:59Z

This allows BuildReceiverIntegrations to be re-used from multiple places.

Initially, this change is done to support: #3491
in which integrations must be built for a purpose other than the usual notification pipeline, and not from the alertmanager config.

That PR simply duplicated the code, but this led to inconsistencies where the notification pipeline and receiver test code would drift. This change instead allows the same logic to be used in multiple places, allowing the above PR to eliminate redundant code in Alertmanager.

This also means downstream consumers of notifiers can re-use this builder code - they currently tend to duplicate it, leading to inconsistency. Added a couple extension points for this:

Consumers can now pass in prometheus/common/config HTTP client options, they will be applied to all relevant notifiers. These notifier arguments are currently largely unused in the prometheus codebase.
Added a wrapper func allowing consumers to modify notifiers as needed.

Signed-off-by: Alex Weaver <[email protected]>

alexweav · 2023-10-09T23:01:36Z

notify/receiver/receiver.go

+
+// BuildReceiverIntegrations builds a list of integration notifiers off of a
+// receiver config.
+func BuildReceiverIntegrations(nc config.Receiver, tmpl *template.Template, logger log.Logger, wrap Wrapper, httpOpts ...commoncfg.HTTPClientOption) ([]notify.Integration, error) {


Largely unchanged, except for the wrap and httpOpts parameters and their handling.

alexweav · 2023-10-09T23:01:58Z

notify/receiver/receiver_test.go

+
+func (s sendResolved) SendResolved() bool { return bool(s) }
+
+func TestBuildReceiverIntegrations(t *testing.T) {


Copied verbatim from main_test.go

alexweav · 2023-10-09T23:02:11Z

notify/receiver/receiver_test.go

+		})
+	}
+
+	t.Run("invokes notifier wrapper func", func(t *testing.T) {


This is the only new test code.

alexweav · 2023-10-09T23:05:55Z

notify/receiver/receiver.go

+			if wrap != nil {
+				n = wrap(name, n)
+			}


I'd like to drop this abstraction - its only real use, is that the wrapper code can correlate Name->Notifier and do special things based on the notifier type. The notifiers cannot otherwise be disambiguated based on what is returned by this function.

A couple alternative approaches:

Add a function func Type() string to the notify.Notifier interface, that lets client code see what type a notifier is.

Return a map of type->Notifier rather than an opaque slice of notifiers from this function.

The current approach is what grafana/mimir does: https://github.com/grafana/mimir/blob/7746f752c23fd5499fca59268d694a3345cfb29e/pkg/alertmanager/alertmanager.go#L466

Do any others have thoughts here?

I like the first approach of modifying the Notifier interface - I think it's quite useful at the expense of a breaking change.

Experimented with this idea a bit -

this is a deep change, for a PR that's trying to refactor something else. There's no common embedded object among the receivers, so we either need to introduce one, or add the method to each receiver individually. It's not complicated, but it touches a lot of code.

I also think the map approach might have merit and would like to experiment a bit.

I'm happy to do this, but perhaps as a separate PR that's not just a simple function export? I'll drop the wrap() bit from here, and we can figure out the best extension point as part of its own change.

Agreed - I like this approach.

alexweav · 2023-10-09T23:08:45Z

notify/receiver/receiver.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package receiver


This had to be moved to a new package due to package import cycles.

Happy to choose any other location for this.

Fun fact - this function actually did used to be exported, up until the notify package was split up.

There's a relevant comment thread here: #1929 (comment)

I like how every package inside notify (with a very obvious exception in test) is an integration. Can this not be part of the notify package or a completely separate package?

Can't be part of notify due to package dependencies. All the integrations depend on notify, and then this code depends on all the integrations, so we'd have an import cycle.

I moved it to a subpackage of config instead (config/receiver), since it's logically glue code between config and notify. WDYT?

I can't make it part ofconfig directly for the same reason as notify - import cycles. It needs to be a subpackage somewhere.

Yeah, I'm happy to keep this in config.

notify/receiver/receiver_test.go

gotjosh

Looks good so far, but please see my comments.

Signed-off-by: Alex Weaver <[email protected]>

…d export Signed-off-by: Alex Weaver <[email protected]>

Signed-off-by: Alex Weaver <[email protected]>

gotjosh

LGTM

* Move and export BuildReceiverIntegrations Signed-off-by: Alex Weaver <[email protected]> --------- Signed-off-by: Alex Weaver <[email protected]>

alexweav added 4 commits October 9, 2023 17:49

Move and export BuildReceiverIntegrations

eba70eb

Signed-off-by: Alex Weaver <[email protected]>

Allow each notifier's client options to be configured from the builder

9a3f6b3

Signed-off-by: Alex Weaver <[email protected]>

Add wrapper extension

0ddc18b

Signed-off-by: Alex Weaver <[email protected]>

Add test and marker const for wrapper

6695f7d

Signed-off-by: Alex Weaver <[email protected]>

alexweav force-pushed the alexweav/extract-reuse-buildReceiverIntegrations branch from 5774ce0 to 6695f7d Compare October 9, 2023 22:49

alexweav mentioned this pull request Oct 9, 2023

amtool: command to test receivers #3491

Open

Address linter complaints

12a3d16

Signed-off-by: Alex Weaver <[email protected]>

alexweav commented Oct 9, 2023

View reviewed changes

gotjosh reviewed Oct 12, 2023

View reviewed changes

notify/receiver/receiver_test.go Outdated Show resolved Hide resolved

gotjosh reviewed Oct 12, 2023

View reviewed changes

alexweav added 3 commits October 16, 2023 17:04

Move files to subpackage of config

4d0bed5

Signed-off-by: Alex Weaver <[email protected]>

Drop wrapper extension and make the PR a more straightforward move an…

60e9531

…d export Signed-off-by: Alex Weaver <[email protected]>

empty commit to kick build

d1ef55f

Signed-off-by: Alex Weaver <[email protected]>

gotjosh approved these changes Oct 17, 2023

View reviewed changes

gotjosh merged commit 412f062 into prometheus:main Oct 17, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate and export BuildReceiverIntegrations #3553

Separate and export BuildReceiverIntegrations #3553

alexweav commented Oct 9, 2023 •

edited

Loading

alexweav Oct 9, 2023

alexweav Oct 9, 2023

alexweav Oct 9, 2023

alexweav Oct 9, 2023 •

edited

Loading

gotjosh Oct 12, 2023

alexweav Oct 16, 2023

gotjosh Oct 17, 2023

alexweav Oct 9, 2023

alexweav Oct 11, 2023 •

edited

Loading

gotjosh Oct 12, 2023

alexweav Oct 16, 2023 •

edited

Loading

gotjosh Oct 17, 2023

gotjosh left a comment

gotjosh left a comment


		func (s sendResolved) SendResolved() bool { return bool(s) }

		func TestBuildReceiverIntegrations(t *testing.T) {

Separate and export BuildReceiverIntegrations #3553

Separate and export BuildReceiverIntegrations #3553

Conversation

alexweav commented Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexweav Oct 9, 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

alexweav Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexweav Oct 16, 2023 • edited Loading

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

gotjosh left a comment

Choose a reason for hiding this comment

alexweav commented Oct 9, 2023 •

edited

Loading

alexweav Oct 9, 2023 •

edited

Loading

alexweav Oct 11, 2023 •

edited

Loading

alexweav Oct 16, 2023 •

edited

Loading