Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alerting: Use configured headers for external alertmanager #63819

Merged
merged 21 commits into from
Apr 21, 2023

Conversation

JohnnyQQQQ
Copy link
Member

@JohnnyQQQQ JohnnyQQQQ commented Feb 27, 2023

What is this feature?

We pass the configured headers from the alertmanager datasource to the external alertmanager call if any are configured.

Why do we need this feature?

Customers who are using an external alertmanager that need some headers, i.e. a multi tenant alertmanager from mimir or cortex.

Who is this feature for?

[Add information on what kind of user the feature is for.]

Which issue(s) does this PR fix?:

Fixes #60807

Special notes for your reviewer:

All changes were tested manually by me.

We did copy notifier.go from upstream and should try to upstream those changes afterward as they might be useful to a broader audience.

@JohnnyQQQQ JohnnyQQQQ added this to the 9.5.0 milestone Mar 2, 2023
@JohnnyQQQQ JohnnyQQQQ marked this pull request as ready for review March 2, 2023 13:23
@JohnnyQQQQ JohnnyQQQQ requested a review from a team as a code owner March 2, 2023 13:23
@JohnnyQQQQ JohnnyQQQQ requested a review from gotjosh March 2, 2023 13:23
Copy link
Contributor

@yuri-tceretian yuri-tceretian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think copying whole client is a good idea. We have our fork of prometheus alertmanager and I think it would make more sense to update it and then use it in sender.

@JohnnyQQQQ
Copy link
Member Author

I do not think copying whole client is a good idea. We have our fork of prometheus alertmanager and I think it would make more sense to update it and then use it in sender.

This code is from prometheus/prometheus and not from the prometheus/alertmanager.

Josh and me did an offline code review and agreed to move forward with this but try not to copy the config.go file. So I will submit some changes in the near future.

@yuri-tceretian
Copy link
Contributor

Then I would appreciate more information about why the decision was to vendor file from upstream instead of updating our fork and then seek for an opportunity to upstream it.

@yuri-tceretian yuri-tceretian dismissed their stale review March 6, 2023 18:32

I was pointed out that the copy was made from not alertmanager code but from prometheus.

Copy link
Contributor

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! Impressive job 👏👏👏👏👏👏👏

Very clean on the copy/paste from Prometheus - I hope we can upstream some of this work by making a good case around it.

Some parts of the router implementation can do with a bit of a cleanup; please see my comments.

pkg/services/ngalert/sender/notifier_ext.go Outdated Show resolved Hide resolved
pkg/services/ngalert/sender/router.go Outdated Show resolved Hide resolved
pkg/services/ngalert/sender/router.go Outdated Show resolved Hide resolved
pkg/services/ngalert/sender/router.go Outdated Show resolved Hide resolved
@grafanabot grafanabot removed this from the 9.5.0 milestone Apr 4, 2023
@grafanabot
Copy link
Contributor

This pull request was removed from the 9.5.0 milestone because 9.5.0 is currently being released.

@JohnnyQQQQ JohnnyQQQQ added this to the 10.0.0 milestone Apr 19, 2023
@JohnnyQQQQ JohnnyQQQQ requested a review from gotjosh April 19, 2023 16:17
Copy link
Contributor

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Beautiful 👏

@JohnnyQQQQ JohnnyQQQQ merged commit bbce69f into main Apr 21, 2023
10 checks passed
@JohnnyQQQQ JohnnyQQQQ deleted the external-am-headers branch April 21, 2023 14:16
@zerok zerok modified the milestones: 10.0.0, 10.0.0-preview May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Grafana managed alerts are not delivered to Mimir Alertmanager in multi-tenant mode
5 participants