Fix/ppl monitor test notification

[riysaxen-amzn]

Description

Bug 1: "Send test message" always fails for PPL monitors.

ConfigureActionsPpl.sendTestMessage had no MONITOR_TYPE.PPL case in its monitor-type switch, so PPL monitors fell into the default: branch, which serializes a v1 query-level monitor — a top-level trigger with a painless condition.script and a match_all search input — and POSTs it to the v1 _execute API. The backend correctly rejects that combination with:

Incompatible trigger [<id>] for monitor type [ppl_monitor]

Fix: PPL monitors now build the test payload with buildPPLMonitorFromFormik — the same serializer used to save them, so the trigger shape (number_of_results etc.) is correct by construction — keep only the action under test, force an always-firing num_results_condition: '>=' / num_results_value: 0 condition, and POST to /api/alerting/v2/monitors/_execute. The v1 path for query/bucket/doc-level monitors is unchanged.

Bug 2: the v2 _execute OSD route forwarded the body untranslated.

The engine only speaks the v1 monitor format; PplAlertingMonitorService.createMonitor/updateMonitor translate via toV1MonitorBody, but executeMonitor forwarded { ppl_monitor: {...} } raw, which the engine's Monitor.parse rejects with Monitor name is null.

Fix: apply the same toV1MonitorBody translation in executeMonitor.

Bug 3: monitor updates could silently erase the PPL query.

toV1MonitorBody defaults a missing query to '', so any update request whose body lacked the query silently persisted an empty query — surfacing to users as "updating a monitor drops the PPL query".

Fix: updateMonitor now rejects query-less bodies with an explicit error instead of writing an empty query.

Also:

• Rejected fetches (e.g. route-validation 400s) in the test-message path now surface as an error toast via backendErrorNotification instead of only console.error.
• The v2 _execute call no longer passes dryrun, which is not part of the v2 route's query schema and failed validation.

Testing

• 10 new unit tests:
  • public/pages/CreateTrigger/containers/ConfigureActions/ConfigureActionsPpl.test.js — v2 endpoint dispatch for PPL monitors, ppl_monitor payload shape (no v1 top-level triggers / painless condition / match_all inputs), forced always-firing condition with only the tested action retained, v1 endpoint regression for query-level monitors, error handling.
  • server/services/PplAlertingMonitorService.test.js — update rejected when query is missing or whitespace-only (engine never called), valid update translated to the v1 engine format, execute body translated to v1 (top-level name, ppl_input inputs, ppl_trigger-wrapped triggers).
• Manually verified against OpenSearch 3.7 with the security plugin: the engine parses and executes the translated payload (triggered: true) via a direct _execute call; the update guard returns the rejection message and leaves the stored query intact; error toasts render in the UI.

Related Issues

n/a — found while investigating PPL monitor notification failures. Happy to file tracking issues if maintainers prefer.

Check List

• New functionality includes testing.
• New functionality has been documented.
• Commits are signed per the DCO using --signoff.

[riysaxen-amzn]

mac/linux fail is a due to platform-dependent dependency hoisting, not related to this PR

[eirsep]

Re: (monitor update erasing the PPL query)

The guard is a good defensive fix — it stops the silent data loss by rejecting query-less
update bodies. But it's shifting the failure mode, not fixing the root cause: we go from "update silently wipes the query" to "update returns a 4xx saying the query is empty." If something upstream is actually sending update bodies without the query, the user still can't save — they just get an error instead of silent corruption. And, we don't have an RCA for why empty query is being passed from ux

[riysaxen-amzn]

Re: (monitor update erasing the PPL query)

The guard is a good defensive fix — it stops the silent data loss by rejecting query-less update bodies. But it's shifting the failure mode, not fixing the root cause: we go from "update silently wipes the query" to "update returns a 4xx saying the query is empty." If something upstream is actually sending update bodies without the query, the user still can't save — they just get an error instead of silent corruption. And, we don't have an RCA for why empty query is being passed from ux

@eirsep Fair challenge on the missing RCA — tracked it down. There is a concrete producer of query-less/empty-query update bodies, and it's the edit-page hydration on the 3.x release lines:

The producer (3.5/3.7 release lines): CreateMonitor.js hydrates the edit form with

initialValues.pplQuery = initialValues.pplQuery || _.get(monitorToEdit, 'ppl_monitor.query') || '';

but the v2 GET /monitors/{id} OSD route returns the monitor flattened (flattenV1Monitor lifts the query to top-level monitor.query; there is no ppl_monitor key in the response). So on edit, pplQuery hydrates to '', and when the user saves any unrelated change, buildPPLMonitorFromFormik serializes query: '' → toV1MonitorBody's query: pplMon.query || '' persisted it. That's the "update drops the PPL query" report: a response-shape mismatch between the fetch route and the edit hydration.

Why it doesn't reproduce on main: the hydration was since reworked — getInitialValues routes PPL monitors through pplAlertingMonitorToFormik, which reads both shapes (monitor.query || inputs[0].ppl_input.query). So on this branch/main the known producer is already fixed.

Why the guard still belongs on main: (1) a PPL monitor with an empty query is never a valid save — there's no legitimate caller intent the rejection could break; (2) the update route is also reachable from quick-action paths (MonitorDetailsV2.updateMonitor merges {...fetchedMonitor, ...update}) and direct API callers, where any future shape drift reintroduces the silent wipe; (3) failing loud converts a silent-data-loss class into an immediately diagnosable error. So: root cause fixed in the hydration (already on main), guard retained as the invariant that the class can't recur silently.

Happy to split the guard into its own PR if you'd prefer this one stay scoped to the test-notification fix.

[lezzago]

Nit (maintainability): the test-message result-handling block is duplicated.

sendTestMessageForPplMonitor (ConfigureActionsPpl.js ~lines 285-311) replicates the checkForError → success-toast → backendErrorNotification logic almost verbatim from sendTestMessage (~lines 383-401). Any future change to how test-message results are handled now has to be made in two places and they'll drift.

Consider extracting a shared helper, e.g. handleTestMessageResponse(response, action, flattenedDestinations, notifications), and calling it from both paths.

Not blocking — just flagging to keep the two paths from diverging.

[riysaxen-amzn]

Nit (maintainability): the test-message result-handling block is duplicated.

sendTestMessageForPplMonitor (ConfigureActionsPpl.js ~lines 285-311) replicates the checkForError → success-toast → backendErrorNotification logic almost verbatim from sendTestMessage (~lines 383-401). Any future change to how test-message results are handled now has to be made in two places and they'll drift.

Consider extracting a shared helper, e.g. handleTestMessageResponse(response, action, flattenedDestinations, notifications), and calling it from both paths.

Not blocking — just flagging to keep the two paths from diverging.

@lezzago Good call — extracted handleTestMessageResponse(response, action) and both paths now share it (e91c645). Kept the catch blocks as-is so the v1 path's behavior is unchanged.