PPL Alerting: PPL Monitor Execution

[toepkerd]

Description

This PR primarily contains logic for running PPL Monitors. It also adds RBAC checks for manual Monitor executions, a gap revealed by a security review.

To support RBAC filtering for manual monitor execution, a new field manual is added to ExecuteMonitorRequest/Response. Many of the file changes are simply to accommodate this new field and can be ignored in review.

The only files that need deep review are:

TriggerService.kt
InputService.kt
AlertService.kt
QueryLevelMonitorRunner.kt
TransportExecuteMonitorAction.kt
QueryLevelTriggerExecutionContext.kt

New manual Field

The manual field in ExecuteMonitorRequest distinguishes between user-initiated executions (via the REST API) and system-initiated executions (via the job scheduler or MonitorJobPoller). When a user calls the Execute Monitor API, the REST handler sets manual = true. When the scheduler or poller triggers execution internally, it sets manual = false. In TransportExecuteMonitorAction, this field gates the RBAC check: when manual is true and the monitor was fetched by ID, checkUserPermissionsWithResource is called to verify the calling user's backend roles overlap with the monitor owner's backend roles (enforcing filter_by_backend_roles). When manual is false, this check is skipped entirely — scheduled executions run as the system and must not be blocked by user-level RBAC. This ensures that monitors continue to fire on schedule regardless of which user created them, while still preventing unauthorized users from manually triggering another user's monitor via the API.

Check List

• New functionality includes testing.
• New functionality has been documented.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff.
• Public documentation issue/PR created.

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.

[lezzago]

Can you explain the difference between the two and how the value differs for ppl monitors?

[toepkerd]

Despite both being List<Map<String, Any>>, the shape/structure of the data inside is different between the two, so the code can't assume it can parse/read that field the same way in either a PPL or DSL Monitor run.

Example results value (used by Query-Level Monitors and other DSL-based Monitors):

[
    {
      "_shards": {"total": 1, "failed": 0, "successful": 1, "skipped": 0},
      "hits": {
        "hits": [{"_id": "abc", "_source": {"status": 500, "endpoint": "/api"}}],
        "total": {"value": 3, "relation": "eq"},
        "max_score": 1.0
      },
      "took": 45,
      "timed_out": false
    }
  ]

Example pplQueryResults value (used only by PPL Monitors):

[
    {"endpoint": "/api/orders", "error_count": 3},
    {"endpoint": "/api/checkout", "error_count": 1}
]

[toepkerd]

Will add comment explaining the difference between these fields.

[lezzago]

Why are we removing this?

[toepkerd]

it's replaced with a per-trigger max execution duration setting as opposed to a per-monitor max execution duration. Each custom trigger can run its own query, so the timeboxing is done at a per trigger level instead of a per monitor level.

[lezzago]

Instead should we call the get monitor api with the user's context, so it would automatically validate correctly if the user has permissions to even fetch the monitor as the get monitor api would do the RBAC check.

[toepkerd]

I'm not sure about this approach. Get Monitor calls checkUserPermissionsWithResource just like ExecuteMonitor is doing here (reference), adding an extra hop from ExecuteMonitor to GetMonitor to call checkUserPermissionsWithResource doesn't seem very clean when ExecuteMonitor can do that itself.

[toepkerd]

Exploring running checkUserPermissionsWithResource() directly, without checking manual field first.

[lezzago]

Rename this to pplQuery and pplQueryResults if this is only meant for ppl monitors.

[toepkerd]

This naming is used so that the query and query results in Alerts specifically can be included for more than just PPL Monitors, in response to this feedback from common-utils. This generalization (of creating a field that either PPL Monitors or existing Monitor types can use) only exists for Alerts currently. In internal Monitor execution logic, a distinction is made between DSL results and PPL results because their shapes are different despite both being List<Map<String, Any>>. See: #2130 (comment).

[toepkerd]

Need to clarify comment that these fields are for PPL Monitors for now, but can be extended for other Monitor types to use.

[lezzago]

if it times out, do we want any logging and catching the exception?

[toepkerd]

That is already done. QueryLevelMonitorRunner calls collectInputResultsForPPLMonitor(), which wraps runPPLBaseQuery() in a try catch. collectInputResultsForPPLMonitor()'s try catch catches the timeout exception (or any other exception), logs an error log, then returns an InputRunResults with a non-empty error. This causes the generation of an ERROR alert.

[eirsep]

why are these changes appearing as an addition from this CR?

[toepkerd]

This must have been an oversight on my part when fixing merge conflicts. ExternalSchedulerService.initialize(settings) is already called on line 393 (and it's not showing up as an addition in the PR). Will remove this.

[eirsep]

if there are no numResults triggers skip runnin base query

[eirsep]

i would bat for PPL monitor to have only 1 custom PPL trigger and if it has any custom trigger then it shouldn't have a num results trigger.. but it can have multiple num_results triggers in the same monitor.

running one monitor sequentially would be a nightmare because partial failures would be tough to ahndle and if the job crahes after processing 2 out 5 custom triggers, then when it retries there would be duplicate

just an open discussion..

[toepkerd]

Alerts are collected while PPL Triggers are run (just like Query-Level Triggers), then bulk saved all at once after all Triggers are executed. If the job crashes in the middle of processing its Triggers, no Alerts would have been written. This means no duplicate Alerts when the PPL Monitor retries.

[eirsep]

What is the timeout value.. also what is max number of PPL triggers allowed?
is x less than 5 minutes.

Am asking because the monitor cannot run longer than 5 minutes in total since the job scheduler lock expires and will fire a duplicate job

[toepkerd]

Will reduce max ppl query duration setting default value from 1m to 30s, and add a new max ppl triggers setting whose max value is 10.

[lezzago]

Did you mean to delete the commented code?