opsgenie: Add visible_to #3958

chlunde · 2024-08-15T13:11:11Z

Add visible_to with the same support for templating and fields as the
existing responders field.

Opsgenie documentation:

Teams and users that the alert will become visible to without sending
any notification. type field is mandatory for each item, where possible
values are team and user. In addition to the type field, either id or
name should be given for teams and either id or username should be given
for users. Please note: that alert will be visible to the teams that are
specified within responders field by default, so there is no need to
re-specify them within visibleTo field. You can refer below for example
values.

The functionality has been tested with unit tests, and with the following
configuration:

  route:
    receiver: og
    group_wait: 30s
    group_interval: 5m
    repeat_interval: 12h
  receivers:
  - name: og
    opsgenie_configs:
    - send_resolved: true
      api_key: ...top secret...
      api_url: https://api.eu.opsgenie.com/
      message: 'Lab message'
      description: 'Static description'
      source: lab
      details:
        foo: bar
      priority: P4
      visible_to:
      - type: user
        username: [email protected]

Fixes #3953

Add visible_to with the same support for templating and fields as the existing responders field. Opsgenie documentation: Teams and users that the alert will become visible to without sending any notification. type field is mandatory for each item, where possible values are team and user. In addition to the type field, either id or name should be given for teams and either id or username should be given for users. Please note: that alert will be visible to the teams that are specified within responders field by default, so there is no need to re-specify them within visibleTo field. You can refer below for example values. The functionality has been tested with unit tests, and with the following configuration: route: receiver: og group_wait: 30s group_interval: 5m repeat_interval: 12h receivers: - name: og opsgenie_configs: - send_resolved: true api_key: ...top secret... api_url: https://api.eu.opsgenie.com/ message: 'Lab message' description: 'Static description' source: lab details: foo: bar priority: P4 visible_to: - type: user username: [email protected] Fixes prometheus#3953 Signed-off-by: Carl Henrik Lunde <[email protected]>

grobinson-grafana · 2024-08-19T09:35:57Z

config/notifiers.go

+			return fmt.Errorf("opsGenieConfig visible_to %v has to have at least one of id, username or name specified", v)
+		}
+
+		if strings.Contains(v.Type, "{{") {


I'm afraid this check doesn't work. Consider the following example:

this is an invalid template }}

I suppose you could also check for strings.Contains(v.Type, "}}")?

Perhaps it would be better to just remove this if statement and check all VisibleTo's using template.New("").Parse()?

This is to match the behaviour for receivers, this code was introduced by 9ae6113.

In the case of this is an invalid template }} it will not be considered as a valid template, and ender the "else" case below, which will complain that it does not match the regex for valid types.

But you're right, at least I should not have reused opsgenieTypeMatcher, which is ^(team|teams|user|escalation|schedule)$. Fixed in new commit.

docs/configuration.md

grobinson-grafana · 2024-08-19T09:41:34Z

config/notifiers.go

@@ -638,6 +657,16 @@ type OpsGenieConfigResponder struct {
 	Type string `yaml:"type,omitempty" json:"type,omitempty"`
 }

