Alerting Enhancements: Alerting Comments (Experimental) #663

toepkerd · 2024-05-29T20:40:38Z

Description

Common utils additions for the Alerting Comments experimental feature

Issues Resolved

opensearch-project/alerting#1500: direct request for adding Alert notes when acknowledging an Alert and showing who acknowledged an Alert.

This feature intends to be a superset of this ask, though adding comments or notes specifically when acknowledging an Alert will be a later extension.

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: Dennis Toepker <[email protected]>

src/main/kotlin/org/opensearch/commons/alerting/action/DeleteCommentRequest.kt

src/main/kotlin/org/opensearch/commons/alerting/action/DeleteCommentResponse.kt

src/main/kotlin/org/opensearch/commons/alerting/action/IndexCommentRequest.kt

src/main/kotlin/org/opensearch/commons/alerting/action/IndexCommentResponse.kt

src/main/kotlin/org/opensearch/commons/alerting/action/SearchCommentRequest.kt

src/test/kotlin/org/opensearch/commons/alerting/action/DeleteCommentRequestTests.kt

src/test/kotlin/org/opensearch/commons/alerting/action/DeleteCommentResponseTests.kt

Signed-off-by: Dennis Toepker <[email protected]>

eirsep · 2024-06-06T00:04:06Z

src/main/kotlin/org/opensearch/commons/alerting/action/AlertingActions.kt

