PPL Alerting: Adding PPL Related Models

[toepkerd]

Description

Changes to support PPL Alerting behind existing (V1) Alerting APIs, and to support PPL Alerting using stateful Alerts instead of stateless Alerts.

PPL Monitors will be run by the QueryLevelMonitorRunner (much like AD and Cluster Metrics Monitors), so these changes prepare for that as well.

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]

Do we want to generalize these fields so it could be the any query for also normal query level monitors and what api for cluster metrics? Also the same for the results.

[toepkerd]

The List<Map<String, Any?>> typing would work for both DSL and PPL query results but we'd need to be mindful that the format of the contents are not at all the same. For Query-Level Monitor runs, the List<Map<String, Any?>> would contain exactly 1 element of the DSL query results in its usual format. For PPL Monitor runs, the List<Map<String, Any?>> would be of this format:

 List<Map<String, Any?>> = listOf(                                                                                                                                                   
      mapOf(                                                                                                                                                                          
          "region" to "us-east-1",                                                                                                                                                    
          "max_price" to 450.0,                                                                                                                                                       
          "avg_latency" to 120.5                                                                                                                                                      
      ),                                                                                                                                                                              
      mapOf(                                                                                                                                                                          
          "region" to "us-west-2",                                                                                                                                                    
          "max_price" to 380.0,                                                                                                                                                       
          "avg_latency" to 95.3                                                                                                                                                       
      ),                                                                                                                                                                              
      mapOf(                                                                                                                                                                          
          "region" to "eu-west-1",                                                                                                                                                    
          "max_price" to 520.0,                                                                                                                                                       
          "avg_latency" to 145.8                                                                                                                                                      
      )                                                                                                                                                                               
  ) 

Seeing that the query results in an Alert are only ever meant to be readable by a user that calls Get Alerts, and not accessed with an alert.field call, the generalization should be fine.

[eirsep]

There is a general mistake in how we are doing readFrom() and writeTo() - basically the writeable serialization.
old classes being deserialized or serialized will fail over the wire since they expect these new PPL query results or other PPL specific changes. you need to add some version checks before expecting these to be avaiablle and serialized or deserialized.

[eirsep]

why are we adding this as part of PPL monitor type addition??

how will this be backward compatible?

[toepkerd]

This length check is from a previous Security review of PPL Alerting behind V2 APIs. The name length check is only done on PPL Monitors, it's not going to be done on existing Monitor types.

[eirsep]

experimental is present intermittently? either remove everywhere or add where missing

[toepkerd]

Removing

[eirsep]

why are the serialized field name not "ppl_sql" prefixed similar to class and variable names.
let's remove sql substring everywhere if possible or make the serialized names consistent.

@lezzago thoughts?

[toepkerd]

Fair callout that there is inconsistency between calling it a "PPL" thing or a "PPLSQL" thing. The only trouble with calling everything "PPLSQL" is that the name of the PPL Trigger type has to be ppl_trigger and not ppl_sql_trigger since that would mean users creating PPL Monitors through Dev Tools or programmatically would need to specify that name ppl_sql_trigger, even though we are seeking to hide potential SQL support for now. Our options are:

1. Remove SQL substring from everything, and then live with the fact that if PPL Monitors support SQL queries in the future, they will continue to be called just PPL Monitors and PPL Triggers
2. Add SQL substring to everything, and then let the construct names hint at future SQL support

I'm leaning toward option 1

[lezzago]

Lets remove SQL since if we add SQL support, that would be a new language and we dont need to show that SQL and PPL are tied together in one plugin.

[eirsep]

the readFrom and writeTo() methods don't seem to be consistent... did you add this serde roundtrip in test coverage with new fields and without new fields? if yes, plz respond to this comment with the test methods

[toepkerd]

Only a roundtrip test with new fields, and even then one of the new fields is just null as you pointed out in another comment: WriteableTests.test inputrunresult as stream. Will improve the test coverage to not use a null PPL query results field, and to test with + without new PPL fields separately.

[eirsep]

plz dont add nullable stuff alone. kindly add proper test coverage for inputrunresults serde

[toepkerd]

Will change to use an actual list of maps

[eirsep]

why are we changing this? how or why is this in scope of PPL models related changes?

[toepkerd]

Because PPL Monitors will not have their own dedicated PPLMonitorRunner. They will run on QueryLevelMonitorRunner, much like AD and Cluster Metrics Monitors do. The thing is PPL Triggers need to be their own type because they are fundamentally different from QueryLevelTriggers. As such, the type of trigger had to be generalized to Trigger interface so that it can handle either QueryLevelTrigger or PPLTrigger.

This constructor is called by composeQueryLevelAlert in Alerting. There are validations in Alerting that the trigger passed into this constructor is only ever type QueryLevelTrigger or PPLTrigger.

[lezzago]

change this to be generic

[toepkerd]

I actually believe keeping it PPL is the better approach for this field specifically. Query-Level Monitors run their query once, then apply painless trigger execution on those results. PPL Monitors with a custom trigger conditions append the condition to the base PPL query. If a PPL Monitor has multiple custom triggers, each Trigger will be running a different query.

In other words, Query-Level Monitors run the query per Monitor execution. PPL Monitors run the query per Trigger execution. Having query results specifically in the Trigger Run Result is unique to PPL Monitors.

[lezzago]

Query-Level Monitors run their query once, then apply painless trigger execution on those results. PPL Monitors with a custom trigger conditions append the condition to the base PPL query. If a PPL Monitor has multiple custom triggers, each Trigger will be running a different query.

Should we change this so its similar to query level monitors? First run the base query and then run the trigger condition query ontop of the PPL query? This is to help search costs. This may be more complex or if we can have this is a temp tables for the results and then run the different trigger queries against it, that would be best. Though this would depend on the feasibility with PPL features.

[toepkerd]

