Reference get monitor and search monitor action / request / responses from common-utils #1305

ohltyler · 2023-11-21T18:00:05Z

Issue #, if available:

Description of changes:

Refactors the get monitor and search monitor action / request / responses from its definitions in this repository, to use the referenced definitions in common-utils. See related common-utils PR: opensearch-project/common-utils#566.

Historically, this was done for several of the alerting transport actions already - see #606 for an example

updates all import statements of GetMonitorAction / GetMonitorRequest / GetMonitorResponse / SearchMonitorAction / SearchMonitorRequest
updates all instances of GetMonitorAction.INSTANCE -> AlertingActions.GET_MONITOR_ACTION_TYPE, and all instances of SearchMonitorAction.INSTANCE -> AlertingActions.SEARCH_MONITORS_ACTION_TYPE
removes the moved files and relevant test files
updates AlertingPlugin to reference the correct get monitor & search monitor actions

Confirmed build and all tests pass. Tested on a local cluster using the changes in common-utils, confirmed all functionality and APIs work as expected.

CheckList:

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.

Signed-off-by: Tyler Ohlsen <[email protected]>

ohltyler · 2023-11-21T18:10:01Z

Changing to draft to ensure this is not merged until opensearch-project/common-utils#566 is merged first. Will then re-run CI checks.

eirsep · 2023-11-21T18:16:15Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportGetFindingsAction.kt

