-
Notifications
You must be signed in to change notification settings - Fork 69
OU-1408: Add create alert kebab item in metrics view #1187
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,7 +76,7 @@ import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; | |
| import type { FC, MouseEvent as ReactMouseEvent, Ref } from 'react'; | ||
| import { useTranslation } from 'react-i18next'; | ||
| import { useDispatch, useSelector } from 'react-redux'; | ||
| import { useSearchParams } from 'react-router'; | ||
| import { useNavigate, useSearchParams } from 'react-router'; | ||
| import { StringParam, useQueryParam } from 'use-query-params'; | ||
|
|
||
| import { DropDownPollInterval } from '@/shared/components/DropdownPollInterval'; | ||
|
|
@@ -97,7 +97,11 @@ import { MonitoringProvider } from '@/shared/contexts/MonitoringContext'; | |
| import { useBoolean } from '@/shared/hooks/useBoolean'; | ||
| import { useMonitoring } from '@/shared/hooks/useMonitoring'; | ||
| import { useMonitoringNamespace } from '@/shared/hooks/useMonitoringNamespace'; | ||
| import { getObserveState } from '@/shared/hooks/usePerspective'; | ||
| import { | ||
| getCreateAlertRuleUrl, | ||
| getObserveState, | ||
| usePerspective, | ||
| } from '@/shared/hooks/usePerspective'; | ||
| import { | ||
| queryBrowserAddQuery, | ||
| queryBrowserDeleteAllQueries, | ||
|
|
@@ -385,6 +389,9 @@ const SeriesButton: FC<SeriesButtonProps> = ({ index, labels }) => { | |
| const QueryKebab: FC<{ index: number }> = ({ index }) => { | ||
| const { t } = useTranslation(process.env.I18N_NAMESPACE); | ||
| const { plugin } = useMonitoring(); | ||
| const { perspective } = usePerspective(); | ||
| const navigate = useNavigate(); | ||
| const [activeNamespace] = useActiveNamespace(); | ||
|
|
||
| const isDisabledSeriesEmpty = useSelector((state: MonitoringState) => | ||
| _.isEmpty(getObserveState(plugin, state).queryBrowser?.queries[index]?.disabledSeries), | ||
|
|
@@ -427,6 +434,10 @@ const QueryKebab: FC<{ index: number }> = ({ index }) => { | |
| dispatch(queryBrowserDuplicateQuery(index)); | ||
| }, [dispatch, index]); | ||
|
|
||
| const doCreateAlert = useCallback(() => { | ||
| navigate(getCreateAlertRuleUrl(perspective, query ?? '', activeNamespace)); | ||
| }, [navigate, perspective, query, activeNamespace]); | ||
|
Comment on lines
+437
to
+439
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -euo pipefail
# Verify whether queryBrowserPatchQuery updates only `text` during editing,
# and whether queryBrowserRunQueries later copies it into `query`.
rg -n -C 8 --glob '*.{ts,tsx}' \
'queryBrowserPatchQuery|queryBrowserRunQueries|text: value|query:.*text' web/srcRepository: openshift/monitoring-plugin Length of output: 24230 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect the alert action's bound state and the query editor's props at the
# changed file, plus the repository conventions and any scoped learnings.
printf '%s\n' '--- MetricsPage alert/navigation context ---'
sed -n '360,455p;900,990p' web/src/features/metrics/pages/MetricsPage.tsx
printf '%s\n' '--- QueryKebab references and definitions ---'
rg -n -C 12 --glob '*.{ts,tsx}' 'QueryKebab|doCreateAlert|getCreateAlertRuleUrl' web/src/features/metrics web/src/shared
printf '%s\n' '--- scoped conventions/learnings ---'
head -5 /tmp/coderabbit-repo-knowledge/openshift-monitoring-plugin-24f216f8/*/*.mdRepository: openshift/monitoring-plugin Length of output: 30948 Pass the edited query to the alert form. Before execution, editing updates 🤖 Prompt for AI Agents |
||
|
|
||
| const isSpan = (item) => item?.title?.props?.children; | ||
| const getSpanText = (item) => item.title.props.children; | ||
|
|
||
|
|
@@ -549,6 +560,14 @@ const QueryKebab: FC<{ index: number }> = ({ index }) => { | |
| > | ||
| {t('Duplicate query')} | ||
| </DropdownItem>, | ||
| <DropdownItem | ||
| key="create-alert" | ||
| component="button" | ||
| onClick={doCreateAlert} | ||
| data-test={DataTestIDs.MetricsPageCreateAlertRuleDropdownItem} | ||
| > | ||
| {t('Create alert')} | ||
| </DropdownItem>, | ||
| ]; | ||
|
|
||
| const hasQueryTableData = () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,4 +13,5 @@ export enum QueryParams { | |
| Refresh = 'refresh', | ||
| Start = 'start', | ||
| Edit = 'edit', | ||
| Query = 'query', | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| jest.mock('@openshift-console/dynamic-plugin-sdk', () => ({ | ||
| ...jest.requireActual('@openshift-console/dynamic-plugin-sdk/lib/api/common-types'), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure why are we mocking here and then requiring an internal package |
||
| })); | ||
|
|
||
| import { getCreateAlertRuleUrl } from '@/shared/hooks/usePerspective'; | ||
|
|
||
| describe('getCreateAlertRuleUrl', () => { | ||
| const query = 'up{job="prometheus"}'; | ||
| const encodedQuery = 'query=up%7Bjob%3D%22prometheus%22%7D'; | ||
|
|
||
| it('builds the admin url', () => { | ||
| expect(getCreateAlertRuleUrl('admin', query)).toBe( | ||
| `/monitoring/v2/alertrule/create?${encodedQuery}`, | ||
| ); | ||
| }); | ||
|
|
||
| it('builds the virtualization url', () => { | ||
| expect(getCreateAlertRuleUrl('virtualization-perspective', query)).toBe( | ||
| `/virt-monitoring/v2/alertrule/create?${encodedQuery}`, | ||
| ); | ||
| }); | ||
|
|
||
| it('builds the acm url', () => { | ||
| expect(getCreateAlertRuleUrl('acm', query)).toBe( | ||
| `/multicloud/monitoring/v2/alertrule/create?${encodedQuery}`, | ||
| ); | ||
| }); | ||
|
|
||
| it('builds the dev url with the namespace in the path', () => { | ||
| expect(getCreateAlertRuleUrl('dev', query, 'my-project')).toBe( | ||
| `/dev-monitoring/ns/my-project/v2/alertrule/create?${encodedQuery}`, | ||
| ); | ||
| }); | ||
|
|
||
| it('falls back to the admin url for an unknown perspective', () => { | ||
| expect(getCreateAlertRuleUrl('unknown' as never, query)).toBe( | ||
| `/monitoring/v2/alertrule/create?${encodedQuery}`, | ||
| ); | ||
| }); | ||
|
|
||
| it('defaults to an empty query when none is provided', () => { | ||
| expect(getCreateAlertRuleUrl('admin')).toBe('/monitoring/v2/alertrule/create?query='); | ||
| }); | ||
|
|
||
| it('url-encodes the query parameter', () => { | ||
| expect(getCreateAlertRuleUrl('admin', 'a b&c=d')).toBe( | ||
| '/monitoring/v2/alertrule/create?query=a+b%26c%3Dd', | ||
| ); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -319,3 +319,21 @@ export const getDashboardsListUrl = (perspective: Perspective) => { | |
| return ''; | ||
| } | ||
| }; | ||
|
|
||
| // TODO: The dev and acm routes below are best-guesses based on the existing perspective URL | ||
| // conventions. Confirm them with the new alert management UI once its routing is finalized. | ||
| export const getCreateAlertRuleUrl = (perspective: Perspective, query = '', namespace?: string) => { | ||
| const params = new URLSearchParams({ [QueryParams.Query]: query }); | ||
|
|
||
| switch (perspective) { | ||
| case 'acm': | ||
| return `/multicloud/monitoring/v2/alertrule/create?${params.toString()}`; | ||
| case 'dev': | ||
| return `/dev-monitoring/ns/${namespace}/v2/alertrule/create?${params.toString()}`; | ||
| case 'virtualization-perspective': | ||
| return `/virt-monitoring/v2/alertrule/create?${params.toString()}`; | ||
| case 'admin': | ||
| default: | ||
| return `/monitoring/v2/alertrule/create?${params.toString()}`; | ||
|
Comment on lines
+323
to
+337
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect route declarations and existing alert-rule navigation paths.
rg -n -C 4 --glob '*.{ts,tsx}' \
'alertrule/create|v2/alertrule|dev-monitoring/ns|multicloud/monitoring' webRepository: openshift/monitoring-plugin Length of output: 17364 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- route/resource definitions ---'
rg -n -C 6 --glob '*.{ts,tsx,js,jsx,json,yaml,yml}' \
'AlertResource|RuleResource|alertrules|alertrule/create|/alerts|route.*alert|alert.*route' web/src web/package.json package.json 2>/dev/null
printf '%s\n' '--- relevant source ---'
sed -n '1,115p' web/src/shared/hooks/usePerspective.tsx
sed -n '315,342p' web/src/shared/hooks/usePerspective.tsx
sed -n '1,120p' web/src/shared/utils/utils.ts 2>/dev/null || trueRepository: openshift/monitoring-plugin Length of output: 50383 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- plugin route registration files ---'
git ls-files | rg -i '(^|/)(route|routes|plugin|console).*(ts|tsx|json|yaml|yml)$|manifest|package\.json$' | head -200
printf '%s\n' '--- route registration references ---'
rg -n -C 3 --glob '*.{ts,tsx,json,yaml,yml}' \
'AlertsPage|AlertRulesPage|AlertingPage|AlertRulesDetailsPage|SilencesPage|/dev-monitoring|/multicloud/monitoring|monitoring/alertrules' \
. ':!web/src/shared/hooks/usePerspective.tsx' ':!web/src/shared/hooks/get-create-alert-rule-url.spec.ts' \
2>/dev/null | head -240Repository: openshift/monitoring-plugin Length of output: 17180 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- ACM alerting routes ---'
sed -n '15,112p' config/acm-alerting.patch.json
printf '%s\n' '--- developer and virtualization alerting routes ---'
sed -n '120,330p' config/alerting.patch.json
printf '%s\n' '--- all create-route declarations ---'
rg -n -C 3 --glob '*.json' \
'alertrule/create|alert.*create|create.*alert' config web
printf '%s\n' '--- applicable repository guidance and learnings ---'
head -5 /tmp/coderabbit-repo-knowledge/openshift-monitoring-plugin-24f216f8/*/*.md 2>/dev/null || trueRepository: openshift/monitoring-plugin Length of output: 28637 🌐 Web query:
💡 Result: In OpenShift, there is no direct user-facing "v2/alertrule/create" command or endpoint for creating alert rules through the console UI. The OpenShift Container Platform web console facilitates alerting management through the Monitoring UI, but the actual creation of alerting rules is performed by applying custom resource definitions, typically PrometheusRule objects (for user-defined projects) [1][2][3] or AlertingRule objects (for platform-level monitoring in the openshift-monitoring namespace) [4][5][6]. To create an alerting rule for a project, follow these guidelines: 1. Permissions: Ensure your user has the necessary permissions, such as the monitoring-rules-edit role, which allows you to manage alerting rules within the Developer perspective [4][3]. 2. Resource Creation: - For user-defined projects: Create a PrometheusRule object in the specific namespace where your application resides [1][2][3]. - For cluster-level platform monitoring: Create an AlertingRule object in the openshift-monitoring namespace [4][5][6]. 3. Method: You can apply these YAML configurations using the oc command-line interface (e.g., oc apply -f .yaml) [3] or by using the Import YAML feature in the OpenShift web console. While you may see internal references to paths like /dev-monitoring/ns//alertrules in the console's source code or URL structure [7][8], these represent internal routes for the Monitoring UI to display existing rules, not public API endpoints for creating them [7]. The management of these rules is handled by the Kubernetes API via the monitoring.coreos.com/v1 (PrometheusRule) or monitoring.openshift.io/v1 (AlertingRule) APIs [5][2][3]. Citations:
🌐 Web query:
💡 Result: The term alertrule/create does not correspond to a standard, widely recognized public API endpoint or command across major cloud providers or monitoring systems. Based on technical documentation and source code repositories (specifically regarding OpenShift and Prometheus monitoring), alertrule, AlertRule, or AlertRules are concepts used to describe alerting configurations [1][2]. These are typically defined as monitoring resources or configurations rather than objects created via a dedicated alertrule/create endpoint [1]. In contexts like OpenShift, alerting rules are generally managed through custom resource definitions (CRDs), configuration files, or Prometheus rules management, rather than a single direct "create" operation [1][3]. If you encountered this term, it is likely: 1. An internal API path, route, or custom function specific to a particular application's codebase [4]. 2. A shorthand representation or variable name within a specific monitoring tool or developer console interface [1][3]. For further clarification, please check the specific API documentation or repository for the platform you are interacting with (e.g., Kubernetes/OpenShift monitoring, Prometheus, or a proprietary cloud service). Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- AlertingPage route handling ---'
sed -n '55,125p' web/src/features/alerts/pages/AlertingPage.tsx
printf '%s\n' '--- complete route paths for alerting ---'
python3 - <<'PY'
import json
from pathlib import Path
for path in (Path('config/acm-alerting.patch.json'), Path('config/alerting.patch.json')):
data = json.loads(path.read_text())
print(path)
for item in data:
value = item.get('value', {})
if value.get('type') == 'console.page/route':
props = value.get('properties', {})
print(' ', props.get('path'))
PY
printf '%s\n' '--- reviewed change context ---'
git diff --stat -- web/src/shared/hooks/usePerspective.tsx
git diff -- web/src/shared/hooks/usePerspective.tsx | sed -n '1,180p'Repository: openshift/monitoring-plugin Length of output: 3173 Replace the guessed ACM and developer URLs with registered route contracts.
🤖 Prompt for AI Agents |
||
| } | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to create an alert without a query?