This option has been explored and deemed not possible. PPL must support the makeresults key word to make PPL Monitors run in the 2-step process you describe above (1. run base query 2. apply trigger condition to results). Even if this key word was supported, custom condition triggers each run their own query to evaluate trigger condition (base query + custom condition).

That said, optimizations were already done post load testing to run just the base query once, so that number of results triggers need not rerun the base query redundantly. This optimization cannot be done for custom triggers though.

[lezzago]

Lets leave this as an open github issue with PPL plugin for the feature request for performance optimizations. Make sure that pplCustomQueryResults is not customer facing so we could change this variable name down the road.

[toepkerd]

The PPL side feature that would eliminate the need for this extra field already exists: opensearch-project/sql#3629

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 092a85d)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Add PPLInput, PPLTrigger, and PPL Monitor type support Relevant files: src/main/kotlin/org/opensearch/commons/alerting/model/PPLInput.kt src/main/kotlin/org/opensearch/commons/alerting/model/PPLTrigger.kt src/main/kotlin/org/opensearch/commons/alerting/util/IndexUtils.kt src/main/kotlin/org/opensearch/commons/alerting/model/Monitor.kt src/main/kotlin/org/opensearch/commons/alerting/model/Input.kt src/main/kotlin/org/opensearch/commons/alerting/model/Trigger.kt Sub-PR theme: Add PPL query/queryResults fields to Alert, InputRunResults, and TriggerRunResult Relevant files: src/main/kotlin/org/opensearch/commons/alerting/model/Alert.kt src/main/kotlin/org/opensearch/commons/alerting/model/MonitorRunResult.kt src/main/kotlin/org/opensearch/commons/alerting/model/QueryLevelTriggerRunResult.kt Sub-PR theme: Promote asTemplateArg to Trigger interface and add override annotations Relevant files: src/main/kotlin/org/opensearch/commons/alerting/model/BucketLevelTrigger.kt src/main/kotlin/org/opensearch/commons/alerting/model/ChainedAlertTrigger.kt src/main/kotlin/org/opensearch/commons/alerting/model/DocumentLevelTrigger.kt src/main/kotlin/org/opensearch/commons/alerting/model/NoOpTrigger.kt src/main/kotlin/org/opensearch/commons/alerting/model/QueryLevelTrigger.kt src/main/kotlin/org/opensearch/commons/alerting/model/remote/monitors/RemoteMonitorTrigger.kt src/main/kotlin/org/opensearch/commons/alerting/model/Trigger.kt