+type OpsGenieConfigVisibleTo struct {
+	// One of those 3 should be filled.


Comment doesn't match the behavior in the code. The code allows for at least one to be filled, not at most one?

I tried to match the battle tested code for receivers here too. After considering changing the code, I think it is right.

We want to send exactly one of those fields to the OpsGenie API.
We must verify that at least one is set.
We probably should not disallow setting two or three as templates, as after templating, it is likely that exactly one has a value.

Of course, we could add a lot of validation to check that exactly one is set if it is not templates, and allow 1-3 set if they are all templates, but I'm not sure if it is worth the complexity?

grobinson-grafana · 2024-08-19T09:41:46Z

docs/configuration.md

+#### `<visible_to>`
+
+```yaml
+# Exactly one of these fields should be defined.


Copied from the receivers docs, but we could try to be more precise.

Something like:

Exactly one of these fields must have a value after templating. If none of the fields, including type, are set, the array item will be ignored.

Fixed in new commit.

grobinson-grafana · 2024-08-19T09:45:17Z

notify/opsgenie/opsgenie.go

+				Type:     tmpl(v.Type),
+			}
+
+			if visibleTo == (opsGenieCreateMessageVisibleTo{}) {


Why would someone want to do this? Is it to catch badly written templates that produce no output?

This code is identical to receivers, for consistent user experience.

Seems like this was introduced in 9ddc5f1

There is an explaination in the comment below. Is this OK?

// Filter out empty responders. This is useful if you want to fill // responders dynamically from alert's common labels.

grobinson-grafana · 2024-08-19T09:46:43Z

notify/opsgenie/opsgenie.go

+				teams := safeSplit(visibleTo.Name, ",")
+				for _, team := range teams {
+					newVisibleTo := opsGenieCreateMessageVisibleTo{
+						Name: tmpl(team),


Why template the name a second time?

Copy and paste from receivers, I did not catch that, thanks! Would you like me to remove the double templating in the code above too? Or have a dedicated PR for that? It does not change the results of the unit tests.

Fixed in new commit.

grobinson-grafana · 2024-08-19T09:47:33Z

notify/opsgenie/opsgenie.go

+				for _, team := range teams {
+					newVisibleTo := opsGenieCreateMessageVisibleTo{
+						Name: tmpl(team),
+						Type: tmpl("team"),


This doesn't make sense to me. You could template it back to user after we've checked that it is teams.

I think it should just be Type: "team". Fixed in new commit.

Not sure if I understand the reference to user here?

grobinson-grafana · 2024-08-19T09:47:49Z

notify/opsgenie/opsgenie.go

+				continue
+			}
+
+			if visibleTo.Type == "teams" {


Where does teams plural come from btw? In the documentation all I can see is team and user?

type field is mandatory for each item, where possible values are team and user

https://docs.opsgenie.com/docs/alert-api#create-alert

This code is identical to receivers, for consistent user experience.

Seems like this was introduced in 05490d8

It is an extension in alert manager not covered by the opsgenie API and documentation, but it is described in configuration.md:

# The `teams` type is configured using the `name` field above. # This field can contain a comma-separated list of team names. # If the list is empty, no additional visibility will be configured.

chlunde

Thanks for the review, sorry for the delay. I added a new commit that addresses some of the code review comments.

But a lot of the comments apply to the existing code, so I would like to get your input again on them before making further changes. Could you mark the ones you think are OK as resolved, and comment on the others?

chlunde · 2024-09-10T18:32:52Z

notify/opsgenie/opsgenie.go

+				Type:     tmpl(v.Type),
+			}
+
+			if visibleTo == (opsGenieCreateMessageVisibleTo{}) {


This code is identical to receivers, for consistent user experience.

Seems like this was introduced in 9ddc5f1

There is an explaination in the comment below. Is this OK?

// Filter out empty responders. This is useful if you want to fill // responders dynamically from alert's common labels.

chlunde · 2024-09-10T18:40:30Z

config/notifiers.go

+			return fmt.Errorf("opsGenieConfig visible_to %v has to have at least one of id, username or name specified", v)
+		}
+
+		if strings.Contains(v.Type, "{{") {


This is to match the behaviour for receivers, this code was introduced by 9ae6113.

In the case of this is an invalid template }} it will not be considered as a valid template, and ender the "else" case below, which will complain that it does not match the regex for valid types.

But you're right, at least I should not have reused opsgenieTypeMatcher, which is ^(team|teams|user|escalation|schedule)$. Fixed in new commit.

chlunde · 2024-09-10T18:46:11Z

config/notifiers.go

@@ -638,6 +657,16 @@ type OpsGenieConfigResponder struct {
 	Type string `yaml:"type,omitempty" json:"type,omitempty"`
 }

+type OpsGenieConfigVisibleTo struct {
+	// One of those 3 should be filled.


I tried to match the battle tested code for receivers here too. After considering changing the code, I think it is right.

We want to send exactly one of those fields to the OpsGenie API.
We must verify that at least one is set.
We probably should not disallow setting two or three as templates, as after templating, it is likely that exactly one has a value.

Of course, we could add a lot of validation to check that exactly one is set if it is not templates, and allow 1-3 set if they are all templates, but I'm not sure if it is worth the complexity?

chlunde · 2024-09-10T18:46:22Z

docs/configuration.md

+#### `<visible_to>`
+
+```yaml
+# Exactly one of these fields should be defined.


Copied from the receivers docs, but we could try to be more precise.

Something like:

Exactly one of these fields must have a value after templating. If none of the fields, including type, are set, the array item will be ignored.

Fixed in new commit.

chlunde · 2024-09-10T18:54:46Z

notify/opsgenie/opsgenie.go

+				continue
+			}
+
+			if visibleTo.Type == "teams" {


This code is identical to receivers, for consistent user experience.

Seems like this was introduced in 05490d8

It is an extension in alert manager not covered by the opsgenie API and documentation, but it is described in configuration.md:

# The `teams` type is configured using the `name` field above. # This field can contain a comma-separated list of team names. # If the list is empty, no additional visibility will be configured.

chlunde · 2024-09-10T19:02:20Z

notify/opsgenie/opsgenie.go

+				teams := safeSplit(visibleTo.Name, ",")
+				for _, team := range teams {
+					newVisibleTo := opsGenieCreateMessageVisibleTo{
+						Name: tmpl(team),


Copy and paste from receivers, I did not catch that, thanks! Would you like me to remove the double templating in the code above too? Or have a dedicated PR for that? It does not change the results of the unit tests.

Fixed in new commit.

chlunde · 2024-09-10T19:03:24Z

notify/opsgenie/opsgenie.go

+				for _, team := range teams {
+					newVisibleTo := opsGenieCreateMessageVisibleTo{
+						Name: tmpl(team),
+						Type: tmpl("team"),


I think it should just be Type: "team". Fixed in new commit.

Not sure if I understand the reference to user here?

Signed-off-by: Carl Henrik Lunde <[email protected]>

chlunde · 2024-10-08T06:39:14Z

@grobinson-grafana Could you take another look? Or should we involve the authors of the original code? Alternatively I could make a stripped down version of the PR but that would make this field behave differently from receivers

chlunde force-pushed the opsgenie-visibleto branch from dc1641f to 656407e Compare August 15, 2024 13:18

chlunde force-pushed the opsgenie-visibleto branch from 656407e to db0f43f Compare August 15, 2024 13:18

grobinson-grafana reviewed Aug 19, 2024

View reviewed changes

chlunde commented Sep 10, 2024

View reviewed changes

opsgenie: Revise visible_to after code review

ff034bc

Signed-off-by: Carl Henrik Lunde <[email protected]>

chlunde force-pushed the opsgenie-visibleto branch from 653dac0 to ff034bc Compare September 10, 2024 19:48

chlunde requested a review from grobinson-grafana September 10, 2024 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

opsgenie: Add visible_to #3958

opsgenie: Add visible_to #3958

chlunde commented Aug 15, 2024 •

edited

Loading

grobinson-grafana Aug 19, 2024 •

edited

Loading

chlunde Sep 10, 2024

grobinson-grafana Aug 19, 2024

chlunde Sep 10, 2024

grobinson-grafana Aug 19, 2024

chlunde Sep 10, 2024

grobinson-grafana Aug 19, 2024

chlunde Sep 10, 2024 •

edited

Loading

grobinson-grafana Aug 19, 2024

chlunde Sep 10, 2024

grobinson-grafana Aug 19, 2024

chlunde Sep 10, 2024

grobinson-grafana Aug 19, 2024

chlunde Sep 10, 2024

chlunde left a comment

chlunde Sep 10, 2024 •

edited

Loading

chlunde Sep 10, 2024

chlunde Sep 10, 2024

chlunde Sep 10, 2024

chlunde Sep 10, 2024

chlunde Sep 10, 2024

chlunde Sep 10, 2024

chlunde commented Oct 8, 2024

opsgenie: Add visible_to #3958

Are you sure you want to change the base?

opsgenie: Add visible_to #3958

Conversation

chlunde commented Aug 15, 2024 • edited Loading

grobinson-grafana Aug 19, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chlunde Sep 10, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chlunde left a comment

Choose a reason for hiding this comment

chlunde Sep 10, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chlunde commented Oct 8, 2024

chlunde commented Aug 15, 2024 •

edited

Loading

grobinson-grafana Aug 19, 2024 •

edited

Loading

chlunde Sep 10, 2024 •

edited

Loading

chlunde Sep 10, 2024 •

edited

Loading