From 93fee4228de22a798135148ad15c9edd57574ed5 Mon Sep 17 00:00:00 2001 From: Dominik Broj Date: Tue, 5 Dec 2023 13:43:48 +0100 Subject: [PATCH 1/3] Rename integrations table tabs (#3501) # What this PR does Rename "Connections" tab to "Monitoring Systems" and "Direct Paging" to "Manual Direct Paging" on Integrations page ## Which issue(s) this PR fixes https://github.com/grafana/oncall-private/issues/2302 ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required) --- CHANGELOG.md | 7 +++--- .../integrations/integrationsTable.test.ts | 10 +++++---- .../src/pages/integrations/Integrations.tsx | 22 +++++++++---------- grafana-plugin/src/plugin.json | 2 +- 4 files changed, 22 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c712805..89dff346 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Disallow creating and deleting direct paging integrations by @vadimkerr ([#3475](https://github.com/grafana/oncall/pull/3475)) +- Renamed "Connections" tab to "Monitoring Systems" and "Direct Paging" to "Manual Direct Paging" on Integrations page ## v1.3.70 (2023-12-01) @@ -63,9 +64,9 @@ Minor bugfixes + dependency updates :) ### Added - Add ability to use Grafana Service Account Tokens for OnCall API (This is only enabled for resolution_notes -endpoint currently) @mderynck ([#3189](https://github.com/grafana/oncall/pull/3189)) + endpoint currently) @mderynck ([#3189](https://github.com/grafana/oncall/pull/3189)) - Add ability for webhook presets to mask sensitive headers @mderynck -([#3189](https://github.com/grafana/oncall/pull/3189)) + ([#3189](https://github.com/grafana/oncall/pull/3189)) ### Changed @@ -74,7 +75,7 @@ endpoint currently) @mderynck ([#3189](https://github.com/grafana/oncall/pull/31 ### Fixed - Fixed issue that blocked saving webhooks with presets if the preset is controlling the URL @mderynck -([#3189](https://github.com/grafana/oncall/pull/3189)) + ([#3189](https://github.com/grafana/oncall/pull/3189)) - User filter doesn't display current value on Alert Groups page ([1714](https://github.com/grafana/oncall/issues/1714)) - Remove displaying rotation modal for Terraform/API based schedules - Filters polishing ([3183](https://github.com/grafana/oncall/issues/3183)) diff --git a/grafana-plugin/e2e-tests/integrations/integrationsTable.test.ts b/grafana-plugin/e2e-tests/integrations/integrationsTable.test.ts index 85408fbd..28b1a3b9 100644 --- a/grafana-plugin/e2e-tests/integrations/integrationsTable.test.ts +++ b/grafana-plugin/e2e-tests/integrations/integrationsTable.test.ts @@ -2,7 +2,9 @@ import { test } from '../fixtures'; import { generateRandomValue } from '../utils/forms'; import { createIntegration, searchIntegrationAndAssertItsPresence } from '../utils/integrations'; -test('Integrations table shows data in Connections and Direct Paging tabs', async ({ adminRolePage: { page } }) => { +test('Integrations table shows data in Monitoring Systems and Direct Paging tabs', async ({ + adminRolePage: { page }, +}) => { const ID = generateRandomValue(); const WEBHOOK_INTEGRATION_NAME = `Webhook-${ID}`; const ALERTMANAGER_INTEGRATION_NAME = `Alertmanager-${ID}`; @@ -22,7 +24,7 @@ test('Integrations table shows data in Connections and Direct Paging tabs', asyn await page.getByRole('tab', { name: 'Tab Integrations' }).click(); // Create 1 Direct Paging integration if it doesn't exist - await page.getByRole('tab', { name: 'Tab Direct Paging' }).click(); + await page.getByRole('tab', { name: 'Tab Manual Direct Paging' }).click(); const integrationsTable = page.getByTestId('integrations-table'); await page.waitForTimeout(2000); const isDirectPagingAlreadyCreated = (await integrationsTable.getByText('Direct paging').count()) >= 1; @@ -37,7 +39,7 @@ test('Integrations table shows data in Connections and Direct Paging tabs', asyn } await page.getByRole('tab', { name: 'Tab Integrations' }).click(); - // By default Connections tab is opened and newly created integrations are visible except Direct Paging one + // By default Monitoring Systems tab is opened and newly created integrations are visible except Direct Paging one await searchIntegrationAndAssertItsPresence({ page, integrationsTable, integrationName: WEBHOOK_INTEGRATION_NAME }); await searchIntegrationAndAssertItsPresence({ page, @@ -52,7 +54,7 @@ test('Integrations table shows data in Connections and Direct Paging tabs', asyn }); // Then after switching to Direct Paging tab only Direct Paging integration is visible - await page.getByRole('tab', { name: 'Tab Direct Paging' }).click(); + await page.getByRole('tab', { name: 'Tab Manual Direct Paging' }).click(); await searchIntegrationAndAssertItsPresence({ page, integrationsTable, diff --git a/grafana-plugin/src/pages/integrations/Integrations.tsx b/grafana-plugin/src/pages/integrations/Integrations.tsx index f9b102a7..b4c30e26 100644 --- a/grafana-plugin/src/pages/integrations/Integrations.tsx +++ b/grafana-plugin/src/pages/integrations/Integrations.tsx @@ -58,7 +58,7 @@ import { PAGE, TEXT_ELLIPSIS_CLASS } from 'utils/consts'; import styles from './Integrations.module.scss'; enum TabType { - Connections = 'connections', + MonitoringSystems = 'monitoring-systems', DirectPaging = 'direct-paging', } @@ -66,11 +66,11 @@ const TAB_QUERY_PARAM_KEY = 'tab'; const TABS = [ { - label: 'Connections', - value: TabType.Connections, + label: 'Monitoring Systems', + value: TabType.MonitoringSystems, }, { - label: 'Direct Paging', + label: 'Manual Direct Paging', value: TabType.DirectPaging, }, ]; @@ -106,7 +106,7 @@ class Integrations extends React.Component integrationsFilters: { searchTerm: '', integration_ne: ['direct_paging'] }, errorData: initErrorDataState(), confirmationModal: undefined, - activeTab: props.query[TAB_QUERY_PARAM_KEY] || TabType.Connections, + activeTab: props.query[TAB_QUERY_PARAM_KEY] || TabType.MonitoringSystems, }; } @@ -204,8 +204,8 @@ class Integrations extends React.Component const { alertReceiveChannelStore } = store; const { count, results, page_size } = alertReceiveChannelStore.getPaginatedSearchResult(); - const isDirectPagingSelectedOnConnectionsTab = - activeTab === TabType.Connections && integrationsFilters.integration?.includes('direct_paging'); + const isDirectPagingSelectedOnMonitoringSystemsTab = + activeTab === TabType.MonitoringSystems && integrationsFilters.integration?.includes('direct_paging'); return ( <> @@ -253,7 +253,7 @@ class Integrations extends React.Component skipFilterOptionFn: ({ name }) => name === 'integration', })} /> - {isDirectPagingSelectedOnConnectionsTab && ( + {isDirectPagingSelectedOnMonitoringSystemsTab && ( alertReceiveChannelStore, filtersStore: { applyLabelFilter }, } = this.props.store; - const isConnectionsTab = this.state.activeTab === TabType.Connections; + const isMonitoringSystemsTab = this.state.activeTab === TabType.MonitoringSystems; const columns = [ { @@ -578,7 +578,7 @@ class Integrations extends React.Component key: 'datasource', render: (item: AlertReceiveChannel) => this.renderDatasource(item, alertReceiveChannelStore), }, - ...(isConnectionsTab + ...(isMonitoringSystemsTab ? [ { width: '10%', @@ -595,7 +595,7 @@ class Integrations extends React.Component ] : []), { - width: isConnectionsTab ? '15%' : '30%', + width: isMonitoringSystemsTab ? '15%' : '30%', title: 'Team', render: (item: AlertReceiveChannel) => this.renderTeam(item, grafanaTeamStore.items), }, diff --git a/grafana-plugin/src/plugin.json b/grafana-plugin/src/plugin.json index b4920482..2dc75d5b 100644 --- a/grafana-plugin/src/plugin.json +++ b/grafana-plugin/src/plugin.json @@ -57,7 +57,7 @@ { "type": "page", "name": "Integrations", - "path": "/a/grafana-oncall-app/integrations?tab=connections", + "path": "/a/grafana-oncall-app/integrations?tab=monitoring-systems", "role": "Viewer", "action": "grafana-oncall-app.integrations:read", "addToNav": true From 2bb80b487e849782037c0d14867eac23ffb45ce8 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 5 Dec 2023 12:12:08 -0500 Subject: [PATCH 2/3] address issue with metrics calculations when redis cluster is used (#3510) ## Which issue(s) this PR fixes Fixes this issue we started seeing popping up because of a change introduced in #3496: ```python3 File "/etc/app/apps/metrics_exporter/views.py", line 22, in get result = generate_latest(application_metrics_registry).decode("utf-8") ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/site-packages/prometheus_client/exposition.py", line 198, in generate_latest for metric in registry.collect(): File "/usr/local/lib/python3.11/site-packages/prometheus_client/registry.py", line 97, in collect yield from collector.collect() File "/etc/app/apps/metrics_exporter/metrics_collectors.py", line 56, in collect alert_groups_total, missing_org_ids_1 = self._get_alert_groups_total_metric(org_ids) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/etc/app/apps/metrics_exporter/metrics_collectors.py", line 97, in _get_alert_groups_total_metric org_id_from_key = RE_ALERT_GROUPS_TOTAL.match(org_key).groups()[0] ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AttributeError: 'NoneType' object has no attribute 'groups' ``` ```python3 >>> import re >>> ALERT_GROUPS_TOTAL = "oncall_alert_groups_total" >>> _RE_BASE_PATTERN = r"{{?{}}}?_(\d+)" >>> RE_ALERT_GROUPS_TOTAL = re.compile(_RE_BASE_PATTERN.format(ALERT_GROUPS_TOTAL)) >>> org_key = "{oncall_alert_groups_total}_1" >>> RE_ALERT_GROUPS_TOTAL.match(org_key).groups()[0] '1' >>> org_key = "oncall_alert_groups_total_1" >>> RE_ALERT_GROUPS_TOTAL.match(org_key).groups()[0] '1' ``` ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [ ] Documentation added (or `pr:no public docs` PR label added if not required) - [ ] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required) --- .../metrics_exporter/metrics_collectors.py | 10 ++-- .../apps/metrics_exporter/tests/conftest.py | 6 +-- .../tests/test_metrics_collectors.py | 46 +++++++++++-------- 3 files changed, 36 insertions(+), 26 deletions(-) diff --git a/engine/apps/metrics_exporter/metrics_collectors.py b/engine/apps/metrics_exporter/metrics_collectors.py index 27d65b12..45ff38ed 100644 --- a/engine/apps/metrics_exporter/metrics_collectors.py +++ b/engine/apps/metrics_exporter/metrics_collectors.py @@ -28,9 +28,13 @@ from apps.metrics_exporter.tasks import start_calculate_and_cache_metrics, start application_metrics_registry = CollectorRegistry() -RE_ALERT_GROUPS_TOTAL = re.compile(r"{}_(\d+)".format(ALERT_GROUPS_TOTAL)) -RE_ALERT_GROUPS_RESPONSE_TIME = re.compile(r"{}_(\d+)".format(ALERT_GROUPS_RESPONSE_TIME)) -RE_USER_WAS_NOTIFIED_OF_ALERT_GROUPS = re.compile(r"{}_(\d+)".format(USER_WAS_NOTIFIED_OF_ALERT_GROUPS)) +# _RE_BASE_PATTERN allows for optional curly-brackets around the metric name as in some cases this may occur +# see common.cache.ensure_cache_key_allocates_to_the_same_hash_slot for more details regarding this +_RE_BASE_PATTERN = r"{{?{}}}?_(\d+)" + +RE_ALERT_GROUPS_TOTAL = re.compile(_RE_BASE_PATTERN.format(ALERT_GROUPS_TOTAL)) +RE_ALERT_GROUPS_RESPONSE_TIME = re.compile(_RE_BASE_PATTERN.format(ALERT_GROUPS_RESPONSE_TIME)) +RE_USER_WAS_NOTIFIED_OF_ALERT_GROUPS = re.compile(_RE_BASE_PATTERN.format(USER_WAS_NOTIFIED_OF_ALERT_GROUPS)) # https://github.com/prometheus/client_python#custom-collectors diff --git a/engine/apps/metrics_exporter/tests/conftest.py b/engine/apps/metrics_exporter/tests/conftest.py index f82ee211..f2868ee4 100644 --- a/engine/apps/metrics_exporter/tests/conftest.py +++ b/engine/apps/metrics_exporter/tests/conftest.py @@ -22,11 +22,11 @@ METRICS_TEST_USER_USERNAME = "Alex" @pytest.fixture() def mock_cache_get_metrics_for_collector(monkeypatch): def _mock_cache_get(key, *args, **kwargs): - if key.startswith(ALERT_GROUPS_TOTAL): + if ALERT_GROUPS_TOTAL in key: key = ALERT_GROUPS_TOTAL - elif key.startswith(ALERT_GROUPS_RESPONSE_TIME): + elif ALERT_GROUPS_RESPONSE_TIME in key: key = ALERT_GROUPS_RESPONSE_TIME - elif key.startswith(USER_WAS_NOTIFIED_OF_ALERT_GROUPS): + elif USER_WAS_NOTIFIED_OF_ALERT_GROUPS in key: key = USER_WAS_NOTIFIED_OF_ALERT_GROUPS test_metrics = { ALERT_GROUPS_TOTAL: { diff --git a/engine/apps/metrics_exporter/tests/test_metrics_collectors.py b/engine/apps/metrics_exporter/tests/test_metrics_collectors.py index 4dbe7d05..670c590b 100644 --- a/engine/apps/metrics_exporter/tests/test_metrics_collectors.py +++ b/engine/apps/metrics_exporter/tests/test_metrics_collectors.py @@ -1,6 +1,7 @@ from unittest.mock import patch import pytest +from django.test import override_settings from prometheus_client import CollectorRegistry, generate_latest from apps.alerts.constants import AlertGroupState @@ -12,29 +13,34 @@ from apps.metrics_exporter.constants import ( from apps.metrics_exporter.metrics_collectors import ApplicationMetricsCollector +# redis cluster usage modifies the cache keys for some operations, so we need to test both cases +# see common.cache.ensure_cache_key_allocates_to_the_same_hash_slot for more details +@pytest.mark.parametrize("use_redis_cluster", [True, False]) @patch("apps.metrics_exporter.metrics_collectors.get_organization_ids", return_value=[1]) @patch("apps.metrics_exporter.metrics_collectors.start_calculate_and_cache_metrics.apply_async") @pytest.mark.django_db def test_application_metrics_collector( - mocked_org_ids, mocked_start_calculate_and_cache_metrics, mock_cache_get_metrics_for_collector + mocked_org_ids, mocked_start_calculate_and_cache_metrics, mock_cache_get_metrics_for_collector, use_redis_cluster ): """Test that ApplicationMetricsCollector generates expected metrics from cache""" - collector = ApplicationMetricsCollector() - test_metrics_registry = CollectorRegistry() - test_metrics_registry.register(collector) - for metric in test_metrics_registry.collect(): - if metric.name == ALERT_GROUPS_TOTAL: - # integration with labels for each alert group state - assert len(metric.samples) == len(AlertGroupState) - elif metric.name == ALERT_GROUPS_RESPONSE_TIME: - # integration with labels for each value in collector's bucket + _count and _sum histogram values - assert len(metric.samples) == len(collector._buckets) + 2 - elif metric.name == USER_WAS_NOTIFIED_OF_ALERT_GROUPS: - # metric with labels for each notified user - assert len(metric.samples) == 1 - result = generate_latest(test_metrics_registry).decode("utf-8") - assert result is not None - assert mocked_org_ids.called - # Since there is no recalculation timer for test org in cache, start_calculate_and_cache_metrics must be called - assert mocked_start_calculate_and_cache_metrics.called - test_metrics_registry.unregister(collector) + + with override_settings(USE_REDIS_CLUSTER=use_redis_cluster): + collector = ApplicationMetricsCollector() + test_metrics_registry = CollectorRegistry() + test_metrics_registry.register(collector) + for metric in test_metrics_registry.collect(): + if metric.name == ALERT_GROUPS_TOTAL: + # integration with labels for each alert group state + assert len(metric.samples) == len(AlertGroupState) + elif metric.name == ALERT_GROUPS_RESPONSE_TIME: + # integration with labels for each value in collector's bucket + _count and _sum histogram values + assert len(metric.samples) == len(collector._buckets) + 2 + elif metric.name == USER_WAS_NOTIFIED_OF_ALERT_GROUPS: + # metric with labels for each notified user + assert len(metric.samples) == 1 + result = generate_latest(test_metrics_registry).decode("utf-8") + assert result is not None + assert mocked_org_ids.called + # Since there is no recalculation timer for test org in cache, start_calculate_and_cache_metrics must be called + assert mocked_start_calculate_and_cache_metrics.called + test_metrics_registry.unregister(collector) From a1edc20cbe7f4c71fda0a5a10e3c4d10fe2d4047 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 5 Dec 2023 12:14:19 -0500 Subject: [PATCH 3/3] Update CHANGELOG.md --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 89dff346..f50399e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +## v1.3.72 (2023-12-05) + +### Fixed + +- Address metrics calculation issue which occurred when `USE_REDIS_CLUSTER` env var was set by @joeyorlando ([#3510](https://github.com/grafana/oncall/pull/3510)) + ## v1.3.71 (2023-12-05) ### Added