PPL Alerting 3.3 by KashKondaka · Pull Request #1310 · opensearch-project/alerting-dashboards-plugin

KashKondaka · 2025-11-07T18:11:31Z

Description

PPL Alerting UI Changes

Create ppl monitor flyout in discover
v1/v2 coexistence in alerts dashboard and monitors dashboard pages
V2 Trigger graph, monitor details page, create monitor flow, alerts flyout page

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…ject#1295) (cherry picked from commit a232821) Signed-off-by: opensearch-ci <opensearch-infra@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…opensearch-project#1297) (cherry picked from commit 147e2e7) Signed-off-by: Peter Zhu <zhujiaxi@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Revert "fix mustache format notif message" This reverts commit 862d635. update tests Signed-off-by: KashKondaka <37753523+KashKondaka@users.noreply.github.com>

amsiglan · 2025-11-07T18:46:01Z

+      sortDirection,
+      search,
+      severityLevel,
+      monitorIds,


Is this something new or existing behavior to support both array and a string as possible input?

this is existing behavior

amsiglan · 2025-11-07T18:47:01Z

+  }
+
+  /** ---------- NEW: generic PPL query passthrough (/_plugins/_ppl) ---------- */
+  pplQuery = async (context, req, res) => {


what does pplQuery mean? Does it generate it/run it? Let's name it accordingly

ppl query is the actual query we submit to /_plugins/_ppl api call. We then use the api response later. Maybe proxyPplQuery might be better suited since it is forwarding the api response?

amsiglan · 2025-11-07T18:48:06Z

+  };
+  /** ------------------------------------------------------------------------ */
+
+    listIndices = async (context, req, res) => {


Are we not doing this in some other service? If so, let's dedupe this

We do have OpensearchService.getIndices, but that endpoint is a POST that requires an index body parameter and returns the full _cat/indices payload (health/status/etc.). The new UI flow just needs the bare names via a GET so the browser can call it without the extra params. b/c of the shape differences, reusing the existing handler would require another wrapper anyway, so keeping this focused helper avoids coupling the two contracts.

amsiglan · 2025-11-07T18:50:45Z

+      // If the data source cluster doesn't support v2 alerts endpoint, try the default cluster
+      if (isNoHandlerError(err) && req.query?.dataSourceId) {
+        try {
+          console.warn('[alertsForMonitorsV2] Data source cluster does not support v2 alerts, falling back to default cluster');


Let's use logger instead of console.warn so that we can look at those logs later if needed

amsiglan · 2025-11-07T18:52:04Z

+      // If the data source cluster doesn't support v2 alerts endpoint, try the default cluster
+      if (isNoHandlerError(err) && req.query?.dataSourceId) {
+        try {
+          console.warn('[alertsForMonitorsV2] Data source cluster does not support v2 alerts, falling back to default cluster');


Why are we falling back here when the ask is for the V2 alerts and the backend doesn't have it?
We should simply return an error, no?

yes will return error, was old code when debugging

amsiglan · 2025-11-07T18:53:42Z

+     const qs = new URLSearchParams();
+     if (Number.isFinite(Number(ifSeqNo))) qs.append('if_seq_no', String(ifSeqNo));
+     if (Number.isFinite(Number(ifPrimaryTerm))) qs.append('if_primary_term', String(ifPrimaryTerm));
+     const path = `/_plugins/_alerting/v2/monitors/${encodeURIComponent(id)}${qs.toString() ? `?${qs}` : ''}`;


Shouldn't this also be qs.toString()?

yes, I missed that

amsiglan · 2025-11-07T18:54:31Z

+      const monitor =
+        _.get(raw, 'monitor_v2.ppl_monitor') ||
+        _.get(raw, 'ppl_monitor') ||
+        _.get(raw, 'monitor') ||
+        _.get(raw, '_source') ||
+        {};


Why do we have so many possibilities with the response? Shouldn't it be defined what to expect?

amsiglan · 2025-11-07T18:55:25Z

+        headers: DEFAULT_HEADERS,
+      });
+
+      console.log("response:", resp);


remove the console.logs and instead use logger if you need it (like important warnings, errors)

amsiglan · 2025-11-07T18:56:54Z

+       if (all.length) {
+         // Filter by state / severity / search (very light client-side filter)
+         const filtered = all.filter((a) => {
+           const stateOk = alertState === 'ALL' ? true : String(a.state).toUpperCase() === String(alertState).toUpperCase();
+           const sevOk = severityLevel === 'ALL' ? true : String(a.severity).toUpperCase() === String(severityLevel).toUpperCase();
+           const text = `${a.trigger_name ?? ''} ${a.monitor_name ?? ''} ${a.message ?? ''}`.toLowerCase();
+           const searchOk = !search || text.includes(String(search).toLowerCase());
+           return stateOk && sevOk && searchOk;
+         });
+
+         // Sort & paginate
+         const dir = String(sortDirection).toLowerCase() === 'asc' ? 1 : -1;
+         const key = sortField || 'start_time';
+         filtered.sort((x, y) => (x[key] === y[key] ? 0 : (x[key] > y[key] ? dir : -dir)));
+         const totalAlerts = filtered.length;
+         const page = filtered.slice(Number(from) || 0, (Number(from) || 0) + (Number(size) || 50));


Shouldn't these get handled in the alerting backend APIs? Not scalable here

amsiglan · 2025-11-07T18:57:38Z

+       }
+     }
+
+     // Fallback 1: old v2 endpoint (if it exists)


I am concerned why we have fallbacks?

amsiglan · 2025-11-07T18:58:13Z

    }
  };

  deleteMonitor = async (context, req, res) => {
    try {
      const { id } = req.params;
-      const params = { monitorId: id };
+      console.log("Deleting monitor id: ", id);


remove the console.logs

amsiglan · 2025-11-07T19:01:46Z

+      const client = this.getClientBasedOnDataSource(context, req);
+
+      // Route to v2 update if payload is v2/PPL
+      if (isV2MonitorPayload(req.body)) {


if we have a separate update method for ppl monitor, this shouldn't be changed

amsiglan · 2025-11-07T19:03:15Z

+  };
+
+  // Get v1 (classic/legacy) monitors only - excludes v2 PPL monitors
+  getMonitorsV1 = async (context, req, res) => {


why is this changing?

amsiglan · 2025-11-07T19:08:00Z

+  // v1 monitors (classic/legacy monitors)
+  router.get(
+    {
+      path: '/api/alerting/monitors/v1',


why are we adding new APIs for the existing experience?

amsiglan · 2025-11-07T19:08:50Z

-          version: schema.number(),
-        }),
+        params: schema.object({ id: schema.string() }),
+        // ⬇️ Same here.


redundant comment?

amsiglan · 2025-11-07T19:09:55Z

-          version: schema.number(),
-        }),
+        params: schema.object({ id: schema.string() }),
+        // ⬇️ Make version optional so the same route can delete v2 monitors (no version) or legacy (with version).


remove now?

AWSHurneyt · 2025-11-07T19:24:32Z

@KashKondaka could you run the cypress tests locally to make sure they pass, and add the output in this PR as a comment? That should help give us a little more confidence that no existing features were broken by these changes.

Looks like the cypress workflow is failing in this PR because the backend is still pending deployment.

AWSHurneyt · 2025-11-08T01:09:22Z

+          label={
+            <span>
+              Use classic monitors{' '}
+              <EuiToolTip content="Use pre-existing monitor types available in classic alerts.">


Would it make more sense to say available in classic Alerting?

AWSHurneyt · 2025-11-08T01:13:53Z

+                id="useClassicMonitorsHeader"
+                label={
+                  <span>
+                    Use classic monitors{' '}


Looks like there are multiple Use classic monitors checkboxes in this file. Could you clarify their differences?

AWSHurneyt · 2025-11-08T01:18:46Z

      monitor_type: MONITOR_TYPE.COMPOSITE_LEVEL,
      workflow_type: MONITOR_TYPE.COMPOSITE_LEVEL,
-      schema_version: 0,
+      //schema_version: 0,


Should this still be commented out?

AWSHurneyt · 2025-11-08T01:21:11Z

      triggers: [],
      ui_metadata: {
-        schedule: uiSchedule,
+        schedule: formikToUiSchedule(values),


Was this needed? Looks like uiSchedule is already created by calling formikToUiSchedule(values).

AWSHurneyt · 2025-11-08T01:21:23Z

    triggers: [],
    ui_metadata: {
-      schedule: uiSchedule,
+      schedule: formikToUiSchedule(values),


Same comment as above.

AWSHurneyt · 2025-11-08T01:33:10Z

-      let query = _.get(values, 'query', '');
-      try {
-        query = JSON.parse(query);
-        description = _.get(query, 'description', description);


The changes below always set description: FORMIK_INITIAL_VALUES.description. Was there a reason to move away from this switch block?

AWSHurneyt · 2025-11-08T01:37:05Z

+        sessionStorage.removeItem(transferKey);
+      }
+    } catch (err) {
+      console.error('[getInitialValues] Failed to load query from sessionStorage:', err);


Should this be bubbled up to the user via a toast?

It would be worthwhile to consider looking at all catch blocks to see if we should add toasts instead of console logs.

AWSHurneyt · 2025-11-08T01:39:44Z

+ * Small service wrapper that calls the server (proxy) API for V2 routes.
+ * (Preview is handled via /_plugins/_ppl; only create/update live here.)
+ */
+export const makeAlertingV2Service = (httpClient) => {


Not blocking:
We should consider moving this to a separate services file/directory.

AWSHurneyt · 2025-11-08T01:43:01Z

+ * Build the Monitor V2 (PPL) payload expected by backend.
+ * Shape: { "ppl_monitor": { name, enabled, schedule, query, triggers, look_back_window_minutes?, timestamp_field? } }
+ */
+export const buildPPLMonitorFromFormik = (values) => {


Not blocking:
Should move formik conversions to a separate file specific to ppl alerting to help clean up this helpers file.

AWSHurneyt · 2025-11-08T01:46:04Z

-  if (!monitor) return formikValues;
+  if (!monitorIn) return formikValues;
+
+  // Accept v2 wrappers transparently (try both camelCase and snake_case)


Could you clarify why we need to look for both camelCase and snakeCase?

AWSHurneyt · 2025-11-08T01:48:19Z

        <div>
-          <OverviewStat header={'Perform action'} value={'Per monitor execution'} />
-          <EuiSpacer size={'s'} />
+          {/* <OverviewStat header={'Perform action'} value={'Per monitor execution'} />


Should these still be commented out? This would make them unavailable for v1 monitors, wouldn't it?

AWSHurneyt · 2025-11-08T02:14:29Z

+                    <EuiCompressedSelect
+                      name={field.name}
+                      value={safeValue}
+                      options={NUMBER_OF_RESULTS_OPERATOR_OPTIONS}


Just to confirm, NUMBER_OF_RESULTS_OPERATOR_OPTIONS has all of the same entries as THRESHOLD_ENUM_OPTIONS, right? I believe bucket monitors use this too, so I want to make sure those aren't impacted.

AWSHurneyt · 2025-11-08T02:17:10Z

    this.loadDestinations();
  }

+  // async componentDidMount() {


Is this still needed?

AWSHurneyt · 2025-11-08T02:18:27Z


    this.setState({ loadingDestinations: true });
    try {
-      const response = await httpClient.get('../api/alerting/destinations', {


Could you clarify why this was removed? We didn't deprecate the alerting plugin destinations in 3.0 to my knowledge.

AWSHurneyt · 2025-11-08T02:29:19Z

-              {isBucketLevelMonitor ? (
-                <FieldArray name={'triggerConditions'} validateOnChange={true}>
-                  {(arrayHelpers) => (
-                    <DefineBucketLevelTrigger


I don't see DefineBucketLevelTrigger in the DefineTriggerV1 file. Does this get used elsewhere now?

AWSHurneyt · 2025-11-08T02:31:52Z

      subject_template: {
        lang: 'mustache',
-        source: 'Monitor {{ctx.monitor.name}} triggered an alert {{ctx.trigger.name}}',
+        source: 'Monitor {{ctx.monitorV2.name}} triggered an alert {{ctx.trigger.name}}',


Could you confirm this template was supposed to be changed? The file path suggests this is a composite monitor asset, and composite monitors wouldn't use v2 naming patterns, right?

AWSHurneyt · 2025-11-08T02:34:06Z

-  {{#ctx.completedAlerts}}
-    * {{id}} : {{bucket_keys}}
-  {{/ctx.completedAlerts}}
+    Monitor {{ctx.monitorV2.name}} just entered alert status. Please investigate the issue.


Were these templates intended to be changed? Looks like they're associated with query and bucket monitors.

AWSHurneyt · 2025-11-08T07:40:31Z

  { value: 'ALL', text: 'All alerts' },
  { value: ALERT_STATE.ACTIVE, text: 'Active' },
-  { value: ALERT_STATE.ACKNOWLEDGED, text: 'Acknowledged' },
-  { value: ALERT_STATE.COMPLETED, text: 'Completed' },


Removing these entries completely would impact v1 monitors, wouldn't it?

AWSHurneyt · 2025-11-08T07:42:57Z

      totalAlerts: 0,
      totalTriggers: 0,
-      chainedAlert: undefined,
+      //chainedAlert: undefined,


Is this intended to be commented out still? Would this impact composite monitors?

AWSHurneyt · 2025-11-08T07:47:43Z

      const { httpClient, history, notifications, perAlertView } = this.props;
      history.replace({ ...this.props.location, search: queryParamsString });
-      const extendedParams = {
-        ...(dataSourceId !== undefined && { dataSourceId }), // Only include dataSourceId if it exists


I see that params gets used a little further down in the apiParams variable; but it doesn't look like dataSourceId doesn't seem to get used. Is it no longer needed?

AWSHurneyt · 2025-11-08T08:03:32Z

+        size: monitorIds.length || 1000,
      };

-      const response = await httpClient.post('../api/alerting/monitors/_search', {


The v2 endpoint doesn't return v1 monitors, right? Should there be a condition here to determine which endpoint to use?

AWSHurneyt · 2025-11-08T08:04:38Z

-  openChainedAlertsFlyout = (chainedAlert) => {
-    this.setState({ chainedAlert });
-  };
+  // openChainedAlertsFlyout = (chainedAlert) => {


These sections that are commented out are needed for composite monitors aren't they?

AWSHurneyt · 2025-11-08T08:05:11Z

      alertsByTriggers,
      alertState,
-      chainedAlert,
+      //chainedAlert,


Same question; isn't this needed for composite monitors?

AWSHurneyt · 2025-11-08T08:05:36Z

-              },
-            ],
-          });
+//           -          columns.push({


Same question; isn't this needed for composite monitors?

AWSHurneyt · 2025-11-08T08:14:19Z

    return (
      <>
-        {chainedAlert && (
+        {/* {chainedAlert && (


Does this need to be re-enabled in classic mode?

AWSHurneyt · 2025-11-08T08:17:42Z

            monitorType={monitorType}
-            alertActions={useUpdatedUx ? actions() : undefined}
-            panelStyles={{ padding: perAlertView ? '8px 0px 16px' : '0px 0px 16px' }}
+            alertActions={null}


Should this only be null in non-classic mode?

AWSHurneyt · 2025-11-08T08:34:25Z

+  // },
+  // {
+  //   field: 'acknowledged_time',
+  //   name: 'Time acknowledged',


Should this be added for classic mode?

AWSHurneyt · 2025-11-08T08:35:40Z

  const dataSources = getDataSources(monitor, localClusterName);

  const overviewStats = [
-    {


Would we want these sections for classic mode?

AWSHurneyt · 2025-11-08T09:02:36Z

-  public setup(core: CoreSetup<AlertingStartDeps, AlertingStart>, { expressions, uiActions, dataSourceManagement, dataSource, assistantDashboards }: AlertingSetupDeps) {
+  public setup(core: CoreSetup<AlertingStartDeps, AlertingStart>, { expressions, uiActions, dataSourceManagement, dataSource, assistantDashboards, explore }: AlertingSetupDeps) {

+    // const mountWrapper = async (params: AppMountParameters, redirect: string) => {


There are a few commented out sections in this file. Are they still needed?

AWSHurneyt · 2025-11-08T09:04:46Z

+  } else if (errorMessage) {
+    messageText = String(errorMessage);
+  } else {
+    messageText = 'An unknown error occurred';


Not blocking:
An error message should end with a full-stop (i.e., .).

opensearch-trigger-bot Bot and others added 3 commits October 7, 2025 13:03

add explore as optional plugin

5cb04ef

Revert "fix mustache format notif message" This reverts commit 862d635. update tests Signed-off-by: KashKondaka <37753523+KashKondaka@users.noreply.github.com>

KashKondaka requested review from AWSHurneyt, amsiglan, bowenlan-amzn, eirsep, getsaurabh02, lezzago, praveensameneni, rishabhmaurya, riysaxen-amzn and sbcd90 as code owners November 7, 2025 18:11

amsiglan reviewed Nov 7, 2025

View reviewed changes

AWSHurneyt reviewed Nov 8, 2025

View reviewed changes

Uh oh!

Conversation

KashKondaka commented Nov 7, 2025

Description

Issues Resolved

Check List

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AWSHurneyt commented Nov 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AWSHurneyt Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AWSHurneyt Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

AWSHurneyt Nov 8, 2025 •

edited

Loading

AWSHurneyt Nov 8, 2025 •

edited

Loading