[Extensions] Migrates Start/Stop Historical Analysis for single-entity and multi-entity detectors#880
Conversation
…ction, ADBatchAnomalyResultTransportAction, replaces Internal Aggregation classes with corresponding Parsed Aggregation class. removes hashring from historical analysis workflow Signed-off-by: Joshua Palis <jpalis@amazon.com>
…Client to bulk index anomaly results, replaces transport service calls with corresponding client execute calls. Adds check for request content for job details registration, this prevents historical task requests from triggering job details registration Signed-off-by: Joshua Palis <jpalis@amazon.com>
…adds transport action for stop historical detector Signed-off-by: Joshua Palis <jpalis@amazon.com>
Signed-off-by: Joshua Palis <jpalis@amazon.com>
Signed-off-by: Joshua Palis <jpalis@amazon.com>
…e no constructor to access via reflection Signed-off-by: Joshua Palis <jpalis@amazon.com>
dbwiddis
left a comment
There was a problem hiding this comment.
LGTM, a comment and question.
| ); | ||
|
|
||
| // Ensure job details are registered with Job Scheduler prior to creating a job, no-op for historical request | ||
| if (rawPath.endsWith(RestHandlerUtils.START_JOB) && !registeredJobDetails && !request.hasContent()) { |
There was a problem hiding this comment.
This works, but I'm wondering if we couldn't leverage the route handlers to do this first conditional.
You could have a handleStartRequest and handleStopRequest; both delegating to this prepareRequest() method with an argument defining whether it's a start.
There was a problem hiding this comment.
This is a good idea, ill make this change
| } | ||
|
|
||
| // Ensure job details are registered with Job Scheduler prior to creating a job | ||
| if (!registeredJobDetails) { |
There was a problem hiding this comment.
It doesn't look like the previous version conditioned on it being a start job or having content. For my own education, why is it necessary for us to add that in the conditional (vs. handling these conditions in theregisterJobDetails() method itself?)
There was a problem hiding this comment.
I can add these checks in the registerJobDetails method. Previously it was not necessary to check if the request was a start job request or if the request had content. Now that we've enabled historical analysis, it is necessary to prevent these routes from registering job details with Job Scheduler as only real time analysis needs to trigger this registration.
owaiskazi19
left a comment
There was a problem hiding this comment.
Overall looks good. Let's not remove the hashring code but comment it to not lose track while working on multi node support.
src/main/java/org/opensearch/ad/transport/handler/AnomalyResultBulkIndexHandler.java
Show resolved
Hide resolved
| // getColdStartSamplesForPeriodsTemplate(DocValueFormat.RAW); | ||
| // } | ||
|
|
||
| /* @anomaly-detection commenting as there are no constructors for corresponding parsed aggregation classes |
There was a problem hiding this comment.
Can we see how OpenSearch is testing parsed aggregation classes?
There was a problem hiding this comment.
sure Ill spend some time today to explore how to test parsed aggregation classes
Signed-off-by: Joshua Palis <jpalis@amazon.com>
….com/opensearch-project/OpenSearch/pull/7309 Signed-off-by: Joshua Palis <jpalis@amazon.com>
Signed-off-by: Joshua Palis <jpalis@amazon.com>
Signed-off-by: Joshua Palis <jpalis@amazon.com>
Signed-off-by: Joshua Palis <jpalis@amazon.com>
Signed-off-by: Joshua Palis <jpalis@amazon.com>
Signed-off-by: Joshua Palis <jpalis@amazon.com>
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (34.96%) is below the target coverage (70.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## feature/extensions #880 +/- ##
========================================================
+ Coverage 34.94% 34.96% +0.01%
+ Complexity 1895 1887 -8
========================================================
Files 300 300
Lines 17915 17843 -72
Branches 1869 1862 -7
========================================================
- Hits 6261 6238 -23
+ Misses 11201 11156 -45
+ Partials 453 449 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| logger.debug("Feature aggregation result size {}", aggregations.size()); | ||
| for (Aggregation agg : aggregations) { | ||
| List<InternalComposite.InternalBucket> buckets = ((InternalComposite) agg).getBuckets(); | ||
| List<ParsedComposite.ParsedBucket> buckets = ((ParsedComposite) agg).getBuckets(); |
There was a problem hiding this comment.
@owaiskazi19 @joshpalis shouldn't we document these changes?
There was a problem hiding this comment.
sure I can add this to the migration documentation
Description
Migrates the following actions to enable starting and stopping historical analysis for single/multi entity detectors :
ForwardADTaskActionADBAtchAnomalyResultActionADCAncelTaskActionRemoves all instances of the
hashringand directs all request back to the local node (extension node), and replaces all internal aggregation/bucket/metrics classes with their corresponding Parsed classes due to the use of theSDKRestClientThe following logs show historical anomaly detection of data generated by the AD DataGeneration script using the following configuration :
python3 generate-cosine-data-multi-entity.py -ep localhost -i server_log --shards 5 -t 10 --no-security -nh 5 -np 5 --ingestion-frequency 10 --points 30000We can retrieve the start and end times for historical detection by retrieving the earliest and latest entries like so, and converting the
@timestampvalue fromISO8601format toUNIX epoch milliseconds:Logs for starting
single-entityhistorical detectionLogs for starting
multi-entityhistorical detection (Historical HCAD)Logs for stopping historical detection
Issues Resolved
Completes : opensearch-project/opensearch-sdk-java#383
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.