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 back opendistro prefixed sweeper enabled field to alerting stats response #283

Merged
merged 2 commits into from
Jan 24, 2022

Conversation

qreshi
Copy link
Contributor

@qreshi qreshi commented Jan 21, 2022

Signed-off-by: Mohammad Qureshi [email protected]

Issue #, if available:

Description of changes:
The opendistro prefixed field response for sweeper/scheduled jobs enabled in the Alerting Stats API was inadvertently changed to plugins during the migration to OpenSearch. This change reintroduces the legacy field back alongside the existing plugins one to maintain backwards compatibility and to not break behavior for anyone using the current field.

This PR also updates the existing tests asserting on the response of the stats API to use hardcoded values for the scheduled jobs enabled field instead of referring to the Setting.key so it can catch a change like this in the future. We should also see if any other tests are currently checking API responses this way. Fixing those, however, is out of scope for this particular PR.

CheckList:
[x] 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.

@qreshi qreshi requested a review from a team January 21, 2022 20:00
Copy link
Member

@getsaurabh02 getsaurabh02 left a comment

Choose a reason for hiding this comment

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

LGTM however you may want to consider the suggestion around Rest API ITs.

@@ -886,8 +886,7 @@ class MonitorRestApiIT : AlertingRestTestCase() {
disableScheduledJob()

val responseMap = getAlertingStats()
// assertEquals("Cluster name is incorrect", responseMap["cluster_name"], "alerting_integTestCluster")
assertEquals("Scheduled job is not enabled", false, responseMap[ScheduledJobSettings.SWEEPER_ENABLED.key])
assertAlertingStatsSweeperEnabled(responseMap, false)
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a Resource File with the Rest API response, and assert on those values instead. You may want to check how it is achieved similarly in Rest ITs of the core. I feel it's more manageable that way and you will not have to define the explicit fields in the test here.

Copy link
Contributor Author

@qreshi qreshi Jan 22, 2022

Choose a reason for hiding this comment

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

Personally, I think way we're currently asserting here is cleaner since for some tests we just want to check specific values and not the entire response. If we had a resource file then we'd need to handcraft the entire responses for new cases that aren't covered by the old ones. That would be more appropriate for unit tests where we're mocking but would add overhead here.

Copy link
Member

Choose a reason for hiding this comment

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

You can also have resource file versioned since any new API changes would only happen with a change in the minor version. Also, the validation would be between the objects in resource file and the API response here, not necessarily requiring any mock. In order to check for any specific values, we can always filter the object in consideration, given it will be a json structure.

Just that having a resource file would defines one source of truth to compare the API responses against. However, it is just an open idea, and might not necessarily be included in the scope of this PR. Feel free to skip it for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point. Let's skip it for this PR but can you create an issue capturing this and we can continue discussions there and have others weigh in on the option.

@codecov-commenter
Copy link

Codecov Report

Merging #283 (0d15bf1) into main (f92dec0) will increase coverage by 0.00%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #283   +/-   ##
=========================================
  Coverage     78.63%   78.64%           
+ Complexity      218      217    -1     
=========================================
  Files           173      173           
  Lines          6968     6971    +3     
  Branches        915      916    +1     
=========================================
+ Hits           5479     5482    +3     
+ Misses         1002     1001    -1     
- Partials        487      488    +1     
Impacted Files Coverage Δ
...search/alerting/transport/SecureTransportAction.kt 39.02% <0.00%> (+8.25%) ⬆️
...ing/core/action/node/ScheduledJobsStatsResponse.kt 65.38% <100.00%> (+1.38%) ⬆️
...ing/destination/client/DestinationEmailClient.java 72.50% <0.00%> (-5.00%) ⬇️
...ing/model/destination/DestinationContextFactory.kt 66.66% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 813f836...0d15bf1. Read the comment docs.

@qreshi qreshi merged commit cb28e8f into opensearch-project:main Jan 24, 2022
qreshi added a commit to qreshi/opensearch-alerting that referenced this pull request Jan 24, 2022
…response (opensearch-project#283)

* Add back opendistro prefixed sweeper enabled to alerting stats response

Signed-off-by: Mohammad Qureshi <[email protected]>

* Rename stats response field variables used in tests

Signed-off-by: Mohammad Qureshi <[email protected]>
qreshi added a commit to qreshi/opensearch-alerting that referenced this pull request Jan 24, 2022
…response (opensearch-project#283)

* Add back opendistro prefixed sweeper enabled to alerting stats response

Signed-off-by: Mohammad Qureshi <[email protected]>

* Rename stats response field variables used in tests

Signed-off-by: Mohammad Qureshi <[email protected]>
qreshi added a commit to qreshi/opensearch-alerting that referenced this pull request Jan 24, 2022
…response (opensearch-project#283)

* Add back opendistro prefixed sweeper enabled to alerting stats response

Signed-off-by: Mohammad Qureshi <[email protected]>

* Rename stats response field variables used in tests

Signed-off-by: Mohammad Qureshi <[email protected]>
@qreshi qreshi mentioned this pull request Jan 24, 2022
qreshi added a commit to qreshi/opensearch-alerting that referenced this pull request Jan 24, 2022
…response (opensearch-project#283)

* Add back opendistro prefixed sweeper enabled to alerting stats response

Signed-off-by: Mohammad Qureshi <[email protected]>

* Rename stats response field variables used in tests

Signed-off-by: Mohammad Qureshi <[email protected]>
qreshi added a commit to qreshi/opensearch-alerting that referenced this pull request Jan 25, 2022
…response (opensearch-project#283)

* Add back opendistro prefixed sweeper enabled to alerting stats response

Signed-off-by: Mohammad Qureshi <[email protected]>

* Rename stats response field variables used in tests

Signed-off-by: Mohammad Qureshi <[email protected]>
qreshi added a commit to qreshi/opensearch-alerting that referenced this pull request Jan 25, 2022
…response (opensearch-project#283)

* Add back opendistro prefixed sweeper enabled to alerting stats response

Signed-off-by: Mohammad Qureshi <[email protected]>

* Rename stats response field variables used in tests

Signed-off-by: Mohammad Qureshi <[email protected]>
qreshi added a commit to qreshi/opensearch-alerting that referenced this pull request Jan 25, 2022
…response (opensearch-project#283)

* Add back opendistro prefixed sweeper enabled to alerting stats response

Signed-off-by: Mohammad Qureshi <[email protected]>

* Rename stats response field variables used in tests

Signed-off-by: Mohammad Qureshi <[email protected]>
qreshi added a commit to qreshi/opensearch-alerting that referenced this pull request Jan 25, 2022
…response (opensearch-project#283)

* Add back opendistro prefixed sweeper enabled to alerting stats response

Signed-off-by: Mohammad Qureshi <[email protected]>

* Rename stats response field variables used in tests

Signed-off-by: Mohammad Qureshi <[email protected]>
qreshi added a commit that referenced this pull request Jan 26, 2022
* Remove runBlocking calls from MonitorRunner (#281)

* Remove runBlocking calls from MonitorRunner

Signed-off-by: Mohammad Qureshi <[email protected]>

* Add security tests with partial user permissions to check user context being correctly applied

Signed-off-by: Mohammad Qureshi <[email protected]>

* Fix ktlint style issues

Signed-off-by: Mohammad Qureshi <[email protected]>

* Add back opendistro prefixed sweeper enabled field to alerting stats response (#283)

* Add back opendistro prefixed sweeper enabled to alerting stats response

Signed-off-by: Mohammad Qureshi <[email protected]>

* Rename stats response field variables used in tests

Signed-off-by: Mohammad Qureshi <[email protected]>
qreshi added a commit that referenced this pull request Jan 26, 2022
* Remove runBlocking calls from MonitorRunner (#281)

* Remove runBlocking calls from MonitorRunner

Signed-off-by: Mohammad Qureshi <[email protected]>

* Add security tests with partial user permissions to check user context being correctly applied

Signed-off-by: Mohammad Qureshi <[email protected]>

* Fix ktlint style issues

Signed-off-by: Mohammad Qureshi <[email protected]>

* Add back opendistro prefixed sweeper enabled field to alerting stats response (#283)

* Add back opendistro prefixed sweeper enabled to alerting stats response

Signed-off-by: Mohammad Qureshi <[email protected]>

* Rename stats response field variables used in tests

Signed-off-by: Mohammad Qureshi <[email protected]>
qreshi added a commit that referenced this pull request Jan 26, 2022
* Remove runBlocking calls from MonitorRunner (#281)

* Remove runBlocking calls from MonitorRunner

Signed-off-by: Mohammad Qureshi <[email protected]>

* Add security tests with partial user permissions to check user context being correctly applied

Signed-off-by: Mohammad Qureshi <[email protected]>

* Fix ktlint style issues

Signed-off-by: Mohammad Qureshi <[email protected]>

* Add back opendistro prefixed sweeper enabled field to alerting stats response (#283)

* Add back opendistro prefixed sweeper enabled to alerting stats response

Signed-off-by: Mohammad Qureshi <[email protected]>

* Rename stats response field variables used in tests

Signed-off-by: Mohammad Qureshi <[email protected]>
qreshi added a commit that referenced this pull request Jan 26, 2022
…response (#283) (#286)

* Add back opendistro prefixed sweeper enabled to alerting stats response

Signed-off-by: Mohammad Qureshi <[email protected]>

* Rename stats response field variables used in tests

Signed-off-by: Mohammad Qureshi <[email protected]>
AWSHurneyt pushed a commit to AWSHurneyt/OpenSearch-Alerting that referenced this pull request Mar 30, 2022
…#287)

* Remove runBlocking calls from MonitorRunner (opensearch-project#281)

* Remove runBlocking calls from MonitorRunner

Signed-off-by: Mohammad Qureshi <[email protected]>

* Add security tests with partial user permissions to check user context being correctly applied

Signed-off-by: Mohammad Qureshi <[email protected]>

* Fix ktlint style issues

Signed-off-by: Mohammad Qureshi <[email protected]>

* Add back opendistro prefixed sweeper enabled field to alerting stats response (opensearch-project#283)

* Add back opendistro prefixed sweeper enabled to alerting stats response

Signed-off-by: Mohammad Qureshi <[email protected]>

* Rename stats response field variables used in tests

Signed-off-by: Mohammad Qureshi <[email protected]>
Signed-off-by: AWSHurneyt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants