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

NoOpTrigger #420

Merged
merged 4 commits into from
May 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 18 additions & 0 deletions src/main/kotlin/org/opensearch/commons/alerting/model/Alert.kt
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,24 @@ data class Alert(
aggregationResultBucket = null, findingIds = findingIds, relatedDocIds = relatedDocIds
)

constructor(
id: String = NO_ID,
monitor: Monitor,
trigger: NoOpTrigger,
startTime: Instant,
lastNotificationTime: Instant?,
state: State = State.ERROR,
errorMessage: String,
errorHistory: List<AlertError> = mutableListOf(),
schemaVersion: Int = NO_SCHEMA_VERSION
) : this(
id = id, monitorId = monitor.id, monitorName = monitor.name, monitorVersion = monitor.version, monitorUser = monitor.user,
triggerId = trigger.id, triggerName = trigger.name, state = state, startTime = startTime,
lastNotificationTime = lastNotificationTime, errorMessage = errorMessage, errorHistory = errorHistory,
severity = trigger.severity, actionExecutionResults = listOf(), schemaVersion = schemaVersion,
aggregationResultBucket = null, findingIds = listOf(), relatedDocIds = listOf()
)

enum class State {
ACTIVE, ACKNOWLEDGED, COMPLETED, ERROR, DELETED
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ data class Monitor(
// Ensure that trigger ids are unique within a monitor
val triggerIds = mutableSetOf<String>()
triggers.forEach { trigger ->
// NoOpTrigger is only used in "Monitor Error Alerts" as a placeholder
require(trigger !is NoOpTrigger)

require(triggerIds.add(trigger.id)) { "Duplicate trigger id: ${trigger.id}. Trigger ids must be unique." }
// Verify Trigger type based on Monitor type
when (monitorType) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
package org.opensearch.commons.alerting.model

import org.opensearch.common.CheckedFunction
import org.opensearch.common.UUIDs
import org.opensearch.common.io.stream.StreamInput
import org.opensearch.common.io.stream.StreamOutput
import org.opensearch.common.xcontent.XContentParserUtils
import org.opensearch.commons.alerting.model.action.Action
import org.opensearch.core.ParseField
import org.opensearch.core.xcontent.NamedXContentRegistry
import org.opensearch.core.xcontent.ToXContent
import org.opensearch.core.xcontent.XContentBuilder
import org.opensearch.core.xcontent.XContentParser
import java.io.IOException

data class NoOpTrigger(
Copy link
Member

Choose a reason for hiding this comment

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

Is the only difference from a normal trigger is that it has no condition, so its technically always true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea it has id/name so that Alert's fields can be populated. Also, creating monitor with this trigger is not permitted.(require statement in init of monitor model)

override val id: String = UUIDs.base64UUID(),
override val name: String = "NoOp trigger",
override val severity: String = "",
override val actions: List<Action> = listOf(),
) : Trigger {

@Throws(IOException::class)
constructor(sin: StreamInput) : this()

override fun toXContent(builder: XContentBuilder, params: ToXContent.Params): XContentBuilder {
builder.startObject()
.startObject(NOOP_TRIGGER_FIELD)
.field(ID_FIELD, id)
.endObject()
.endObject()
return builder
}

override fun name(): String {
return NOOP_TRIGGER_FIELD
}

fun asTemplateArg(): Map<String, Any> {
return mapOf()
}

@Throws(IOException::class)
override fun writeTo(out: StreamOutput) {
}

companion object {
const val ID_FIELD = "id"
const val NOOP_TRIGGER_FIELD = "noop_trigger"
val XCONTENT_REGISTRY = NamedXContentRegistry.Entry(
Trigger::class.java, ParseField(NOOP_TRIGGER_FIELD),
CheckedFunction { parseInner(it) }
)

@JvmStatic @Throws(IOException::class)
fun parseInner(xcp: XContentParser): NoOpTrigger {
var id = UUIDs.base64UUID()
if (xcp.currentToken() == XContentParser.Token.START_OBJECT) xcp.nextToken()
if (xcp.currentName() == ID_FIELD) {
xcp.nextToken()
id = xcp.text()
xcp.nextToken()
}
if (xcp.currentToken() != XContentParser.Token.END_OBJECT) {
XContentParserUtils.throwUnknownToken(xcp.currentToken(), xcp.tokenLocation)
}
return NoOpTrigger(id = id)
}

@JvmStatic
@Throws(IOException::class)
fun readFrom(sin: StreamInput): NoOpTrigger {
return NoOpTrigger(sin)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ interface Trigger : BaseModel {
enum class Type(val value: String) {
DOCUMENT_LEVEL_TRIGGER(DocumentLevelTrigger.DOCUMENT_LEVEL_TRIGGER_FIELD),
QUERY_LEVEL_TRIGGER(QueryLevelTrigger.QUERY_LEVEL_TRIGGER_FIELD),
BUCKET_LEVEL_TRIGGER(BucketLevelTrigger.BUCKET_LEVEL_TRIGGER_FIELD);
BUCKET_LEVEL_TRIGGER(BucketLevelTrigger.BUCKET_LEVEL_TRIGGER_FIELD),
NOOP_TRIGGER(NoOpTrigger.NOOP_TRIGGER_FIELD);

override fun toString(): String {
return value
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import org.opensearch.commons.alerting.model.Finding
import org.opensearch.commons.alerting.model.Input
import org.opensearch.commons.alerting.model.IntervalSchedule
import org.opensearch.commons.alerting.model.Monitor
import org.opensearch.commons.alerting.model.NoOpTrigger
import org.opensearch.commons.alerting.model.QueryLevelTrigger
import org.opensearch.commons.alerting.model.Schedule
import org.opensearch.commons.alerting.model.SearchInput
Expand Down Expand Up @@ -395,7 +396,8 @@ fun xContentRegistry(): NamedXContentRegistry {
DocLevelMonitorInput.XCONTENT_REGISTRY,
QueryLevelTrigger.XCONTENT_REGISTRY,
BucketLevelTrigger.XCONTENT_REGISTRY,
DocumentLevelTrigger.XCONTENT_REGISTRY
DocumentLevelTrigger.XCONTENT_REGISTRY,
NoOpTrigger.XCONTENT_REGISTRY
) + SearchModule(Settings.EMPTY, emptyList()).namedXContents
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import org.opensearch.core.xcontent.ToXContent
import org.opensearch.index.query.QueryBuilders
import org.opensearch.search.builder.SearchSourceBuilder
import org.opensearch.test.OpenSearchTestCase
import java.time.Instant
import java.time.temporal.ChronoUnit
import kotlin.test.assertFailsWith

Expand Down Expand Up @@ -179,6 +180,16 @@ class XContentTests {
Assertions.assertEquals(trigger, parsedTrigger, "Round tripping BucketLevelTrigger doesn't work")
}

@Test
fun `test no-op trigger parsing`() {
val trigger = NoOpTrigger()

val triggerString = trigger.toXContent(builder(), ToXContent.EMPTY_PARAMS).string()
val parsedTrigger = Trigger.parse(parser(triggerString))

Assertions.assertEquals(trigger, parsedTrigger, "Round tripping NoOpTrigger doesn't work")
}

@Test
fun `test creating a monitor with duplicate trigger ids fails`() {
try {
Expand Down Expand Up @@ -357,6 +368,16 @@ class XContentTests {
assertEquals("Round tripping alert doesn't work", alert, parsedAlert)
}

@Test
Copy link
Member

Choose a reason for hiding this comment

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

Lets add xcontent tests for NoOp triggers.

fun `test alert parsing with noop trigger`() {
val monitor = randomQueryLevelMonitor()
val alert = Alert(
monitor = monitor, trigger = NoOpTrigger(), startTime = Instant.now().truncatedTo(ChronoUnit.MILLIS),
errorMessage = "some error", lastNotificationTime = Instant.now()
)
assertEquals("Round tripping alert doesn't work", alert.triggerName, "NoOp trigger")
}

@Test
fun `test alert parsing without user`() {
val alertStr = "{\"id\":\"\",\"version\":-1,\"monitor_id\":\"\",\"schema_version\":0,\"monitor_version\":1," +
Expand Down