@@ -167,7 +166,7 @@ class TransportGetFindingsSearchAction @Inject constructor(
                )
                val getMonitorResponse: GetMonitorResponse =
                    [email protected] {
-                        execute(GetMonitorAction.INSTANCE, getMonitorRequest, it)
+                        execute(AlertingActions.GET_MONITOR_ACTION_TYPE, getMonitorRequest, it)


rewrite request to avoid serialization issue?

This is following strategies of your previous refactoring PRs - see https://github.com/opensearch-project/alerting/pull/606/files

Could you explain more what the potential issue could be?

https://github.com/opensearch-project/alerting/blob/main/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexMonitorAction.kt#L121-L123

we had faced ser/deserialization issues due to different plugins not sharing the same classloader amongst themselves

so, although these tests may pass now, the plugin invoking get monitor request would fail if the request is not transformed and re-deserialized in the transport layer

Got it, thanks for the clarification. Updated the transport actions in latest commit, and did another round of manual verification on the changed APIs

eirsep · 2023-11-21T18:17:14Z

plz add verification in rest tests to ensure get monitor api still works as expected.

ohltyler · 2023-11-21T19:13:33Z

plz add verification in rest tests to ensure get monitor api still works as expected.

This is already covered via the different REST handler test suites - MonitorRestApiIT & SecureMonitorRestApiIT. I've confirmed this all tests locally as well, the failures in the CI are unrelated. Also, I've manually tested out by spinning up a local cluster and exercising all of the APIs.

Signed-off-by: Tyler Ohlsen <[email protected]>

bowenlan-amzn · 2023-11-22T18:42:14Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportGetMonitorAction.kt

@@ -131,7 +131,6 @@ class TransportGetMonitorAction @Inject constructor(
                                        response.version,
                                        response.seqNo,
                                        response.primaryTerm,
-                                        RestStatus.OK,


What happened with this line

See last bullet point in opensearch-project/common-utils#566:

removed the status field from GetMonitorResponse. This is so we can change the response from extending ActionResponse and ToXContentObject explicitly, and changing to BaseResponse, which has an override for getStatus(). Note the BaseResponse extends the same ActionResponse and ToXContentObject. This follows the same approach as done previously - see https://github.com/opensearch-project/alerting/pull/556/files as an example.

Signed-off-by: Tyler Ohlsen <[email protected]>

opensearch-trigger-bot · 2023-11-28T17:06:31Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

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.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/alerting/backport-2.x
# Create a new branch
git switch --create backport-1305-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b7c1823ea8e6ddba894f92a6f30ad2750ca6edd9
# Push it to GitHub
git push --set-upstream origin backport-1305-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/alerting/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport-1305-to-2.x.

… from common-utils (opensearch-project#1305) * Use get monitor action / req / resp from common-utils Signed-off-by: Tyler Ohlsen <[email protected]> * Clean up remaining references Signed-off-by: Tyler Ohlsen <[email protected]> * Refactor search monitor action/request also Signed-off-by: Tyler Ohlsen <[email protected]> * Rewrite get monitor and search monitor transport action requests Signed-off-by: Tyler Ohlsen <[email protected]> --------- Signed-off-by: Tyler Ohlsen <[email protected]>

… from common-utils (#1305) * Use get monitor action / req / resp from common-utils Signed-off-by: Tyler Ohlsen <[email protected]> * Clean up remaining references Signed-off-by: Tyler Ohlsen <[email protected]> * Refactor search monitor action/request also Signed-off-by: Tyler Ohlsen <[email protected]> * Rewrite get monitor and search monitor transport action requests Signed-off-by: Tyler Ohlsen <[email protected]> --------- Signed-off-by: Tyler Ohlsen <[email protected]> Signed-off-by: Chase Engelbrecht <[email protected]>

ohltyler added 2 commits November 21, 2023 16:48

Use get monitor action / req / resp from common-utils

2e46dde

Signed-off-by: Tyler Ohlsen <[email protected]>

Clean up remaining references

762efda

Signed-off-by: Tyler Ohlsen <[email protected]>

ohltyler requested review from lezzago, AWSHurneyt, sbcd90, eirsep, getsaurabh02, praveensameneni, qreshi, bowenlan-amzn and rishabhmaurya as code owners November 21, 2023 18:00

ohltyler mentioned this pull request Nov 21, 2023

Move get monitor and search monitor action / request / responses to common-utils opensearch-project/common-utils#566

Merged

5 tasks

ohltyler added the backport 2.x label Nov 21, 2023

ohltyler marked this pull request as draft November 21, 2023 18:09

eirsep reviewed Nov 21, 2023

View reviewed changes

ohltyler changed the title ~~Reference get monitor action / request / response from common-utils~~ Reference get monitor and search monitor action / request / responses from common-utils Nov 21, 2023

Refactor search monitor action/request also

9d05072

Signed-off-by: Tyler Ohlsen <[email protected]>

ohltyler marked this pull request as ready for review November 22, 2023 17:47

bowenlan-amzn approved these changes Nov 22, 2023

View reviewed changes

Rewrite get monitor and search monitor transport action requests

15464b2

Signed-off-by: Tyler Ohlsen <[email protected]>

ohltyler mentioned this pull request Nov 27, 2023

[WIP] Implement search monitors tool opensearch-project/ml-commons#1699

Closed

5 tasks

eirsep approved these changes Nov 28, 2023

View reviewed changes

eirsep merged commit b7c1823 into opensearch-project:main Nov 28, 2023
13 of 14 checks passed

opensearch-trigger-bot bot added the failed backport label Nov 28, 2023

ohltyler mentioned this pull request Nov 28, 2023

[Backport 2.x] Reference get monitor and search monitor action / request / responses from common-utils #1315

Merged

1 task

ohltyler mentioned this pull request Dec 11, 2023

Implement search monitors tool opensearch-project/ml-commons#1749

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reference get monitor and search monitor action / request / responses from common-utils #1305

Reference get monitor and search monitor action / request / responses from common-utils #1305

ohltyler commented Nov 21, 2023 •

edited

Loading

ohltyler commented Nov 21, 2023 •

edited

Loading

eirsep Nov 21, 2023

ohltyler Nov 21, 2023

eirsep Nov 27, 2023

ohltyler Nov 27, 2023

eirsep commented Nov 21, 2023

ohltyler commented Nov 21, 2023

bowenlan-amzn Nov 22, 2023

ohltyler Nov 22, 2023

opensearch-trigger-bot bot commented Nov 28, 2023

Reference get monitor and search monitor action / request / responses from common-utils #1305

Reference get monitor and search monitor action / request / responses from common-utils #1305

Conversation

ohltyler commented Nov 21, 2023 • edited Loading

ohltyler commented Nov 21, 2023 • edited Loading

eirsep Nov 21, 2023

Choose a reason for hiding this comment

ohltyler Nov 21, 2023

Choose a reason for hiding this comment

eirsep Nov 27, 2023

Choose a reason for hiding this comment

ohltyler Nov 27, 2023

Choose a reason for hiding this comment

eirsep commented Nov 21, 2023

ohltyler commented Nov 21, 2023

bowenlan-amzn Nov 22, 2023

Choose a reason for hiding this comment

ohltyler Nov 22, 2023

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Nov 28, 2023

ohltyler commented Nov 21, 2023 •

edited

Loading

ohltyler commented Nov 21, 2023 •

edited

Loading