AlertManager v2 #2643

Konstantinov-Innokentii · 2023-07-26T04:25:46Z

Introduce AlertManager v2 integration with improved internal behaviour – it's using grouping from AlertManager, not trying to re-group alerts on OnCall side.
Existing AlertManager and Grafana Alerting integrations are marked as Legacy with options to migrate them manually now or be migrated automatically after DEPRECATION DATE(TBD).
Integration urls and public api responses stay the same both for legacy and new integrations.

- Introduce legacy_alertmanager/grafana_alerting integrations - Hanlde legacy/v2 integrations in public API – Handle alerts from legacy/v2 integrations on same urls

# Conflicts: # CHANGELOG.md

joeyorlando

overall LGTM, nice work 👍 !

I would also check w/ someone who is more familiar w/ the engine/config_integrations/*.py files and CSS.

joeyorlando · 2023-07-31T11:59:41Z

CHANGELOG.md

@@ -25,6 +25,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 - Apply swap requests details to schedule events ([#2677](https://github.com/grafana/oncall/pull/2677))

+- Rework of AlertManager integration ([#2643](https://github.com/grafana/oncall/pull/2643))


move to "Unreleased" section

joeyorlando · 2023-07-31T12:00:44Z

docs/sources/integrations/alertmanager/index.md

-> You must have the [role of Admin][user-and-team-management] to be able to create integrations in Grafana OnCall.
+> ⚠️ A note about **(Legacy)** integrations:
+> We are changing internal behaviour of AlertManager integration.
+> Integrations that were created before version **VERSION** are marked as **(Legacy)**.


swap out **VERSION**

joeyorlando · 2023-07-31T12:01:00Z

docs/sources/integrations/alertmanager/index.md

+> ⚠️ A note about **(Legacy)** integrations:
+> We are changing internal behaviour of AlertManager integration.
+> Integrations that were created before version **VERSION** are marked as **(Legacy)**.
+> These integrations are still receiving and escalating alerts but will be automatically migrated after DEPRECATION_DATE.


swap out DEPRECATION_DATE

docs/sources/integrations/alertmanager/index.md

joeyorlando · 2023-07-31T12:03:59Z

docs/sources/integrations/grafana-alerting/index.md

+> Integrations that were created before version **VERSION** are marked as **(Legacy)**.
+> These integrations are still receiving and escalating alerts but will be automatically migrated after DEPRECATION_DATE.


same thing here w/ **VERSION** and DEPRECATION_DATE

I'll fill them right before the release, since I'll now VERSION only then.

joeyorlando · 2023-07-31T12:16:09Z

engine/apps/public_api/serializers/integrations.py

+        value = remove_legacy_prefix(value)
+        return value


Suggested change

value = remove_legacy_prefix(value)

return value

return remove_legacy_prefix(value)

joeyorlando · 2023-07-31T12:16:41Z

engine/apps/public_api/serializers/integrations.py

        if integration == AlertReceiveChannel.INTEGRATION_GRAFANA_ALERTING:
+            # TODO: probably only needs to check if unified alerting is on


is this TODO still relevant?

Yep, it still makes sense for Grafana Alerting v2

joeyorlando · 2023-07-31T14:54:03Z

grafana-plugin/src/pages/integrations/Integrations.tsx

+    return renderContent();
+
+    function renderContent() {
+      if (isLegacyIntegration) {
+        return (
+          <HorizontalGroup spacing="xs">
+            <Tooltip placement="top" content={'This integration has been deprecated, consider migrating it.'}>
+              <Icon name="info-circle" className="u-opacity" />
+            </Tooltip>
+            <Text type="secondary">
+              <span className="u-opacity">{integration?.display_name}</span>
+            </Text>
+          </HorizontalGroup>
+        );
+      }
+
+      return (
+        <HorizontalGroup spacing="xs">
+          <IntegrationLogo scale={0.08} integration={integration} />
+          <Text type="secondary">{integration?.display_name}</Text>
+        </HorizontalGroup>
+      );
+    }


is a separate renderContent function needed here? what about just including the body of renderContent inside of renderDatasource instead of declaring a separate function that is then always invoked?

no, not really. silly mistake

joeyorlando · 2023-07-31T14:55:56Z

grafana-plugin/src/pages/integration/Integration.tsx

+              {isLegacyIntegration && (
+                <WithPermissionControlTooltip userAction={UserActions.IntegrationsWrite}>
+                  <div
+                    className={cx('integration__actionItem')}
+                    onClick={() =>
+                      setConfirmModal({
+                        isOpen: true,
+                        title: 'Migrate Integration?',
+                        body: (
+                          <VerticalGroup spacing="lg">
+                            <Text type="primary">
+                              Are you sure you want to migrate <Emoji text={alertReceiveChannel.verbal_name} /> ?
+                            </Text>
+
+                            <VerticalGroup spacing="xs">
+                              <Text type="secondary">- Integration internal behaviour will be changed</Text>
+                              <Text type="secondary">
+                                - Integration URL will stay the same, so no need to change {getMigrationDisplayName()}{' '}
+                                configuration
+                              </Text>
+                              <Text type="secondary">
+                                - Integration templates will be reset to suit the new payload
+                              </Text>
+                              <Text type="secondary">- It is needed to adjust routes manually to the new payload</Text>
+                            </VerticalGroup>
+                          </VerticalGroup>
+                        ),
+                        onConfirm: onIntegrationMigrate,
+                        dismissText: 'Cancel',
+                        confirmText: 'Migrate',
+                      })
+                    }
+                  >
+                    Migrate
+                  </div>
+                </WithPermissionControlTooltip>
+              )}


did you consider adding a link to the migration docs you added?

Link is added in warning banner. This modal is a final message before pressing migrate button. I'll show a demo at weekly :)

joeyorlando · 2023-07-31T14:59:25Z

grafana-plugin/src/models/alert_receive_channel/alert_receive_channel.ts

+  async migrateChannelFilter(id: AlertReceiveChannel['id']) {
+    return await makeRequest(`/alert_receive_channels/${id}/migrate`, {
+      method: 'POST',
+    });
+  }
+


Suggested change

async migrateChannelFilter(id: AlertReceiveChannel['id']) {

return await makeRequest(`/alert_receive_channels/${id}/migrate`, {

method: 'POST',

});

}

migrateChannelFilter = (id: AlertReceiveChannel['id']) =>

makeRequest(`/alert_receive_channels/${id}/migrate`, {

method: 'POST',

});

All store methods use async/await notation, so I think it's worth to discuss it with frontend team. What is the advantage of using arrow function here?

It's just a preference of shortening the outputted code, no real advantage. IMO marking it as async/await it has better readability

Co-authored-by: Joey Orlando <joey.orlando@grafana.com>

iskhakov · 2023-08-01T04:27:45Z

engine/apps/alerts/models/alert_receive_channel.py

@@ -552,7 +555,12 @@ def send_demo_alert(self, payload=None):
        if payload is None:
            payload = self.config.example_payload

-        if self.has_alertmanager_payload_structure:
+        # TODO: AMV2: hack to keep demo alert working for integration with legacy alertmanager behaviour.


Could you provide more clear instruction what should be done here and when?

Actually I can't. We still have GRAFANA integration which might or might not have legacy alertmanager behaviour and that's why there is no clear instruction what to do. I'll remove TODO.

iskhakov · 2023-08-01T04:28:45Z

engine/apps/api/serializers/alert_receive_channel.py

+        # if has_legacy_prefix(integration):
+        #     raise BadRequest(detail="This integration is deprecated")


bumping this

iskhakov · 2023-08-01T04:30:22Z

engine/apps/integrations/templates/heartbeat_instructions/legacy_alertmanager.html

can be removed

iskhakov · 2023-08-01T04:30:29Z

engine/apps/integrations/templates/html/integration_legacy_grafana_alerting.html

can be removed

iskhakov · 2023-08-01T04:31:22Z

engine/apps/integrations/tests/test_legacy_am.py

please don't use abbreviations as they are reducing code readability

iskhakov · 2023-08-01T04:35:08Z

engine/apps/integrations/views.py

+        alerts = request.data.get("alerts", [])
+
+        data = request.data
+        if "firingAlerts" not in request.data:


consider adding a check for max_alerts (or limit alerts), to avoid processing alerts from misconfigured alertmanagers

It won't help us. If we receive request which suits our size limits, iteration over array which already in memory is almost free

iskhakov · 2023-08-01T04:37:15Z

engine/config_integrations/alertmanager.py

@@ -1,38 +1,50 @@
 # Main
 enabled = True
-title = "Alertmanager"
+title = "AlertManager"


keep "Alertmanager", this is how it called by the authors https://prometheus.io/docs/alerting/latest/alertmanager/

iskhakov · 2023-08-01T04:38:16Z

engine/config_integrations/alertmanager.py


-# Slack
+# Slack templates


let's be consistent with the comments

iskhakov · 2023-08-01T04:38:34Z

engine/config_integrations/alertmanager.py


 slack_image_url = None

-# SMS


Let's be consistent with the comments

iskhakov · 2023-08-01T04:39:23Z

engine/settings/base.py

@@ -65,7 +65,7 @@
 FEATURE_INBOUND_EMAIL_ENABLED = getenv_boolean("FEATURE_INBOUND_EMAIL_ENABLED", default=False)
 FEATURE_PROMETHEUS_EXPORTER_ENABLED = getenv_boolean("FEATURE_PROMETHEUS_EXPORTER_ENABLED", default=False)
 FEATURE_WEBHOOKS_2_ENABLED = getenv_boolean("FEATURE_WEBHOOKS_2_ENABLED", default=True)
-FEATURE_SHIFT_SWAPS_ENABLED = getenv_boolean("FEATURE_SHIFT_SWAPS_ENABLED", default=False)
+FEATURE_SHIFT_SWAPS_ENABLED = getenv_boolean("FEATURE_SHIFT_SWAPS_ENABLED", default=True)


Introduce AlertManager v2 integration with improved internal behaviour it's using grouping from AlertManager, not trying to re-group alerts on OnCall side. Existing AlertManager and Grafana Alerting integrations are marked as Legacy with options to migrate them manually now or be migrated automatically after DEPRECATION DATE(TBD). Integration urls and public api responses stay the same both for legacy and new integrations. --------- Co-authored-by: Rares Mardare <rares.mardare@grafana.com> Co-authored-by: Joey Orlando <joey.orlando@grafana.com>

Konstantinov-Innokentii and others added 21 commits July 24, 2023 15:21

First pass on AlertManager v2

4d5fa44

- Introduce legacy_alertmanager/grafana_alerting integrations - Hanlde legacy/v2 integrations in public API – Handle alerts from legacy/v2 integrations on same urls

Introduce legacy_prefix package

1a24597

Merge branch 'dev' into grafana_alerting_v2

a3d41fe

Use set literal for INTEGRATION_TYPES

bd731f9

Add migrate endpoint for integration

7d9c9e7

Temporary allow creating legacy integrations

1422bd1

Merge branch 'dev' into amv2

1f685ef

Update CHANGELOG.md

22f79e6

Merge remote-tracking branch 'origin/amv2' into amv2

802dd63

# Conflicts: # CHANGELOG.md

Decouple GrafanaAPIView from AlertManagerAPIView

cb18375

based_on_alertmanager naming

342cfaf

Hack to keep demo alert working for integration with legacy AM behaviour

06b865a

Add migration

fe903b7

frontend changes

2c4084c

fixed updateItems by passing the page param

629f46c

isLegacy check

13b6a83

Draft docs

4d29d48

Docs iteration

0234c71

Docs polishing

58d7f59

Docs polishing

2c7940a

Add annotations to payload example

f200d95

Konstantinov-Innokentii marked this pull request as ready for review July 28, 2023 05:41

Konstantinov-Innokentii requested a review from a team as a code owner July 28, 2023 05:41

Konstantinov-Innokentii requested review from a team July 28, 2023 05:41

Konstantinov-Innokentii added 5 commits July 28, 2023 13:41

Merge branch 'dev' into amv2

f5cdbf5

Text polishing

cbe4328

Fix typos

052ae46

Merge remote-tracking branch 'origin/dev' into amv2

046636a

Temporary remove migration

a67f92f

teodosii and others added 12 commits July 28, 2023 12:25

frontend changes

05e4ee8

linter

d6d91e6

ui display changes

fef74ea

Polishing

6e7253d

Merge branch 'dev' into amv2

166cf97

Fix tests

dc468c1

Comments polishing

54cbfa6

Add migration

c17dd28

Docs polishing

d9b70d8

Comment polishing

38e20c3

Refresh templates on migration

16da5c1

Merge branch 'dev' into amv2

283282d

joeyorlando reviewed Jul 31, 2023

View reviewed changes

Konstantinov-Innokentii and others added 11 commits August 1, 2023 09:54

Update docs/sources/integrations/alertmanager/index.md

f4630bc

Co-authored-by: Joey Orlando <joey.orlando@grafana.com>

Merge branch 'dev' into amv2

029abef

Rename migrateChannelFiter to migrateChannel

58ffe1b

Remove excess function

a228f83

Skip test_related_shifts

c08937c

Fix migration

5fe2dbb

Polishing

a5c05ad

Add migration Date on frontend

b6bc830

Fix tests

237cce1

Skip test_related_shifts

4a01a81

Update CHANGELOG.md

595e7ac

Konstantinov-Innokentii enabled auto-merge August 1, 2023 04:15

Konstantinov-Innokentii disabled auto-merge August 1, 2023 04:15

Konstantinov-Innokentii merged commit 1ccb9d6 into dev Aug 1, 2023
25 checks passed

Konstantinov-Innokentii deleted the amv2 branch August 1, 2023 04:18

iskhakov reviewed Aug 1, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AlertManager v2 #2643

AlertManager v2 #2643

Konstantinov-Innokentii commented Jul 26, 2023 •

edited

Loading

joeyorlando left a comment

joeyorlando Jul 31, 2023

joeyorlando Jul 31, 2023

joeyorlando Jul 31, 2023

joeyorlando Jul 31, 2023

Konstantinov-Innokentii Aug 1, 2023

joeyorlando Jul 31, 2023

joeyorlando Jul 31, 2023

Konstantinov-Innokentii Aug 1, 2023

joeyorlando Jul 31, 2023

teodosii Aug 1, 2023

joeyorlando Jul 31, 2023

Konstantinov-Innokentii Aug 1, 2023

joeyorlando Jul 31, 2023

Konstantinov-Innokentii Aug 1, 2023 •

edited

Loading

teodosii Aug 1, 2023

iskhakov Aug 1, 2023 •

edited

Loading

Konstantinov-Innokentii Aug 1, 2023

iskhakov Aug 1, 2023

iskhakov Aug 1, 2023

iskhakov Aug 1, 2023

iskhakov Aug 1, 2023

iskhakov Aug 1, 2023

Konstantinov-Innokentii Aug 1, 2023

iskhakov Aug 1, 2023

iskhakov Aug 1, 2023

iskhakov Aug 1, 2023

iskhakov Aug 1, 2023

Konstantinov-Innokentii Aug 1, 2023

		@@ -25,6 +25,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

		- Apply swap requests details to schedule events ([#2677](https://github.com/grafana/oncall/pull/2677))

		- Rework of AlertManager integration ([#2643](https://github.com/grafana/oncall/pull/2643))

		> Integrations that were created before version VERSION are marked as (Legacy).
		> These integrations are still receiving and escalating alerts but will be automatically migrated after DEPRECATION_DATE.

	value = remove_legacy_prefix(value)
	return value
	return remove_legacy_prefix(value)

		if integration == AlertReceiveChannel.INTEGRATION_GRAFANA_ALERTING:
		# TODO: probably only needs to check if unified alerting is on

		# if has_legacy_prefix(integration):
		# raise BadRequest(detail="This integration is deprecated")

AlertManager v2 #2643

AlertManager v2 #2643

Conversation

Konstantinov-Innokentii commented Jul 26, 2023 • edited Loading

joeyorlando left a 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

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

Choose a reason for hiding this comment

Konstantinov-Innokentii Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iskhakov Aug 1, 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

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

Choose a reason for hiding this comment

Konstantinov-Innokentii commented Jul 26, 2023 •

edited

Loading

Konstantinov-Innokentii Aug 1, 2023 •

edited

Loading

iskhakov Aug 1, 2023 •

edited

Loading