-
Notifications
You must be signed in to change notification settings - Fork 71
Implmentation of contextual launch #1005
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
Conversation
Signed-off-by: Jackie Han <[email protected]>
Signed-off-by: Jackie Han <[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.
Can you write some ITs or UTs to cover your functionality?
public/pages/DetectorResults/containers/AnomalyResultsTable.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Jackie Han <[email protected]>
Added a UT file. There was no existing UT setup for the file I touched, so I originally planned to cover it with an IT. However, after reviewing it, I felt it would be better to have UTs for this feature. So I set up a new file to add unit tests for the AnomalyResultsTable page. |
Signed-off-by: Jackie Han <[email protected]>
public/pages/DetectorResults/containers/AnomalyResultsTable.tsx
Outdated
Show resolved
Hide resolved
| const startISO = new Date(startTime - TEN_MINUTES_IN_MS).toISOString(); | ||
| const endISO = new Date(endTime + TEN_MINUTES_IN_MS).toISOString(); | ||
|
|
||
| const basePath = `${window.location.origin}${window.location.pathname.split('/app/')[0]}`; |
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.
instead you could use the core service that is accessible by every plugin getUrlForApp
https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/core/public/application/types.ts#L826
so like
window.open(getUrlForApp(def.appId), '_blank', 'noopener, noreferrer')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.
Thanks for the suggestion, I didn't know we have such method :)
I would leave it as the current implementation because these two methods work exactly the same, but the one that uses the core service adds extra burden when writing unit tests, as it needs to be mocked.
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 you plan on mock the services to be super basic, you should be able to use the helper methods which should instantiate all the mocks for those services
https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/core/public/mocks.ts#L200
for example,
import { coreMock } from '../../../../../core/public/mocks';
const startMock = coreMock.createStart();|
Test failed: Test Suites: 1 failed, 1 skipped, 85 passed, 86 of 87 total |
Signed-off-by: Jackie Han <[email protected]>
The failed test case is flaky, so I deleted it in the latest revision. I tried to capture the error message in the inner try catch block but sometimes the error message from the outer try catch block is being captured, so it's flaky. |
the error looks like related to AnomalyDetailsChart when rendering the chart as some field value is off instead of contextual launch. But let me look into it. |
Signed-off-by: Jackie Han <[email protected]>
fixed in the new revision, the current implementation works with remote index use case, just needed also update the props name in the parent file when changing the props name. Fixed in the latest revision and added a test case for it |

Description
This PR implements the contextual launch feature. It first checks whether an index pattern with a title matching the indices used to create the detector already exists. If it does, the index pattern id is retrieved and included in the Discover page URL. If not, a new index pattern is created using all the indices that were used to create the detector, and its id is then included in the Discover page URL.
Issues Resolved
[List any issues this PR will resolve]
Check List
--signoffBy 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.