- 
                Notifications
    
You must be signed in to change notification settings  - Fork 119
 
PPL Alerting: Get Alerts and Alert Lifecycle #1972
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Dennis Toepker <[email protected]>
Signed-off-by: Dennis Toepker <[email protected]>
| val monitorV2Id: String? | ||
| val monitorV2Ids: List<String>? | ||
| val alertV2Ids: List<String>? | ||
| val boolQueryBuilder: BoolQueryBuilder? | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we intend to support BoolQueryBuilder in API? user prolly wouldn't know Alert structure and this could be a security issue i reckon if user is able to query alerts by User fields,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was not an explicit product requirement, but I chose to include it with V1 users migrating to V2 in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can instead simplify the API to have similar functionality but not require them to create the query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have later decided to do this ^. I've actually removed boolQueryBuilder outright, as well as the alertV2Ids filter.
        
          
                alerting/src/main/kotlin/org/opensearch/alerting/actionv2/GetAlertsV2Request.kt
          
            Show resolved
            Hide resolved
        
      | @Throws(IOException::class) | ||
| override fun toXContent(builder: XContentBuilder, params: ToXContent.Params): XContentBuilder { | ||
| builder.startObject() | ||
| .field("alertV2s", alertV2s) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"v2alerts" ? also do we do camel_case or snake_case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will change to snake case, but will also retain the alerts v2 ordering of names
| override fun toXContent(builder: XContentBuilder, params: ToXContent.Params): XContentBuilder { | ||
| builder.startObject() | ||
| .field("alertV2s", alertV2s) | ||
| .field("totalAlertV2s", totalAlertV2s) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no page information? is this API paginated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less paginated and more capped. Users can specify a size param in the API to indicate how many alerts they want returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eirsep the total alerts is how many alerts there are and that can be higher than the alerts response sent. Then the use can use the start index in the request to get the next page. Though it would be good to indicate the start and end index in the response to make it easier to paginate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raised this Github issue to include start/end index in get alerts response: #1976
| actionListener.onFailure( | ||
| AlertingException.wrap( | ||
| OpenSearchStatusException( | ||
| "Alerting V2 is currently disabled, please enable it with the " + | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
users can't change cluster settings (in managed service atleast). this is not the expected behaviour imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will be pushing a CR that allowlists this setting to be adjustable by users.
We are also setting the flag to enabled by default, despite initial PPL Alerting launch being experimental.
| monitorV2Id: String?, | ||
| monitorV2Ids: List<String>? = null, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plz dont support both
this is a poor practice adopted in V1 due to BWC restrictions. We should remove monitorV2Id String
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, just use the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will remove individual monitorV2Id support
| import org.opensearch.index.query.BoolQueryBuilder | ||
| import java.io.IOException | ||
| 
               | 
          ||
| class GetAlertsV2Request : ActionRequest { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
serde unit tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding
Signed-off-by: Dennis Toepker <[email protected]>
Signed-off-by: Dennis Toepker <[email protected]>
… request and response Signed-off-by: Dennis Toepker <[email protected]>
        
          
                alerting/src/main/kotlin/org/opensearch/alerting/alertsv2/AlertV2Mover.kt
          
            Show resolved
            Hide resolved
        
              
          
                alerting/src/main/kotlin/org/opensearch/alerting/alertsv2/AlertV2Mover.kt
          
            Show resolved
            Hide resolved
        
              
          
                alerting/src/main/resources/org/opensearch/alerting/alertsv2/alert_v2_mapping.json
          
            Show resolved
            Hide resolved
        
      Signed-off-by: Dennis Toepker <[email protected]>
| // first collect all active alerts | ||
| val allAlertsSearchQuery = SearchSourceBuilder.searchSource() | ||
| .query(QueryBuilders.matchAllQuery()) | ||
| .size(10000) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens when there are more than 10,000 alerts? We should paginate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this Github issue to better handle the pagination case with scrolling: #1967
| val triggerV2Id = alertV2.triggerId | ||
| val triggeredTime = alertV2.triggeredTime.toEpochMilli() | ||
| 
               | 
          ||
| val expireDuration = triggerToExpireDuration[triggerV2Id] | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we confirmed we will use the latest trigger expiration time here? So even if the alert was created when the expiration was 1 year and got changed to 1 day, it will expire after a day the alert was generated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we have received product-side confirmation that alert expiration will happen at the trigger level and not alert level.
| 
               | 
          ||
| val alertsSearchQuery = SearchSourceBuilder.searchSource() | ||
| .query(boolQuery) | ||
| .size(10000) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pagination?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this Github issue to better handle the pagination case with scrolling: #1967
| 
           There seems to be no integration tests on alerts. We need normal integration tests and security tests which includes RBAC.  | 
    
| 
           Alert ITs are included here: https://github.com/toepkerd/alerting/blob/ppl-main/alerting/src/test/kotlin/org/opensearch/alerting/alertsv2/AlertV2IndicesIT.kt They are not published in this PR because execute monitor functionality hasn't yet been published. However, I believe Get Alerts ITs for RBAC are still in order, will add those. EDIT: added, see description  | 
    
Signed-off-by: Dennis Toepker <[email protected]>
Description
Get Alerts and Alert Lifecycle
ITs depend on execute monitor functionality that has not yet been published, but they have been written:
Alerts Lifecycle ITs: https://github.com/toepkerd/alerting/blob/ppl-main/alerting/src/test/kotlin/org/opensearch/alerting/alertsv2/AlertV2IndicesIT.kt
Get Alerts RBAC ITs: https://github.com/toepkerd/alerting/blob/ec4672e6321f7665e5059d7103f619725d2eb254/alerting/src/test/kotlin/org/opensearch/alerting/resthandler/SecureMonitorV2RestApiIT.kt#L629
Related Issues
#1880
Check List
--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.