⚡ Recommended focus areas for review Fallback Parsing The else branch in Input.parse() unconditionally attempts to parse any unknown input type as PPLInput. If an unrecognized input type is encountered, this will produce a confusing error rather than a clear "unsupported input type" message. Consider adding an explicit check for PPL_INPUT and throwing an IllegalArgumentException for truly unknown types. } else { PPLInput.parseInner(xcp) } Length Validation The UUID_LENGTH constant is 20, but the error messages say "length must be less than $UUID_LENGTH" when the check is <=. The message should say "at most" or "less than or equal to" to be accurate. Similarly, ALERTING_MAX_NAME_LENGTH uses <= but the message says "less than". This inconsistency could mislead users. require(this.id.length <= UUID_LENGTH) { "Trigger ID too long, length must be less than $UUID_LENGTH." } require(this.name.length <= ALERTING_MAX_NAME_LENGTH) { "Trigger name too long, length must be less than $ALERTING_MAX_NAME_LENGTH." } XContent Always Emits PPL Fields toXContent always emits ppl_query_results and ppl_num_results fields even for non-PPL monitors (where they will be empty/null). This may pollute the response for existing monitor types and could be a breaking change for consumers parsing the output. .field("ppl_query_results", pplBaseQueryResults) .field("ppl_num_results", pplBaseQueryNumResults) .field("error", error?.message) PPL Input Serialization In writeTo, the else branch for serializing inputs still writes REMOTE_DOC_LEVEL_MONITOR_INPUT for any input that is not SearchInput, DocLevelMonitorInput, or PPLInput. If a ClusterMetricsInput or other input type is used, it will be incorrectly serialized as REMOTE_DOC_LEVEL_MONITOR_INPUT. } else if (it is PPLInput) { out.writeEnum(Input.Type.PPL_INPUT) } else { out.writeEnum(Input.Type.REMOTE_DOC_LEVEL_MONITOR_INPUT) } Parsing Loop Bug In parseInner, after processing each field in the when block, xcp.nextToken() is called unconditionally at line 299. However, for the ACTIONS_FIELD case, the loop already advances the token to END_ARRAY and then calls nextToken() again, which may skip a token. This pattern could cause parsing errors for triggers with actions. while (xcp.nextToken() != XContentParser.Token.END_ARRAY) { actions.add(Action.parse(xcp)) } } else -> throw IllegalArgumentException("Unexpected field when parsing PPL Trigger: $fieldName") } xcp.nextToken()

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 092a85d

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle all input types explicitly during serialization The serialization logic for inputs uses an if/else if/else chain where the final else branch defaults to REMOTE_DOC_LEVEL_MONITOR_INPUT. This means a ClusterMetricsInput or any other input type would be incorrectly serialized as REMOTE_DOC_LEVEL_MONITOR_INPUT, causing deserialization failures. The ClusterMetricsInput type should be explicitly handled. src/main/kotlin/org/opensearch/commons/alerting/model/Monitor.kt [263-267] +if (it is SearchInput) { + out.writeEnum(Input.Type.SEARCH_INPUT) +} else if (it is DocLevelMonitorInput) { + out.writeEnum(Input.Type.DOCUMENT_LEVEL_INPUT) } else if (it is PPLInput) { out.writeEnum(Input.Type.PPL_INPUT) +} else if (it is ClusterMetricsInput) { + out.writeEnum(Input.Type.CLUSTER_METRICS_INPUT) } else { out.writeEnum(Input.Type.REMOTE_DOC_LEVEL_MONITOR_INPUT) } Suggestion importance[1-10]: 8 __ Why: The else branch defaulting to REMOTE_DOC_LEVEL_MONITOR_INPUT would incorrectly serialize ClusterMetricsInput instances, causing deserialization failures for cluster metrics monitors. This is a real correctness bug that existed before this PR but is made more risky by the addition of PPLInput handling.	Medium
	Consolidate repeated version checks into single block The version check sin.version.onOrAfter(Version.V_3_7_0) is called three separate times, which is redundant and could lead to inconsistency if the condition ever changes. Consolidate all three version-gated reads into a single if block to ensure they are always read together (since they were written together) and to avoid any potential desync. src/main/kotlin/org/opensearch/commons/alerting/model/MonitorRunResult.kt [152-167] -val pplCount = if (sin.version.onOrAfter(Version.V_3_7_0)) { - sin.readVInt() -} else { - 0 -} val pplList = mutableListOf<Map<String, Any?>>() +val pplNumResults: Long? if (sin.version.onOrAfter(Version.V_3_7_0)) { + val pplCount = sin.readVInt() for (i in 0 until pplCount) { pplList.add(suppressWarning(sin.readMap())) // pplResults } -} -val pplNumResults = if (sin.version.onOrAfter(Version.V_3_7_0)) { - sin.readOptionalLong() + pplNumResults = sin.readOptionalLong() } else { - null + pplNumResults = null } Suggestion importance[1-10]: 7 __ Why: The three separate sin.version.onOrAfter(Version.V_3_7_0) checks are redundant and could lead to subtle bugs if the reads get out of sync. Consolidating them into a single block is cleaner and safer since the writes are done together in a single version-gated block.	Medium
	Fix parser token advancement for array fields The parser calls xcp.nextToken() at the end of each loop iteration to advance past the value, but for complex value types like arrays (the ACTIONS_FIELD case), the inner while loop already consumes the END_ARRAY token. The extra xcp.nextToken() after the when block will then skip the next field name, causing parsing errors or silently dropping fields. The ACTIONS_FIELD branch should return or the outer nextToken() call should be skipped for array fields. src/main/kotlin/org/opensearch/commons/alerting/model/PPLTrigger.kt [248-300] while (xcp.currentToken() != XContentParser.Token.END_OBJECT) { val fieldName = xcp.currentName() xcp.nextToken() when (fieldName) { - ... + ID_FIELD -> id = xcp.text() + NAME_FIELD -> name = xcp.text() + SEVERITY_FIELD -> severity = xcp.text() + CONDITION_TYPE_FIELD -> { ... } + NUM_RESULTS_CONDITION_FIELD -> { ... } + NUM_RESULTS_VALUE_FIELD -> { ... } + CUSTOM_CONDITION_FIELD -> { ... } + ACTIONS_FIELD -> { + XContentParserUtils.ensureExpectedToken( + XContentParser.Token.START_ARRAY, + xcp.currentToken(), + xcp + ) + while (xcp.nextToken() != XContentParser.Token.END_ARRAY) { + actions.add(Action.parse(xcp)) + } + // Skip the extra nextToken() for array fields by continuing + continue + } else -> throw IllegalArgumentException("Unexpected field when parsing PPL Trigger: $fieldName") } xcp.nextToken() } Suggestion importance[1-10]: 7 __ Why: The outer xcp.nextToken() at line 299 is called after every field parse, but the ACTIONS_FIELD branch already consumes the END_ARRAY token via its inner loop. This extra nextToken() call would skip the next field name token, potentially causing parsing failures or silently dropping fields. This is a real parsing bug that needs to be addressed.	Medium
	Fix nullable query usage after null check After requireNotNull(query), the Kotlin compiler still considers query as String? (nullable), so passing it directly to PPLInput(query, queryLanguage) may cause a compile error or require a non-null assertion. Use requireNotNull(query) { "..." } with a message and assign the result, or use query!! when constructing the object. src/main/kotlin/org/opensearch/commons/alerting/model/PPLInput.kt [102-104] -requireNotNull(query) +val nonNullQuery = requireNotNull(query) { "PPL query must be specified" } -return PPLInput(query, queryLanguage) +return PPLInput(nonNullQuery, queryLanguage) Suggestion importance[1-10]: 4 __ Why: In Kotlin, requireNotNull(query) does perform a smart cast in many cases, but using the return value explicitly (or query!!) is safer and more idiomatic. However, Kotlin's smart cast typically handles this correctly after requireNotNull, so this is a minor style/safety improvement rather than a critical bug.	Low

---

Previous suggestions

Suggestions up to commit 04c0466

Category	Suggestion	Impact
Possible issue	Consolidate repeated version checks into one block The version check sin.version.onOrAfter(Version.V_3_7_0) is performed three separate times, which is redundant and could lead to inconsistency if the condition changes. Consolidate all three reads into a single version check block to ensure they are always read together (since they were written together in writeTo). src/main/kotlin/org/opensearch/commons/alerting/model/MonitorRunResult.kt [152-167] -val pplCount = if (sin.version.onOrAfter(Version.V_3_7_0)) { - sin.readVInt() -} else { - 0 -} val pplList = mutableListOf<Map<String, Any?>>() +val pplNumResults: Long? if (sin.version.onOrAfter(Version.V_3_7_0)) { + val pplCount = sin.readVInt() for (i in 0 until pplCount) { pplList.add(suppressWarning(sin.readMap())) // pplResults } -} -val pplNumResults = if (sin.version.onOrAfter(Version.V_3_7_0)) { - sin.readOptionalLong() + pplNumResults = sin.readOptionalLong() } else { - null + pplNumResults = null } Suggestion importance[1-10]: 7 __ Why: The three separate sin.version.onOrAfter(Version.V_3_7_0) checks are redundant and could cause inconsistency since the fields were written together in writeTo. Consolidating them into a single block is cleaner and safer.	Medium
	Prevent silent misclassification of unknown input types The else branch silently maps any unrecognized Input type (including ClusterMetricsInput and RemoteMonitorInput) to REMOTE_DOC_LEVEL_MONITOR_INPUT, which would cause incorrect deserialization. Each known input type should be handled explicitly, and the else branch should throw an exception for unknown types. src/main/kotlin/org/opensearch/commons/alerting/model/Monitor.kt [263-267] } else if (it is PPLInput) { out.writeEnum(Input.Type.PPL_INPUT) +} else if (it is ClusterMetricsInput) { + out.writeEnum(Input.Type.CLUSTER_METRICS_INPUT) +} else if (it is RemoteMonitorInput) { + out.writeEnum(Input.Type.REMOTE_MONITOR_INPUT) +} else if (it is RemoteDocLevelMonitorInput) { + out.writeEnum(Input.Type.REMOTE_DOC_LEVEL_MONITOR_INPUT) } else { - out.writeEnum(Input.Type.REMOTE_DOC_LEVEL_MONITOR_INPUT) + throw IllegalStateException("Unexpected input type [${it.javaClass}] when writing Monitor") } Suggestion importance[1-10]: 7 __ Why: The else branch silently maps unrecognized Input types (like ClusterMetricsInput or RemoteMonitorInput) to REMOTE_DOC_LEVEL_MONITOR_INPUT, which would cause incorrect deserialization. Adding explicit handling for each known type and throwing for unknown types is an important correctness fix.	Medium
	Fix potential token skipping in actions array parsing The parser loop calls xcp.nextToken() at the end of each iteration to advance past the current field's value. However, for the ACTIONS_FIELD case, the inner while loop already consumes tokens up to END_ARRAY, and then the outer xcp.nextToken() advances one more token — this is correct. But if an unknown field with a complex value (object/array) is encountered, the else -> throw branch fires before consuming the value, which is fine for strict parsing. However, the double nextToken() pattern (one inside the loop start, one at the end) is fragile. Verify that the ACTIONS_FIELD branch does not cause the outer xcp.nextToken() to skip a field, since after END_ARRAY the next token should be a FIELD_NAME or END_OBJECT. src/main/kotlin/org/opensearch/commons/alerting/model/PPLTrigger.kt [255-307] while (xcp.currentToken() != XContentParser.Token.END_OBJECT) { val fieldName = xcp.currentName() xcp.nextToken() when (fieldName) { - ... + ID_FIELD -> id = xcp.text() + NAME_FIELD -> name = xcp.text() + SEVERITY_FIELD -> severity = xcp.text() + CONDITION_TYPE_FIELD -> { + val input = xcp.text() + val enumMatchResult = ConditionType.enumFromString(input) + ?: throw IllegalArgumentException( + "Invalid value for $CONDITION_TYPE_FIELD: $input. " + + "Supported values are ${ConditionType.entries.map { it.value }}" + ) + conditionType = enumMatchResult + } + NUM_RESULTS_CONDITION_FIELD -> { + if (xcp.currentToken() != XContentParser.Token.VALUE_NULL) { + val input = xcp.text() + val enumMatchResult = NumResultsCondition.enumFromString(input) + ?: throw IllegalArgumentException( + "Invalid value for $NUM_RESULTS_CONDITION_FIELD: $input. " + + "Supported values are ${NumResultsCondition.entries.map { it.value }}" + ) + numResultsCondition = enumMatchResult + } + } + NUM_RESULTS_VALUE_FIELD -> { + if (xcp.currentToken() != XContentParser.Token.VALUE_NULL) { + numResultsValue = xcp.longValue() + } + } + CUSTOM_CONDITION_FIELD -> { + if (xcp.currentToken() != XContentParser.Token.VALUE_NULL) { + customCondition = xcp.text() + } + } + ACTIONS_FIELD -> { + XContentParserUtils.ensureExpectedToken( + XContentParser.Token.START_ARRAY, + xcp.currentToken(), + xcp + ) + while (xcp.nextToken() != XContentParser.Token.END_ARRAY) { + actions.add(Action.parse(xcp)) + } + // Do NOT call xcp.nextToken() again at end of iteration for this branch + continue + } + else -> throw IllegalArgumentException("Unexpected field when parsing PPL Trigger: $fieldName") } xcp.nextToken() } Suggestion importance[1-10]: 3 __ Why: The concern about double nextToken() after ACTIONS_FIELD parsing is valid in theory, but looking at the actual code, after the inner while loop ends on END_ARRAY, the outer xcp.nextToken() correctly advances to the next field. The existing pattern is consistent with how other parsers in the codebase work, making this a low-impact suggestion.	Low
General	Fix misleading error messages for length constraints The error messages say "less than" but the checks use <= (less than or equal). This is misleading — the messages should say "at most" or "less than or equal to" to accurately reflect the constraint, consistent with how the Monitor name length check is worded elsewhere. src/main/kotlin/org/opensearch/commons/alerting/model/PPLTrigger.kt [64-70] require(this.id.length <= UUID_LENGTH) { - "Trigger ID too long, length must be less than $UUID_LENGTH." + "Trigger ID too long, length must be at most $UUID_LENGTH." } require(this.name.length <= ALERTING_MAX_NAME_LENGTH) { - "Trigger name too long, length must be less than $ALERTING_MAX_NAME_LENGTH." + "Trigger name too long, length must be at most $ALERTING_MAX_NAME_LENGTH." } Suggestion importance[1-10]: 3 __ Why: The error messages say "less than" but the checks use <=, which is a minor wording inconsistency. This is a low-impact cosmetic fix that improves accuracy of error messages.	Low

Suggestions up to commit d44d5ce

Category	Suggestion	Impact
Possible issue	Consolidate repeated version checks into one block The version check sin.version.onOrAfter(Version.V_3_7_0) is performed three separate times, which is redundant and could lead to inconsistency if the condition changes. Consolidate all three reads into a single version check block to ensure they are always read together (since they were written together in writeTo). src/main/kotlin/org/opensearch/commons/alerting/model/MonitorRunResult.kt [152-167] -val pplCount = if (sin.version.onOrAfter(Version.V_3_7_0)) { - sin.readVInt() -} else { - 0 -} val pplList = mutableListOf<Map<String, Any?>>() +val pplNumResults: Long? if (sin.version.onOrAfter(Version.V_3_7_0)) { + val pplCount = sin.readVInt() for (i in 0 until pplCount) { pplList.add(suppressWarning(sin.readMap())) // pplResults } -} -val pplNumResults = if (sin.version.onOrAfter(Version.V_3_7_0)) { - sin.readOptionalLong() + pplNumResults = sin.readOptionalLong() } else { - null + pplNumResults = null } Suggestion importance[1-10]: 7 __ Why: The three separate sin.version.onOrAfter(Version.V_3_7_0) checks are redundant and could lead to subtle bugs if the condition is changed in one place but not others. Consolidating them into a single block ensures the reads always happen together, matching the writeTo structure.	Medium
	Handle all input types explicitly during serialization The else branch unconditionally writes REMOTE_DOC_LEVEL_MONITOR_INPUT for any input type that is not SearchInput, DocLevelMonitorInput, or PPLInput, including ClusterMetricsInput and RemoteMonitorInput. This means those input types will be incorrectly serialized and fail to deserialize. Each known input type should have its own explicit branch. src/main/kotlin/org/opensearch/commons/alerting/model/Monitor.kt [263-267] } else if (it is PPLInput) { out.writeEnum(Input.Type.PPL_INPUT) +} else if (it is ClusterMetricsInput) { + out.writeEnum(Input.Type.CLUSTER_METRICS_INPUT) +} else if (it is RemoteMonitorInput) { + out.writeEnum(Input.Type.REMOTE_MONITOR_INPUT) } else { out.writeEnum(Input.Type.REMOTE_DOC_LEVEL_MONITOR_INPUT) } Suggestion importance[1-10]: 7 __ Why: The else branch silently maps any unrecognized input type (including ClusterMetricsInput and RemoteMonitorInput) to REMOTE_DOC_LEVEL_MONITOR_INPUT, which would cause incorrect deserialization. Adding explicit branches for all known input types is important for correctness.	Medium
	Fix parser token advancement for array fields The parser calls xcp.nextToken() at the end of each loop iteration, but for the ACTIONS_FIELD case the inner while loop already advances the parser to END_ARRAY, and then the outer xcp.nextToken() advances past the next field name. This misalignment will cause parsing errors or silently skip fields when actions is not the last field. The ACTIONS_FIELD branch should not rely on the outer xcp.nextToken() call, or the loop structure needs to be adjusted to handle this case. src/main/kotlin/org/opensearch/commons/alerting/model/PPLTrigger.kt [255-307] while (xcp.currentToken() != XContentParser.Token.END_OBJECT) { val fieldName = xcp.currentName() xcp.nextToken() when (fieldName) { - ... + ID_FIELD -> id = xcp.text() + NAME_FIELD -> name = xcp.text() + SEVERITY_FIELD -> severity = xcp.text() + CONDITION_TYPE_FIELD -> { + val input = xcp.text() + val enumMatchResult = ConditionType.enumFromString(input) + ?: throw IllegalArgumentException( + "Invalid value for $CONDITION_TYPE_FIELD: $input. " + + "Supported values are ${ConditionType.entries.map { it.value }}" + ) + conditionType = enumMatchResult + } + NUM_RESULTS_CONDITION_FIELD -> { + if (xcp.currentToken() != XContentParser.Token.VALUE_NULL) { + val input = xcp.text() + val enumMatchResult = NumResultsCondition.enumFromString(input) + ?: throw IllegalArgumentException( + "Invalid value for $NUM_RESULTS_CONDITION_FIELD: $input. " + + "Supported values are ${NumResultsCondition.entries.map { it.value }}" + ) + numResultsCondition = enumMatchResult + } + } + NUM_RESULTS_VALUE_FIELD -> { + if (xcp.currentToken() != XContentParser.Token.VALUE_NULL) { + numResultsValue = xcp.longValue() + } + } + CUSTOM_CONDITION_FIELD -> { + if (xcp.currentToken() != XContentParser.Token.VALUE_NULL) { + customCondition = xcp.text() + } + } + ACTIONS_FIELD -> { + XContentParserUtils.ensureExpectedToken( + XContentParser.Token.START_ARRAY, + xcp.currentToken(), + xcp + ) + while (xcp.nextToken() != XContentParser.Token.END_ARRAY) { + actions.add(Action.parse(xcp)) + } + // Do not call xcp.nextToken() again; advance manually here + xcp.nextToken() + continue + } else -> throw IllegalArgumentException("Unexpected field when parsing PPL Trigger: $fieldName") } xcp.nextToken() } Suggestion importance[1-10]: 3 __ Why: The concern about xcp.nextToken() being called after the ACTIONS_FIELD inner loop is valid in principle, but looking at similar parsers in the codebase (e.g., QueryLevelTrigger, BucketLevelTrigger), this pattern is consistently used and works correctly because Action.parse(xcp) leaves the parser positioned at the end of each action object. The improved code uses continue which is not idiomatic for this codebase pattern.	Low
General	Fix misleading error messages for length constraints The error messages say "less than" but the checks use <= (less than or equal). This is misleading — the messages should say "at most" or "less than or equal to" to accurately describe the constraint, consistent with how the Monitor name length check is worded elsewhere in the PR. src/main/kotlin/org/opensearch/commons/alerting/model/PPLTrigger.kt [64-70] require(this.id.length <= UUID_LENGTH) { - "Trigger ID too long, length must be less than $UUID_LENGTH." + "Trigger ID too long, length must be at most $UUID_LENGTH." } require(this.name.length <= ALERTING_MAX_NAME_LENGTH) { - "Trigger name too long, length must be less than $ALERTING_MAX_NAME_LENGTH." + "Trigger name too long, length must be at most $ALERTING_MAX_NAME_LENGTH." } Suggestion importance[1-10]: 3 __ Why: The error messages say "less than" but the constraint is <= (less than or equal to), which is misleading. This is a minor documentation/message accuracy issue with low functional impact.	Low

Suggestions up to commit bbebb2a

Category	Suggestion	Impact
Possible issue	Consolidate repeated version checks in deserialization The version check sin.version.onOrAfter(Version.V_3_7_0) is called three separate times, which is redundant and could lead to inconsistency. Consolidate all three reads into a single version check block to ensure they are always read together or skipped together, matching the write path. src/main/kotlin/org/opensearch/commons/alerting/model/MonitorRunResult.kt [152-167] -val pplCount = if (sin.version.onOrAfter(Version.V_3_7_0)) { - sin.readVInt() -} else { - 0 -} val pplList = mutableListOf<Map<String, Any?>>() +val pplNumResults: Long? if (sin.version.onOrAfter(Version.V_3_7_0)) { + val pplCount = sin.readVInt() for (i in 0 until pplCount) { pplList.add(suppressWarning(sin.readMap())) // pplResults } -} -val pplNumResults = if (sin.version.onOrAfter(Version.V_3_7_0)) { - sin.readOptionalLong() + pplNumResults = sin.readOptionalLong() } else { - null + pplNumResults = null } Suggestion importance[1-10]: 7 __ Why: The three separate sin.version.onOrAfter(Version.V_3_7_0) checks are redundant and could lead to inconsistency if modified later. Consolidating them into a single block matches the write path structure and improves maintainability.	Medium
	Use safe cast helper when reading map list from stream The readMap() call returns MutableMap<String?, Any?> but the field type is List<Map<String, Any?>>. This unchecked cast may silently produce incorrect types at runtime (e.g., null keys). It should use the same suppressWarning helper used elsewhere in the codebase to safely cast the map, consistent with how InputRunResults.readFrom handles this. src/main/kotlin/org/opensearch/commons/alerting/model/QueryLevelTriggerRunResult.kt [33-37] pplCustomQueryResults = if (sin.version.onOrAfter(Version.V_3_7_0)) { - sin.readList { it.readMap() } + sin.readList { suppressWarning(it.readMap()) } } else { listOf() } Suggestion importance[1-10]: 6 __ Why: The raw it.readMap() returns MutableMap<String?, Any?> which may contain null keys, while the field type is List<Map<String, Any?>>. Using suppressWarning as done in InputRunResults.readFrom ensures consistent and safe handling of this type cast across the codebase.	Low
	Fix token advancement bug after parsing actions array The parser calls xcp.nextToken() at the end of each loop iteration, but for the ACTIONS_FIELD case the inner while loop already consumes tokens up to END_ARRAY, and then the outer xcp.nextToken() advances past the next field name. This means the token after the actions array is skipped, potentially causing fields following actions in the JSON to be silently ignored or causing a parse error. The ACTIONS_FIELD case should either break out of the outer loop's extra nextToken() call or the loop structure should be adjusted to handle this consistently (similar to how other triggers parse actions). src/main/kotlin/org/opensearch/commons/alerting/model/PPLTrigger.kt [255-307] while (xcp.currentToken() != XContentParser.Token.END_OBJECT) { val fieldName = xcp.currentName() xcp.nextToken() when (fieldName) { - ... + ID_FIELD -> id = xcp.text() + NAME_FIELD -> name = xcp.text() + SEVERITY_FIELD -> severity = xcp.text() + CONDITION_TYPE_FIELD -> { + val input = xcp.text() + val enumMatchResult = ConditionType.enumFromString(input) + ?: throw IllegalArgumentException( + "Invalid value for $CONDITION_TYPE_FIELD: $input. " + + "Supported values are ${ConditionType.entries.map { it.value }}" + ) + conditionType = enumMatchResult + } + NUM_RESULTS_CONDITION_FIELD -> { + if (xcp.currentToken() != XContentParser.Token.VALUE_NULL) { + val input = xcp.text() + val enumMatchResult = NumResultsCondition.enumFromString(input) + ?: throw IllegalArgumentException( + "Invalid value for $NUM_RESULTS_CONDITION_FIELD: $input. " + + "Supported values are ${NumResultsCondition.entries.map { it.value }}" + ) + numResultsCondition = enumMatchResult + } + } + NUM_RESULTS_VALUE_FIELD -> { + if (xcp.currentToken() != XContentParser.Token.VALUE_NULL) { + numResultsValue = xcp.longValue() + } + } + CUSTOM_CONDITION_FIELD -> { + if (xcp.currentToken() != XContentParser.Token.VALUE_NULL) { + customCondition = xcp.text() + } + } + ACTIONS_FIELD -> { + XContentParserUtils.ensureExpectedToken( + XContentParser.Token.START_ARRAY, + xcp.currentToken(), + xcp + ) + while (xcp.nextToken() != XContentParser.Token.END_ARRAY) { + actions.add(Action.parse(xcp)) + } + // Do NOT call xcp.nextToken() again here; fall through to the outer nextToken() + } else -> throw IllegalArgumentException("Unexpected field when parsing PPL Trigger: $fieldName") } xcp.nextToken() } Suggestion importance[1-10]: 5 __ Why: The concern about double nextToken() after the ACTIONS_FIELD case is valid in principle, but the improved_code doesn't actually fix the issue — it still calls xcp.nextToken() at the end of the loop for all cases including ACTIONS_FIELD. The suggestion content and improved code are inconsistent with each other.	Low
General	Fix misleading error messages for length constraints The error messages say "less than" but the checks use <= (less than or equal to). This is misleading to users — the messages should say "at most" or "less than or equal to" to accurately reflect the constraint, consistent with the wording used in Monitor.kt for the same limit. src/main/kotlin/org/opensearch/commons/alerting/model/PPLTrigger.kt [64-70] require(this.id.length <= UUID_LENGTH) { - "Trigger ID too long, length must be less than $UUID_LENGTH." + "Trigger ID too long, length must be at most $UUID_LENGTH." } require(this.name.length <= ALERTING_MAX_NAME_LENGTH) { - "Trigger name too long, length must be less than $ALERTING_MAX_NAME_LENGTH." + "Trigger name too long, length must be at most $ALERTING_MAX_NAME_LENGTH." } Suggestion importance[1-10]: 4 __ Why: The error messages say "less than" but the constraints use <= (less than or equal to), which is misleading. The fix aligns the wording with Monitor.kt which uses "at most" for the same constraint.	Low

Suggestions up to commit 6917880

Category	Suggestion	Impact
Possible issue	Validate required field before object construction The query variable is declared with lateinit but is never guaranteed to be initialized if the QUERY_FIELD is absent from the parsed content. If the field is missing, accessing query will throw an UninitializedPropertyAccessException. Add a validation check before constructing the object. src/main/kotlin/org/opensearch/commons/alerting/model/PPLInput.kt [101] +require(::query.isInitialized) { "PPLInput query field must be included." } return PPLInput(query, queryLanguage) Suggestion importance[1-10]: 7 __ Why: The query field is declared with lateinit and will throw UninitializedPropertyAccessException if missing from parsed content. Adding a validation check before constructing the object is a valid defensive measure, though Kotlin's lateinit already provides a clear error message.	Medium
	Prevent silent data corruption for unknown input types The else branch silently maps any unrecognized Input type to REMOTE_DOC_LEVEL_MONITOR_INPUT, which will cause silent data corruption or deserialization errors for unknown input types. It should throw an exception to surface the problem clearly. src/main/kotlin/org/opensearch/commons/alerting/model/Monitor.kt [263-267] } else if (it is PPLInput) { out.writeEnum(Input.Type.PPL_INPUT) +} else if (it is RemoteDocLevelMonitorInput) { + out.writeEnum(Input.Type.REMOTE_DOC_LEVEL_MONITOR_INPUT) } else { - out.writeEnum(Input.Type.REMOTE_DOC_LEVEL_MONITOR_INPUT) + throw IllegalStateException("Unexpected input type [${it::class.java}] when writing Monitor") } Suggestion importance[1-10]: 6 __ Why: The else branch silently maps unknown Input types to REMOTE_DOC_LEVEL_MONITOR_INPUT, which could cause silent data corruption. Making the else branch explicit with a proper type check and exception is a valid improvement for correctness and debuggability.	Low
	Fix double token advancement in parse loop The parse loop calls xcp.nextToken() both at the start of the loop body (to move past the field name) and at the end (to advance to the next token). This double-advance pattern can cause the parser to skip tokens or misparse fields, especially for complex nested types like ACTIONS_FIELD. The standard pattern is to call xcp.nextToken() once per iteration at the top, not at both top and bottom. src/main/kotlin/org/opensearch/commons/alerting/model/PPLTrigger.kt [255-307] -while (xcp.currentToken() != XContentParser.Token.END_OBJECT) { +while (xcp.nextToken() != XContentParser.Token.END_OBJECT) { val fieldName = xcp.currentName() xcp.nextToken() - ... - xcp.nextToken() + + when (fieldName) { + ID_FIELD -> id = xcp.text() + NAME_FIELD -> name = xcp.text() + SEVERITY_FIELD -> severity = xcp.text() + CONDITION_TYPE_FIELD -> { + val input = xcp.text() + val enumMatchResult = ConditionType.enumFromString(input) + ?: throw IllegalArgumentException( + "Invalid value for $CONDITION_TYPE_FIELD: $input. " + + "Supported values are ${ConditionType.entries.map { it.value }}" + ) + conditionType = enumMatchResult + } + NUM_RESULTS_CONDITION_FIELD -> { + if (xcp.currentToken() != XContentParser.Token.VALUE_NULL) { + val input = xcp.text() + val enumMatchResult = NumResultsCondition.enumFromString(input) + ?: throw IllegalArgumentException( + "Invalid value for $NUM_RESULTS_CONDITION_FIELD: $input. " + + "Supported values are ${NumResultsCondition.entries.map { it.value }}" + ) + numResultsCondition = enumMatchResult + } + } + NUM_RESULTS_VALUE_FIELD -> { + if (xcp.currentToken() != XContentParser.Token.VALUE_NULL) { + numResultsValue = xcp.longValue() + } + } + CUSTOM_CONDITION_FIELD -> { + if (xcp.currentToken() != XContentParser.Token.VALUE_NULL) { + customCondition = xcp.text() + } + } + ACTIONS_FIELD -> { + XContentParserUtils.ensureExpectedToken( + XContentParser.Token.START_ARRAY, + xcp.currentToken(), + xcp + ) + while (xcp.nextToken() != XContentParser.Token.END_ARRAY) { + actions.add(Action.parse(xcp)) + } + } + else -> throw IllegalArgumentException("Unexpected field when parsing PPL Trigger: $fieldName") + } } Suggestion importance[1-10]: 3 __ Why: The suggestion claims the double xcp.nextToken() pattern is incorrect, but looking at the actual code, the loop condition uses xcp.currentToken() (not xcp.nextToken()), and the xcp.nextToken() at the end advances to the next field name. This is a valid pattern used in other parsers in the codebase. The suggestion's improved code changes the loop structure significantly and may actually break parsing.	Low
General	Consolidate repeated version checks into one block The version check sin.version.onOrAfter(Version.V_3_7_0) is performed three separate times, which is redundant and error-prone. Consolidate into a single version check block to avoid inconsistency and reduce repeated calls. src/main/kotlin/org/opensearch/commons/alerting/model/MonitorRunResult.kt [148-163] -val pplCount = if (sin.version.onOrAfter(Version.V_3_7_0)) { - sin.readVInt() -} else { - 0 -} val pplList = mutableListOf<Map<String, Any?>>() +var pplNumResults: Long? = null if (sin.version.onOrAfter(Version.V_3_7_0)) { + val pplCount = sin.readVInt() for (i in 0 until pplCount) { pplList.add(suppressWarning(sin.readMap())) // pplResults } -} -val pplNumResults = if (sin.version.onOrAfter(Version.V_3_7_0)) { - sin.readOptionalLong() -} else { - null + pplNumResults = sin.readOptionalLong() } Suggestion importance[1-10]: 5 __ Why: The three separate sin.version.onOrAfter(Version.V_3_7_0) checks are redundant and could be consolidated into a single block for clarity and maintainability. The improved code correctly refactors this without changing behavior.	Low

[eirsep]

shouldn't there be another check that PPL can have exactly 1 trigger?

[toepkerd]

PPL Monitors can have multiple Triggers. They are, however, only allowed to have 1 Input.

[eirsep]

why "base" ?

[toepkerd]

The base PPL query is what the PPL Monitor is configured with. Custom condition triggers append to this base query to run their own queries.

[lezzago]

should we add comments to make this clear?

[toepkerd]

Will add clarifying comment

[eirsep]

why lateinit??

[toepkerd]

No specific reason for lateinit, can change to initialize with null

[eirsep]

what is this field? plz don't pass empty. this won't test serde roundtrip extensively

[toepkerd]

It's the results field for the existing Monitor types. The idea is that DSL Monitors would only ever use the results field, and PPL Monitors will only ever use the pplBaseQueryResults field. For testing purposes, though, there's no harm in actually populating this list.

[toepkerd]

Upcoming change will only populate this results list, but not touch existing randomInputRunResults() helpers that return a null aggTriggersAfterKey and exception. aggTriggersAfterKey is intentionally left out of serde (reference). Including an exception would cause unit tests outside the scope of this PR to fail because Exceptions' .equals() checks reference equality, not value equality. Fixing this would require a more general effort across the unit tests of all existing Monitor types.

[lezzago]

is this really needed? Its for the destination id which would be the notification channel id. We dont create or generate an id from this plugin. Does it make sense for this validation?

[toepkerd]

This change was requested during a security review of PPL Alerting. The reasoning is that Channel IDs are user input (e.g. in CreateMonitor API) that must be sanitized.

[lezzago]

Ack, have we confirmed with notification plugin that this is how he notification channel id is formatted?

[toepkerd]

After offline discussion, will be removing this ID check. Channel creation happens in Notifications plugin, and there are no rules/constraints there on the contents of Notification channel IDs. As such, Alerting can't block Monitor creation based on its own constraints for what a Notifications channel ID should follow.

[github-actions]

Persistent review updated to latest commit bbebb2a

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 092a85d.

Path	Line	Severity	Description
src/main/kotlin/org/opensearch/commons/alerting/model/PPLInput.kt	18	medium	The `query` field accepts arbitrary PPL/SQL strings with no content-level sanitization or allowlisting at the model layer. If the execution layer does not enforce a strict sandbox, malicious queries (e.g., exfiltrating index data or causing resource exhaustion) could be submitted and stored.
src/main/kotlin/org/opensearch/commons/alerting/model/PPLTrigger.kt	48	medium	The `customCondition` field accepts arbitrary PPL where-clause expressions with no structural validation. This string is intended to be appended to a base PPL query at runtime; without sanitization at the execution layer, it is a PPL injection vector that could alter query semantics or exfiltrate data.
src/main/kotlin/org/opensearch/commons/alerting/model/Alert.kt	759	low	The `asTemplateArg()` method now includes the full `queryResults` list (arbitrary key-value maps from query output) in the template context. Sensitive fields returned by a PPL query (PII, credentials, etc.) will be automatically available to every Mustache/Painless notification template, potentially leaking them into notification destinations.
src/main/kotlin/org/opensearch/commons/alerting/model/Input.kt	46	low	The else/fallthrough branch in `Input.parse()` was changed from `RemoteDocLevelMonitorInput.parse(xcp)` to `PPLInput.parseInner(xcp)`. Any unrecognised input type will now silently be parsed as a PPLInput rather than throwing. This could mask future type confusion or allow unexpected input types to be accepted as PPL queries.
src/main/kotlin/org/opensearch/commons/alerting/model/QueryLevelTriggerRunResult.kt	54	low	The new `pplCustomQueryResults` field (arbitrary query row data) is serialised into `toXContent` output unconditionally. Query results containing sensitive data (user records, credentials, etc.) from PPL/custom conditions will be surfaced in every API response that includes trigger run results.
src/main/kotlin/org/opensearch/commons/alerting/model/MonitorRunResult.kt	97	low	The `pplBaseQueryResults` list (raw query row data) is included in `toXContent` under `ppl_query_results`. If the base PPL query touches sensitive indices, this unconditionally exposes full row data in the monitor run result API response, independent of any field-level access controls.

The table above displays the top 10 most important findings.

Total: 6 | Critical: 0 | High: 0 | Medium: 2 | Low: 4

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

Persistent review updated to latest commit d44d5ce

[github-actions]

Persistent review updated to latest commit 04c0466

[github-actions]

Persistent review updated to latest commit 092a85d