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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0360a6a
Add back opendistro prefixed sweeper enabled to alerting stats response
qreshi Jan 21, 2022
0d15bf1
Rename stats response field variables used in tests
qreshi Jan 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions alerting/src/test/kotlin/org/opensearch/alerting/AlertingRestTestCase.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ abstract class AlertingRestTestCase : ODFERestTestCase() {
protected val numberOfNodes = System.getProperty("cluster.number_of_nodes", "1")!!.toInt()
protected val isMultiNode = numberOfNodes > 1

protected val statsResponseOpendistroSweeperEnabledField = "opendistro.scheduled_jobs.enabled"
protected val statsResponseOpenSearchSweeperEnabledField = "plugins.scheduled_jobs.enabled"

override fun xContentRegistry(): NamedXContentRegistry {
return NamedXContentRegistry(
mutableListOf(
Expand Down
31 changes: 20 additions & 11 deletions alerting/src/test/kotlin/org/opensearch/alerting/resthandler/MonitorRestApiIT.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

assertEquals("Scheduled job index exists but there are no scheduled jobs.", false, responseMap["scheduled_job_index_exists"])
val _nodes = responseMap["_nodes"] as Map<String, Int>
validateAlertingStatsNodeResponse(_nodes)
Expand All @@ -899,7 +898,7 @@ class MonitorRestApiIT : AlertingRestTestCase() {
val monitorId = createMonitor(randomQueryLevelMonitor(enabled = true), refresh = true).id

var alertingStats = getAlertingStats()
assertEquals("Scheduled job is not enabled", true, alertingStats[ScheduledJobSettings.SWEEPER_ENABLED.key])
assertAlertingStatsSweeperEnabled(alertingStats, true)
assertEquals("Scheduled job index does not exist", true, alertingStats["scheduled_job_index_exists"])
assertEquals("Scheduled job index is not yellow", "yellow", alertingStats["scheduled_job_index_status"])
assertEquals("Nodes are not on schedule", numberOfNodes, alertingStats["nodes_on_schedule"])
Expand All @@ -916,7 +915,7 @@ class MonitorRestApiIT : AlertingRestTestCase() {
disableScheduledJob()

alertingStats = getAlertingStats()
assertEquals("Scheduled job is still enabled", false, alertingStats[ScheduledJobSettings.SWEEPER_ENABLED.key])
assertAlertingStatsSweeperEnabled(alertingStats, false)
assertFalse(
"Monitor [$monitorId] was still scheduled based on the alerting stats response: $alertingStats",
isMonitorScheduled(monitorId, alertingStats)
Expand All @@ -929,7 +928,7 @@ class MonitorRestApiIT : AlertingRestTestCase() {
Thread.sleep(2000)

alertingStats = getAlertingStats()
assertEquals("Scheduled job is not enabled", true, alertingStats[ScheduledJobSettings.SWEEPER_ENABLED.key])
assertAlertingStatsSweeperEnabled(alertingStats, true)
assertTrue(
"Monitor [$monitorId] was not re-scheduled based on the alerting stats response: $alertingStats",
isMonitorScheduled(monitorId, alertingStats)
Expand All @@ -941,8 +940,7 @@ class MonitorRestApiIT : AlertingRestTestCase() {
enableScheduledJob()

val responseMap = getAlertingStats()
// assertEquals("Cluster name is incorrect", responseMap["cluster_name"], "alerting_integTestCluster")
assertEquals("Scheduled job is not enabled", true, responseMap[ScheduledJobSettings.SWEEPER_ENABLED.key])
assertAlertingStatsSweeperEnabled(responseMap, true)
assertEquals("Scheduled job index exists but there are no scheduled jobs.", false, responseMap["scheduled_job_index_exists"])
val _nodes = responseMap["_nodes"] as Map<String, Int>
validateAlertingStatsNodeResponse(_nodes)
Expand All @@ -954,8 +952,7 @@ class MonitorRestApiIT : AlertingRestTestCase() {
createRandomMonitor(refresh = true)

val responseMap = getAlertingStats()
// assertEquals("Cluster name is incorrect", responseMap["cluster_name"], "alerting_integTestCluster")
assertEquals("Scheduled job is not enabled", true, responseMap[ScheduledJobSettings.SWEEPER_ENABLED.key])
assertAlertingStatsSweeperEnabled(responseMap, true)
assertEquals("Scheduled job index does not exist", true, responseMap["scheduled_job_index_exists"])
assertEquals("Scheduled job index is not yellow", "yellow", responseMap["scheduled_job_index_status"])
assertEquals("Nodes are not on schedule", numberOfNodes, responseMap["nodes_on_schedule"])
Expand Down Expand Up @@ -983,8 +980,7 @@ class MonitorRestApiIT : AlertingRestTestCase() {
createRandomMonitor(refresh = true)

val responseMap = getAlertingStats("/jobs_info")
// assertEquals("Cluster name is incorrect", responseMap["cluster_name"], "alerting_integTestCluster")
assertEquals("Scheduled job is not enabled", true, responseMap[ScheduledJobSettings.SWEEPER_ENABLED.key])
assertAlertingStatsSweeperEnabled(responseMap, true)
assertEquals("Scheduled job index does not exist", true, responseMap["scheduled_job_index_exists"])
assertEquals("Scheduled job index is not yellow", "yellow", responseMap["scheduled_job_index_status"])
assertEquals("Nodes not on schedule", numberOfNodes, responseMap["nodes_on_schedule"])
Expand Down Expand Up @@ -1102,4 +1098,17 @@ class MonitorRestApiIT : AlertingRestTestCase() {

return false
}

private fun assertAlertingStatsSweeperEnabled(alertingStatsResponse: Map<String, Any>, expected: Boolean) {
assertEquals(
"Legacy scheduled job enabled field is not set to $expected",
expected,
alertingStatsResponse[statsResponseOpendistroSweeperEnabledField]
)
assertEquals(
"Scheduled job is not ${if (expected) "enabled" else "disabled"}",
expected,
alertingStatsResponse[statsResponseOpenSearchSweeperEnabledField]
)
}
}
2 changes: 2 additions & 0 deletions core/src/main/kotlin/org/opensearch/alerting/core/action/node/ScheduledJobsStatsResponse.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package org.opensearch.alerting.core.action.node

import org.opensearch.action.FailedNodeException
import org.opensearch.action.support.nodes.BaseNodesResponse
import org.opensearch.alerting.core.settings.LegacyOpenDistroScheduledJobSettings
import org.opensearch.alerting.core.settings.ScheduledJobSettings
import org.opensearch.cluster.ClusterName
import org.opensearch.cluster.health.ClusterIndexHealth
Expand Down Expand Up @@ -56,6 +57,7 @@ class ScheduledJobsStatsResponse : BaseNodesResponse<ScheduledJobStats>, ToXCont
}

override fun toXContent(builder: XContentBuilder, params: ToXContent.Params): XContentBuilder {
builder.field(LegacyOpenDistroScheduledJobSettings.SWEEPER_ENABLED.key, scheduledJobEnabled)
builder.field(ScheduledJobSettings.SWEEPER_ENABLED.key, scheduledJobEnabled)
builder.field("scheduled_job_index_exists", indexExists)
builder.field("scheduled_job_index_status", indexHealth?.status?.name?.toLowerCase())
Expand Down