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

Propagate labels to Opsgenie details #2276

Merged
merged 6 commits into from
Jun 9, 2020
Merged

Propagate labels to Opsgenie details #2276

merged 6 commits into from
Jun 9, 2020

Conversation

ricoberger
Copy link
Contributor

Introduce two new config options for the Opsgenie receiver:

  • all_labels_as_details: If true, all labels are propagated to the Opsgenie details (default is false).
  • labels_as_details: An optional list of labels, which should be propagated to the Opsgenie details.

If the all_labels_as_details and labels_as_details value isn't set, the existing details option is used.

Closes #682.

Signed-off-by: ricoberger <mail@ricoberger.de>
@simonpasquier
Copy link
Member

Thanks for looking into this.

Instead of having 2 more parameters, maybe we can interpret labels_as_details: ['...'] as "include all common labels in the details" (this would be similar to what has been done for group_by).

As it stands, labels_as_details and details are exclusive so it would be good IMO to fail the configuration parsing if the user configures both at the same time. Or maybe the resulting map should be a merge of both with details taking precedence over labels_as_details?

@brian-brazil
Copy link
Contributor

this would be similar to what has been done for group_by

Grouping labels are not the same as common labels. The naming of these proposed config fields is confusing.

Signed-off-by: ricoberger <mail@ricoberger.de>
@ricoberger
Copy link
Contributor Author

Hi @simonpasquier and @brian-brazil, I made the suggested changes, so that there is only a details_labels field. The handling is similar to the group_by handling.

If the details_labels and details field is provided the loading of the configuration will fail now.

Copy link
Contributor

@brian-brazil brian-brazil left a comment

Choose a reason for hiding this comment

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

This is still misleading. Common is not all, in fact all is not possible.

config/notifiers.go Outdated Show resolved Hide resolved
config/notifiers.go Outdated Show resolved Hide resolved
@ricoberger
Copy link
Contributor Author

This is still misleading. Common is not all, in fact all is not possible.

So you want to change the option to details_common_labels or sth. similar?

Signed-off-by: ricoberger <mail@ricoberger.de>
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Amazing contribution and sorry for the delay in review.

I think this is almost ready to go. I have some idea that might make this even more flexible and simple to implement. Let me know what you think @ricoberger 🤗

I have a tiny proposal: What about having this option CommonLabelsAsDetails and if true it will populate details from common labels, and then anything in Details will be added additionally? It will be bit more flexible for user and also quite simpler to implement and ensure. (:

config/notifiers.go Outdated Show resolved Hide resolved
config/notifiers.go Outdated Show resolved Hide resolved
notify/opsgenie/opsgenie.go Outdated Show resolved Hide resolved
config/notifiers.go Outdated Show resolved Hide resolved
Signed-off-by: ricoberger <mail@ricoberger.de>
@ricoberger
Copy link
Contributor Author

Hi @bwplotka, good point. I adjusted the code accordingly.

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

This looks perfect to me, thanks! 👍

cc @simonpasquier

@@ -120,7 +120,14 @@ func (n *Notifier) createRequest(ctx context.Context, as ...*types.Alert) (*http

tmpl := notify.TmplText(n.tmpl, data, &err)

details := make(map[string]string, len(n.conf.Details))
details := make(map[string]string)

Copy link
Member

Choose a reason for hiding this comment

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

to be very strict this new line could be omitted. It's best to only put new line to separate logically different sections, whereas the map is related to below if. Not a blocker (:

@bwplotka
Copy link
Member

bwplotka commented Jun 9, 2020

Also do you think we can add a quick unit test @ricoberger ? (:

Signed-off-by: ricoberger <mail@ricoberger.de>
@ricoberger
Copy link
Contributor Author

Also do you think we can add a quick unit test @ricoberger ? (:

Sure. I added some test cases to the existing Opsgenie test.

@brian-brazil
Copy link
Contributor

then anything in Details will be added additionally?

I had had a similar thought. Would there be harm in always doing this, not needing a config option?

Also, if there is harm then is common labels safe as it's unpredictable what labels will actually be present?

@bwplotka
Copy link
Member

bwplotka commented Jun 9, 2020

Yea, good question. I think this is breaking compatibility, but with very tiny bad effect, just additional details. It's up to maintainers to decide, cc @simonpasquier

Since we are 0.x I think it's ok to just remove bool option and do it ;p

Sorry @ricoberger for so many direction changes. Open source is a collaborative work!

@brian-brazil
Copy link
Contributor

I think this is breaking compatibility but with very tiny bad effect, just additional details. It's up to maintainers to decide

Are you using OpsGenie so you can say exactly what might break? If that is the case then common labels is incorrect, and group labels should be used instead - though if it's group labels then you can do it with existing templating as you know the labels a-priori.

@ricoberger
Copy link
Contributor Author

Sorry @ricoberger for so many direction changes. Open source is a collaborative work!

No problem 🙂

I think this is breaking compatibility but with very tiny bad effect, just additional details. It's up to maintainers to decide

Are you using OpsGenie so you can say exactly what might break? If that is the case then common labels is incorrect, and group labels should be used instead - though if it's group labels then you can do it with existing templating as you know the labels a-priori.

I think this shouldn't break anything in Opsgenie. Existing details would be the same, so that created routing rules shouldn't be effected.

Should I adjust the PR accordingly?

@brian-brazil
Copy link
Contributor

I think this shouldn't break anything in Opsgenie.

In that case making this hardcoded would seem the best approach.

Signed-off-by: ricoberger <mail@ricoberger.de>
@ricoberger
Copy link
Contributor Author

I removed the config option and adjusted the tests.

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks 👍

@bwplotka bwplotka merged commit 6d77929 into prometheus:master Jun 9, 2020
@brian-brazil
Copy link
Contributor

👍

The docs over in the docs repo will also need an update.

@osterman
Copy link

This is wondeful! Any chance a new release will be cut @bwplotka?

@simonpasquier
Copy link
Member

@osterman last release was mid-June. I'd expect we can have another one beginning of September.

@Links2004
Copy link

Any timeline for the next release, this feature would help us a lot.

@srueg
Copy link

srueg commented Jan 12, 2021

Really looking forward to this, thanks! 🎉
Any news on a release?

@kedare
Copy link

kedare commented Apr 20, 2021

Looks like the option is now gone or has never been released ?
I don't see it in the current documentation :
https://prometheus.io/docs/alerting/latest/configuration/#opsgenie_config

@ricoberger
Copy link
Contributor Author

I think this was never released, because last release is from last summer.

@FlxPeters
Copy link

FlxPeters commented May 5, 2021

That should be included in 0.22.0, right? It's not included in the release notes.

roidelapluie added a commit to roidelapluie/alertmanager that referenced this pull request May 5, 2021
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
roidelapluie added a commit to roidelapluie/alertmanager that referenced this pull request May 5, 2021
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
roidelapluie added a commit to roidelapluie/alertmanager that referenced this pull request May 5, 2021
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
@roidelapluie
Copy link
Member

Thank you, I have missed it!

@ties
Copy link

ties commented May 6, 2021

For any other reader: In 4b59db0 it was changed to default to always passing all labels to OpsGenie.

(I did not find any reference to labels_as_details/all_labels_as_details in the current master branch)

roidelapluie added a commit that referenced this pull request May 6, 2021
@darkl0rd
Copy link

darkl0rd commented Jul 3, 2021

Is there a way to override the behavior? We also raise alerts for our support department, some of the labels we add should ideally not be visible to them? I’ve tried setting details explicitly, but it seems all labels are always added, no matter what I define?

@ricoberger
Copy link
Contributor Author

Hi @darkl0rd, no the behavior can not be changed, but it should be possible to overwrite the values for the labels which shouldn't be visible. When you set a label in the details section with an empty value, the original value for the label value shouldn't be visible in Opsgenie:

for k, v := range n.conf.Details {

@darkl0rd
Copy link

darkl0rd commented Jul 3, 2021

Thanks for the quick and detailed response. This should do the trick.

@darkl0rd
Copy link

darkl0rd commented Jul 4, 2021

Thanks for the quick and detailed response. This should do the trick.

Overriding with an empty value does indeed clear out the label. But the key remains visible in OpsGenie. Would it be an idea to adjust the code to only publish those keys which actually have a value set?

@darkl0rd
Copy link

darkl0rd commented Jul 6, 2021

Honestly, after playing around with it some more - I'd really like to have a way to get rid of the ExtraProperties all together. As labels differ per alert type, it's a hell of a job to determine which ones I need to blank out where. I'd much rather just explicitly define which ones I do want to show or even just disable the entire "details" section when the alert is sent to our first line support department.

paulfantom added a commit to paulfantom/alertmanager that referenced this pull request Jul 23, 2021
v0.22.2

* tag 'v0.22.2': (181 commits)
  Release 0.22.2
  Include pending silences for future muting decisions
  Release 0.22.1
  Default the isEqual flag to true in alertmanager
  Add test to expose issue prometheus#2426
  Release alertmanager 0.22
  Relase 0.22.0-rc.2
  API: Only pass cluster peer if empty
  fixed small typo
  Release 0.22.0-rc.1
  Fix panic when HA is disabled
  Update matcher examples
  Add prometheus#2276 to release notes
  Release 0.22.0-rc.0
  Lift moratorium on AlertManager receivers with long-term maintenance plan (prometheus#2561)
  Add OAuth 2.0 Config (prometheus#2560)
  Add missing EOL
  Fix flapping acceptance test
  Unlock at specific points instead of deferring
  Dispatch: Make sure mutex gets unlocked on call to Stop
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

opsgenie: propagate label k/v to opsgenie details