-
Notifications
You must be signed in to change notification settings - Fork 81
Introduce Insights API #1610
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?
Introduce Insights API #1610
Conversation
Signed-off-by: Jackie <[email protected]>
Signed-off-by: Jackie <[email protected]>
Signed-off-by: Jackie <[email protected]>
Signed-off-by: Jackie <[email protected]>
Signed-off-by: Jackie <[email protected]>
Signed-off-by: Jackie <[email protected]>
4883d42 to
77fcda9
Compare
Signed-off-by: Jackie <[email protected]>
Signed-off-by: Jackie <[email protected]>
0eba385 to
49018e0
Compare
|
CI failed due to jacoco changes in build.gradle. Not sure how to fix. One naive way is to add correlation request, response, and Action in AD to avoid ml-commons dependency. |
kaituo
left a comment
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.
partial review
|
|
||
| InjectSecurity injectSecurity = new InjectSecurity(jobParameter.getName(), settings, localClient.threadPool().getThreadContext()); | ||
| try { | ||
| injectSecurity.inject(user, roles); |
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.
A normal user cannot query system index. Please add security 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.
updated in the new revision
| // Insights job | ||
| // ====================================== | ||
| // The Insights job name | ||
| public static final String INSIGHTS_JOB_NAME = "insights_job"; |
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.
how about changing to ad_insights_job in case we need forecasting job later?
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.
updated in the new revision
kaituo
left a comment
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.
partial review
| return ImmutableList | ||
| .of( | ||
| // Start insights job | ||
| new ReplacedRoute( |
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.
You don't need ReplaceRoute as this is a new API. TimeSeriesAnalyticsPlugin.AD_BASE_URI alone is enough.
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.
updated in the new revision
| builder.startObject(); | ||
|
|
||
| // Task metadata | ||
| builder.field("task_id", "task_" + ADCommonName.INSIGHTS_JOB_NAME + "_" + UUID.randomUUID().toString()); |
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.
Why do you need task id? AD task id is the doc id of state index.
|
|
||
| if (parts.length > 1) { | ||
| String seriesKey = parts[1]; | ||
| seriesKeys.add(seriesKey); |
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.
Is the entities set redundant with seriesKeys set?
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 don't necessarily need it. Just followed the current practice to have this logical run identifier for the insights generation. Maybe it's useful in the future when integrate with Investigation so we can refer to a specific insights run this this id.
src/main/java/org/opensearch/ad/ml/MLMetricsCorrelationInputBuilder.java
Show resolved
Hide resolved
| "fields": { | ||
| "raw": { | ||
| "type": "keyword", | ||
| "ignore_above": 32766 | ||
| } |
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 you need keyword?
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.
Not strictly, was following other AD index mapping to see how is it storing text field. Removed in the new revision
Signed-off-by: Jackie <[email protected]>
kaituo
left a comment
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.
partial review
| handleStartOperation(request, listener); | ||
| } else if (request.isStatusOperation()) { | ||
| handleStatusOperation(request, listener); | ||
| } else if (request.isStopOperation()) { | ||
| handleStopOperation(request, listener); |
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 you need to stash context before accessing job index (system index)? Please add security 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.
yes, I'm using stashed context when accessing job index, will add security tests
| .sort("generated_at", SortOrder.DESC) | ||
| ); | ||
|
|
||
| client.search(searchRequest, ActionListener.wrap(searchResponse -> { |
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 you need to add backend role filtering before search? Please add security tests with backend role filtering on.
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 need to do tenant-isolated search, but not necessarily backend role filtering here. For insights generation, it's a background job, so followed existing pattern to use InjectSecurity directly for background work, just impersonate the stored user via InjectSecurity, then execute search directly. For user-facing search APIs like search anomaly result transport action, AD reads the current user from thread context and then adds backend role filtering.
Adding security tests in the next revision
|
|
||
| private static final Logger log = LogManager.getLogger(InsightsJobProcessor.class); | ||
|
|
||
| private static InsightsJobProcessor INSTANCE; |
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.
add volatile. read https://en.wikipedia.org/wiki/Double-checked_locking
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.
good catch, we should do the same for ADJobProcessor and ForecastJobProcessor too
| try { | ||
| injectSecurity.inject(user, roles); | ||
|
|
||
| localClient |
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 should verify if mapping is changed by customer before writing. If yes, report error/stop job and stop writing.
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.
good catch, updated in the new revision
| Instant.now(), | ||
| lockDurationSeconds, | ||
| user, | ||
| ADCommonName.INSIGHTS_RESULT_INDEX_ALIAS, |
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.
Can you add this index in ADIndex? This would be consistent with other indexes.
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.
already added, check file in diff - src/main/java/org/opensearch/ad/indices/ADIndex.java
Signed-off-by: Jackie <[email protected]>
3076f15 to
1356920
Compare
Signed-off-by: Jackie <[email protected]>
kaituo
left a comment
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.
partial review
src/main/java/org/opensearch/ad/rest/handler/InsightsJobActionHandler.java
Show resolved
Hide resolved
| private void createNewJob(String frequency, User user, ActionListener<InsightsJobResponse> listener) { | ||
| try { | ||
| IntervalSchedule schedule = createSchedule(frequency); | ||
| long lockDurationSeconds = java.time.Duration.of(schedule.getInterval(), schedule.getUnit()).getSeconds() * 2; |
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.
lockDurationSeconds should be less than or equal to frequency, right?
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 good catch, updated it to
long lockDurationSeconds = java.time.Duration.of(schedule.getInterval(), schedule.getUnit()).getSeconds();
| Instant.now(), | ||
| null, | ||
| Instant.now(), | ||
| existingJob.getLockDurationSeconds(), |
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.
When you update frequency, you need to update lock duration as well.
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.
good catch, updated in the new revision
| null, | ||
| Instant.now(), | ||
| existingJob.getLockDurationSeconds(), | ||
| user != null ? user : existingJob.getUser(), |
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.
If we allow overwrite user, it would enable malicious user to update a job with. new user and thus cause original user to lose access.
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.
good catch, updated it to preserve the original job user and only fall back to the current request user if the job has no user
|
|
||
| log.info("Running Insights job for time window: {} to {}", executionStartTime, executionEndTime); | ||
|
|
||
| querySystemResultIndex(jobParameter, lockService, lock, executionStartTime, executionEndTime); |
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.
Only querying system result index would hinder your ability to go GA alone. You have to tie insights with Auto AD creation. One route to go to GA is to add a text box in AD overview page. That would add a summary on top of existing detectors' results.
Description
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
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.