Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add alerting tools IT; fix missing system index bug of SearchMonitorsTool #135

Merged
merged 9 commits into from
Jan 22, 2024

Conversation

ohltyler
Copy link
Member

@ohltyler ohltyler commented Jan 17, 2024

Description

This PR adds improvements to the existing SearchAlertsTool and SearchMonitors tools:

  • adds basic integ tests (more to be added tracked in followup issue Improve alerting tool IT #136)
  • handles edge cases of missing system indices for SearchMonitorsTool
  • cleans up processing logic of responses into helper fns for SearchMonitorsTool

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • 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 <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ohltyler
Copy link
Member Author

ohltyler commented Jan 17, 2024

Note these integ tests will fail until opensearch-project/alerting#1382 is merged and published to sonatype, so the CI can pick up the updated alerting dependency. The CI can be reran when that happens. In the meantime confirmed everything passes locally using a locally-published version with the alerting bug fix.

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ohltyler
Copy link
Member Author

ohltyler commented Jan 18, 2024

TODOs: add/improve integ tests. Tracked in separate issue and mentioned in source code comments: #136

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ohltyler ohltyler changed the title Add alerting tool IT; various alerting tool bug fixes Add alerting tool IT; fix missing system index bug of SearchMonitorsTool Jan 18, 2024
@ohltyler ohltyler changed the title Add alerting tool IT; fix missing system index bug of SearchMonitorsTool Add alerting tools IT; fix missing system index bug of SearchMonitorsTool Jan 18, 2024
Copy link

codecov bot commented Jan 19, 2024

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (e278d8d) 80.83% compared to head (f5d056c) 80.65%.

Files Patch % Lines
...org/opensearch/agent/tools/SearchMonitorsTool.java 78.94% 8 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #135      +/-   ##
============================================
- Coverage     80.83%   80.65%   -0.19%     
- Complexity      195      198       +3     
============================================
  Files            13       13              
  Lines          1002     1013      +11     
  Branches        133      133              
============================================
+ Hits            810      817       +7     
- Misses          141      145       +4     
  Partials         51       51              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

// TODO: Add IT to test against sample monitor data

@SneakyThrows
private void createMonitorsSystemIndex(String monitorId, String monitorName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are trying to create the system index for alerting. The best way to do this is to create a dummy monitor, so that the schema mapping of the index is setup correctly and would be better to test for an integration test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - this was an interim test fn that is actually currently not used. I'll remove and leave the TODO, as this will be grouped with the added tests in the followup issue #136. Thanks for the suggestion, I will likely create dummy monitors and alerts for testing the populated results of the search monitors and search alerts tools respectively.

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ohltyler ohltyler requested a review from lezzago January 22, 2024 18:26
@ohltyler ohltyler merged commit 3e4d451 into opensearch-project:main Jan 22, 2024
11 of 13 checks passed
@ohltyler ohltyler deleted the alerting-tool-it branch January 22, 2024 22:44
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 22, 2024
…Tool (#135)

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
(cherry picked from commit 3e4d451)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
ohltyler pushed a commit that referenced this pull request Jan 23, 2024
…Tool (#135) (#141)

(cherry picked from commit 3e4d451)

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
yuye-aws pushed a commit to yuye-aws/skills that referenced this pull request Apr 26, 2024
…Tool (opensearch-project#135) (opensearch-project#141)

(cherry picked from commit 3e4d451)

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants