PPL Alerting: Create and Update Monitor V2 #1961
Conversation
Signed-off-by: Dennis Toepker <[email protected]>
toepkerd
requested review from
AWSHurneyt,
amsiglan,
bowenlan-amzn,
eirsep,
engechas,
getsaurabh02,
goyamegh,
jowg-amazon,
lezzago,
praveensameneni,
rishabhmaurya,
riysaxen-amzn and
sbcd90
as code owners
alerting/src/main/kotlin/org/opensearch/alerting/AlertingV2Utils.kt
Outdated
Show resolved
Hide resolved
alerting/src/main/kotlin/org/opensearch/alerting/AlertingV2Utils.kt
Outdated
Show resolved
Hide resolved
alerting/src/main/kotlin/org/opensearch/alerting/actionv2/IndexMonitorV2Response.kt
Show resolved
Hide resolved
alerting/src/main/kotlin/org/opensearch/alerting/transportv2/TransportIndexMonitorV2Action.kt
Outdated
Show resolved
Hide resolved
| import org.opensearch.sql.plugin.transport.TransportPPLQueryRequest | ||
| import org.opensearch.transport.client.node.NodeClient | ||
|
|
||
| object AlertingV2Utils { |
There was a problem hiding this comment.
Can we move the utils in this function better suited for ppl into a PPL utils class?
| // the PPL keyword "eval", followed by a whitespace must be present, otherwise a syntax error from PPL plugin would've | ||
| // been thrown when executing the query (without the whitespace, the query would've had something like "evalresult", | ||
| // which is invalid PPL | ||
| val regex = """\beval\s+([a-zA-Z_][a-zA-Z0-9_]*)\s*=""".toRegex() |
There was a problem hiding this comment.
can this be a static variable and given our dependency on sql plugin, can we import invalid ppl regex from there?
There was a problem hiding this comment.
Will keep this logic for now but add TODOs to replace these in-house parsers with PPL Plugin functions that likely exist.
| return indices | ||
| } | ||
|
|
||
| fun capPplQueryResultsSize(pplQueryResults: JSONObject, maxSize: Long): JSONObject { |
There was a problem hiding this comment.
nitpick: use PPL instead of Ppl
| /* | ||
| the query results JSON object schema: | ||
| schema: an array of objects storing the data types of each value of the query result rows, in order | ||
| datarows: an array of arrays storing the query results themselves | ||
| total: total number of results / data rows | ||
| size: same as total, redundant field | ||
| */ |
There was a problem hiding this comment.
Can we use java docs instead at the top of these functions?
|
Can we add security integ tests also using RBAC? |
| // Needed for integ tests | ||
| zipArchive group: 'org.opensearch.plugin', name:'opensearch-notifications-core', version: "${opensearch_build}" | ||
| zipArchive group: 'org.opensearch.plugin', name:'notifications', version: "${opensearch_build}" | ||
| zipArchive group: 'org.opensearch.plugin', name:'opensearch-job-scheduler', version: "${opensearch_build}" |
There was a problem hiding this comment.
What has been moved to use the job scheduler plugin? I thought Alerting has its own job scheduler
There was a problem hiding this comment.
This is a needed dependency of the SQL/PPL plugin actually. It is explicitly included here solely for integ test purposes. Without this dependency, integ tests fail.
RBAC ITs have already been written (source), they're just not included in this current PR because further (not yet published) API support is needed to support those tests. |
Signed-off-by: Dennis Toepker <[email protected]>
| * Some util functions that were initially created for Alerting V1, and are leveraged by | ||
| * both Alerting V1 and V2 | ||
| */ | ||
| object AlertingV1Utils { |
There was a problem hiding this comment.
Do we want to move this to AlertingUtils.kt file under the util folder?
There was a problem hiding this comment.
Not a blocking comment but we should have these v1 related pieces in the AlertingUtils or have some consolidation there.
|
Approving assuming checks pass, and other comments are resolved. |
|
|
||
| /** | ||
| * Some util functions that were initially created for Alerting V1, and are leveraged by | ||
| * both Alerting V1 and V2 |
There was a problem hiding this comment.
if its for v1 and v2, then there is no need to call this v1 utils
There was a problem hiding this comment.
I can move this to AlertingV2Utils and delete AlertingV1Utils, does that work?
Signed-off-by: Dennis Toepker <[email protected]>
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/alerting/backport-2.19 2.19
# Navigate to the new working tree
pushd ../.worktrees/alerting/backport-2.19
# Create a new branch
git switch --create backport-1961-to-2.19
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 c911cfa625c8c3de2b47aefc0962cda25b426051
# Push it to GitHub
git push --set-upstream origin backport-1961-to-2.19
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/alerting/backport-2.19Then, create a pull request where the |
* PPL Alerting: Create and Update Monitor V2 Signed-off-by: Dennis Toepker <[email protected]> * addressing PR comments Signed-off-by: Dennis Toepker <[email protected]> * removing AlertingV1Utils Signed-off-by: Dennis Toepker <[email protected]> --------- Signed-off-by: Dennis Toepker <[email protected]> Co-authored-by: Dennis Toepker <[email protected]> (cherry picked from commit c911cfa) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/alerting/backport-3.3 3.3
# Navigate to the new working tree
pushd ../.worktrees/alerting/backport-3.3
# Create a new branch
git switch --create backport-1961-to-3.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 c911cfa625c8c3de2b47aefc0962cda25b426051
# Push it to GitHub
git push --set-upstream origin backport-1961-to-3.3
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/alerting/backport-3.3Then, create a pull request where the |
* PPL Alerting: Create and Update Monitor V2 Signed-off-by: Dennis Toepker <[email protected]> * addressing PR comments Signed-off-by: Dennis Toepker <[email protected]> * removing AlertingV1Utils Signed-off-by: Dennis Toepker <[email protected]> --------- Signed-off-by: Dennis Toepker <[email protected]> Co-authored-by: Dennis Toepker <[email protected]> (cherry picked from commit c911cfa)
* PPL Alerting: Create and Update Monitor V2 * addressing PR comments * removing AlertingV1Utils --------- (cherry picked from commit c911cfa) Signed-off-by: Dennis Toepker <[email protected]> Co-authored-by: Dennis Toepker <[email protected]>
4 participants
Description
PPL Monitor Create and Update MonitorV2 functionality.
Related Issues
#1880
Note on testing
ITs for these functionality have been written, but are not included in this PR because they require support of some other not-yet-published PPL Alerting APIs. The tests will come in later PRs, but for now, here are the tests from the full PPL Alerting branch:
Regular API and xcp ITs: https://github.com/toepkerd/alerting/blob/ppl-main/alerting/src/test/kotlin/org/opensearch/alerting/resthandler/MonitorV2RestApiIT.kt
RBAC ITs: https://github.com/toepkerd/alerting/blob/ppl-main/alerting/src/test/kotlin/org/opensearch/alerting/resthandler/SecureMonitorV2RestApiIT.kt
Check List
--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.