@@ -21,6 +21,9 @@ object AlertingActions {
    const val SUBSCRIBE_FINDINGS_ACTION_NAME = "cluster:admin/opensearch/alerting/findings/subscribe"
    const val GET_MONITOR_ACTION_NAME = "cluster:admin/opendistro/alerting/monitor/get"
    const val SEARCH_MONITORS_ACTION_NAME = "cluster:admin/opendistro/alerting/monitor/search"
+    const val INDEX_COMMENT_ACTION_NAME = "cluster:admin/opendistro/alerting/alerts/comments/write"


no need for opendistro

check latest actions for naming convention

riysaxen-amzn · 2024-06-06T04:21:08Z

src/main/kotlin/org/opensearch/commons/alerting/model/Comment.kt

+        content = sin.readString(),
+        createdTime = sin.readInstant(),
+        lastUpdatedTime = sin.readOptionalInstant(),
+        user = if (sin.readBoolean()) {


use ternary op for better readability

Kotlin does not seem to have a ternary operator (https://discuss.kotlinlang.org/t/ternary-operator/2116), and favors directly using if else as it is more idiomatic. I can make it one line to resemble a ternary operator structure more.

riysaxen-amzn · 2024-06-06T04:21:48Z

src/main/kotlin/org/opensearch/commons/alerting/model/Comment.kt

+    val content: String,
+    val createdTime: Instant,
+    val lastUpdatedTime: Instant?,
+    val user: User?


will this be optional field?

lastUpdatedTime will be an optional field because upon a Comment's creation, it will not have a last updated time, only a created time. lastUpdatedTime is only assigned when a User makes a subsequent call to the same Comment to edit/update it. user will also be an optional field because there's no guarantee that the Security Plugin will be installed.

nullable because of security and without security

riysaxen-amzn · 2024-06-06T04:23:01Z

if you've have a github issue for this feature, add it in PR description for ref

Signed-off-by: Dennis Toepker <[email protected]>

AWSHurneyt · 2024-06-06T20:45:55Z

Looks good to me as long as other folks' comments are addressed. I've resolved the comments I left.

AWSHurneyt

@toepkerd Oh actually, could you confirm whether this should be merged to main or a feature branch?

src/main/kotlin/org/opensearch/commons/alerting/action/DeleteCommentRequest.kt

src/main/kotlin/org/opensearch/commons/alerting/action/DeleteCommentResponse.kt

src/main/kotlin/org/opensearch/commons/alerting/action/IndexCommentRequest.kt

src/main/kotlin/org/opensearch/commons/alerting/action/SearchCommentRequest.kt

eirsep · 2024-06-06T21:21:36Z

src/main/kotlin/org/opensearch/commons/alerting/model/Comment.kt

+    val content: String,
+    val createdTime: Instant,
+    val lastUpdatedTime: Instant?,
+    val user: User?


nullable because of security and without security

Signed-off-by: Dennis Toepker <[email protected]>

eirsep · 2024-06-10T19:47:40Z

src/main/kotlin/org/opensearch/commons/alerting/model/DataSources.kt

@@ -31,6 +31,12 @@ data class DataSources(
    /** Configures a custom index pattern for alertHistoryIndex alias.*/
    val alertsHistoryIndexPattern: String? = "<.opendistro-alerting-alert-history-{now/d}-1>", // AlertIndices.ALERT_HISTORY_INDEX_PATTERN

+    /** Configures a custom index alias to store comments associated with alerts.*/
+    val commentsIndex: String = ".opensearch-alerting-comments-history-write", // AlertIndices.COMMENTS_HISTORY_WRITE_INDEX


this should be optional??
existing usage of dataSources like SEcurity analytics will not be setting this field.

* initial commit, functional but needs refactoring Signed-off-by: Dennis Toepker <[email protected]> * added lastUpdatedTime to Note object Signed-off-by: Dennis Toepker <[email protected]> * changed name from alert id to entity id Signed-off-by: Dennis Toepker <[email protected]> * import cleanup Signed-off-by: Dennis Toepker <[email protected]> * changing name from Alerting Notes to Alerting Comments Signed-off-by: Dennis Toepker <[email protected]> * misc fixes and cleanup Signed-off-by: Dennis Toepker <[email protected]> * adding unit test coverage Signed-off-by: Dennis Toepker <[email protected]> * misc cleanup Signed-off-by: Dennis Toepker <[email protected]> * misc review-based cleanup Signed-off-by: Dennis Toepker <[email protected]> * added validation exception messages Signed-off-by: Dennis Toepker <[email protected]> --------- Signed-off-by: Dennis Toepker <[email protected]> Co-authored-by: Dennis Toepker <[email protected]> (cherry picked from commit acaa844) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* initial commit, functional but needs refactoring * added lastUpdatedTime to Note object * changed name from alert id to entity id * import cleanup * changing name from Alerting Notes to Alerting Comments * misc fixes and cleanup * adding unit test coverage * misc cleanup * misc review-based cleanup * added validation exception messages --------- (cherry picked from commit acaa844) Signed-off-by: Dennis Toepker <[email protected]> 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> Co-authored-by: Dennis Toepker <[email protected]>

toepkerd-zz added 4 commits May 26, 2024 17:09

initial commit, functional but needs refactoring

0fd6ac5

Signed-off-by: Dennis Toepker <[email protected]>

added lastUpdatedTime to Note object

c12827e

Signed-off-by: Dennis Toepker <[email protected]>

changed name from alert id to entity id

4e5a983

Signed-off-by: Dennis Toepker <[email protected]>

import cleanup

2c80bc1

Signed-off-by: Dennis Toepker <[email protected]>

toepkerd changed the title ~~Alerting enhancements~~ Alerting enhancements: Alerting Notes May 30, 2024

toepkerd marked this pull request as ready for review May 30, 2024 21:17

toepkerd requested review from lezzago, bowenlan-amzn, rishabhmaurya, getsaurabh02, eirsep, sbcd90, AWSHurneyt, engechas, riysaxen-amzn and jowg-amazon as code owners May 30, 2024 21:17

toepkerd changed the title ~~Alerting enhancements: Alerting Notes~~ Alerting enhancements: Alerting Notes (Experimental) May 30, 2024

toepkerd-zz added 2 commits June 4, 2024 16:37

changing name from Alerting Notes to Alerting Comments

e770578

Signed-off-by: Dennis Toepker <[email protected]>

misc fixes and cleanup

0ac563e

Signed-off-by: Dennis Toepker <[email protected]>

toepkerd changed the title ~~Alerting enhancements: Alerting Notes (Experimental)~~ Alerting enhancements: Alerting Comments (Experimental) Jun 5, 2024

toepkerd-zz added 2 commits June 5, 2024 08:24

adding unit test coverage

ae3754c

Signed-off-by: Dennis Toepker <[email protected]>

Merge branch 'main' into alerting-enhancements

a833a9f

AWSHurneyt reviewed Jun 5, 2024

View reviewed changes

misc cleanup

3b3566e

Signed-off-by: Dennis Toepker <[email protected]>

eirsep reviewed Jun 6, 2024

View reviewed changes

riysaxen-amzn reviewed Jun 6, 2024

View reviewed changes

misc review-based cleanup

9ad5a15

Signed-off-by: Dennis Toepker <[email protected]>

AWSHurneyt approved these changes Jun 6, 2024

View reviewed changes

AWSHurneyt requested changes Jun 6, 2024

View reviewed changes

AWSHurneyt previously approved these changes Jun 6, 2024

View reviewed changes

riysaxen-amzn previously approved these changes Jun 6, 2024

View reviewed changes

eirsep requested changes Jun 6, 2024

View reviewed changes

added validation exception messages

a5d8a47

Signed-off-by: Dennis Toepker <[email protected]>

toepkerd dismissed stale reviews from riysaxen-amzn and AWSHurneyt via a5d8a47 June 7, 2024 18:39

toepkerd changed the title ~~Alerting enhancements: Alerting Comments (Experimental)~~ Alerting Enhancements: Alerting Comments (Experimental) Jun 7, 2024

eirsep approved these changes Jun 7, 2024

View reviewed changes

AWSHurneyt approved these changes Jun 7, 2024

View reviewed changes

AWSHurneyt merged commit acaa844 into opensearch-project:main Jun 7, 2024
9 checks passed

toepkerd mentioned this pull request Jun 10, 2024

Adding EntityType to Comment Model #671

Merged

5 tasks

eirsep reviewed Jun 10, 2024

View reviewed changes

toepkerd mentioned this pull request Jun 10, 2024

adding constructor with optional comments indices args #674

Merged

5 tasks

AWSHurneyt added the backport 2.x label Jun 10, 2024

opensearch-trigger-bot bot mentioned this pull request Jun 10, 2024

[Backport 2.x] Alerting Enhancements: Alerting Comments (Experimental) #675

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alerting Enhancements: Alerting Comments (Experimental) #663

Alerting Enhancements: Alerting Comments (Experimental) #663

toepkerd commented May 29, 2024 •

edited

Loading

eirsep Jun 6, 2024

eirsep Jun 6, 2024

riysaxen-amzn Jun 6, 2024 •

edited

Loading

toepkerd Jun 6, 2024

riysaxen-amzn Jun 6, 2024

toepkerd Jun 6, 2024

eirsep Jun 6, 2024

riysaxen-amzn commented Jun 6, 2024

AWSHurneyt commented Jun 6, 2024

AWSHurneyt left a comment

eirsep Jun 6, 2024

eirsep Jun 10, 2024

Alerting Enhancements: Alerting Comments (Experimental) #663

Alerting Enhancements: Alerting Comments (Experimental) #663

Conversation

toepkerd commented May 29, 2024 • edited Loading

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riysaxen-amzn Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riysaxen-amzn commented Jun 6, 2024

AWSHurneyt commented Jun 6, 2024

AWSHurneyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toepkerd commented May 29, 2024 •

edited

Loading

riysaxen-amzn Jun 6, 2024 •

edited

Loading