-
Notifications
You must be signed in to change notification settings - Fork 58
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
Migrate plugin to be compatible with OpenSearch Dashboards #1
Conversation
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.
Some remaining opendistro's that we might not want. Would search the codebase for more.
ODFE_VERSION: 1.13.1 | ||
AD_KIBANA_PLUGIN_NAME: opendistroAnomalyDetectionKibana | ||
AD_OPENSEARCH_DASHBOARDS_PLUGIN_NAME: opendistroAnomalyDetectionOpenSearchDashboards |
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.
Should this be anomalyDetectionOpenSearchDashboards
or even anomalyDetectionDashboards
?
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 taking a look. Regarding the remaining opendistro
s and plugin name: I believe there is nothing finalized here regarding plugin naming convention changes, so have just simply replaced Kibana
with OpenSearchDashboards
for now. I was also told there is no change of branding for opendistro for now, which is why I have left it for now. Let me reach out again to get some clarification on this.
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.
@dblock have changed the plugin name (as seen in the url) to anomaly-detection-dashboards
, and build zip to anomalyDetectionDashboards
, and have removed remaining mentions of opendistro
/ open-distro
/ Open Distro for Elasticsearch
, with a few exceptions:
- Sample detectors and corresponding indices: this will be a very involved change that will affect current users if we change these names, lots to consider regarding backwards compatibility, etc. Will consider this in a follow-up PR. Have created Update sample detectors names & indices #3 to track)
- Release notes - will leave these as-is until a new standard format is decided upon in the future
- Links to documentation (currently still using opendistro website for this)
- References to backend indices, which will be changed in the future
- Left all references to
odfe
for now until there is a new standard acronym
Have re-ran all unit & integration tests; sanity tested workflows via browser.
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.
Nice work. I'll re-read the PR for typos.
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.
The only question I have is around ODFE that you called out above, for things like volume names in docker containers. Isn't that just becoming opensearch
? I don't think it makes sense to continue abbreviating that to, say, os
, that would be super confusing.
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.
@dblock as discussed, we will not have a separate acronym for OpenSearch, and can replace odfe
with opensearch
. Have made those changes in the latest commit. Re-ran tests + did another round of sanity testing, looks good. No remaining instances of odfe
besides release notes.
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.
@ohltyler Except for the sample detectors, we need to consider migrating all AD ODFE indices to open search ? And change ODFE AD URL /_opendistro/_anomaly_detection
to /_opensearch/_anomaly_detection
.
Do we have some issue to track these? It needs both backend and frontend changes.
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.
And we need to call some alerting APIs like search alerts, remember to change this part once alerting API changes done.
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, the changes regarding indices / api changes from opendistro
-> opensearch
is being treated as a separate step. Current efforts are to make the ODFE plugins compatible with OpenSearch / OpenSearch Dashboards first.
…to anomalyDetectionDashboards; remove most instances of opendistro / open distro
public/pages/HistoricalDetectorList/containers/HistoricalDetectorList.tsx
Outdated
Show resolved
Hide resolved
Should we change the ODFE doc link now? Like this one : https://github.com/opendistro-for-elasticsearch/anomaly-detection-kibana-plugin/blob/main/public/pages/AnomalyCharts/components/AlertsFlyout/AlertsFlyout.tsx#L89 |
We have not. I was told we are ok to continue referencing the Open Distro docs for now. |
Do we plan to keep ODFE repo in future? If yes, will we commit any bug fix/patch to it or just keep it immutable? If not, should we remove or modify the ODFE issue link in code and release code? |
Yes, bug fixes may be backported to the Open Distro repo for current users. The issues and projects can be moved to this repo once it is public to move community development focus here. |
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.
Looks good to me. Thanks for the change.
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.
LGTM. Thanks for the change!
uses: actions/checkout@v2 | ||
with: | ||
path: kibana/plugins/anomaly-detection-kibana-plugin | ||
path: OpenSearch-Dashboards/plugins/anomaly-detection-dashboards-plugin |
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.
Should this have been lower-case opensearch-dashboards? It's a path.
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 cloning the repo via the checkout action, the dir is OpenSearch-Dashboards
, which is different than the cloned Kibana repo which was just kibana
. Will confirm if it needs to be changed once these workflows are runnable.
This PR migrates the plugin to be compatible with OpenSearch Dashboards, and be able to work with the OpenSearch Anomaly Detection plugin. Compatible with OpenSearch Dashboards 7.10.2.
Major changes include:
package.json
and as shown in the url via browser) fromopendistro-anomaly-detection-kibana
->anomaly-detection-dashboards
opendistroAnomalyDetectionKibana
->anomalyDetectionDashboards
Kibana
changed toOpenSearch Dashboards
ES
/Elasticsearch
changed toOpenSearch
kbn
changed toosd
odfe
toopensearch
KibanaRequest
/IKibanaResponse
toOpenSearchDashboardsRequest
/IOpenSearchDashboardsResponse
, respectivelyOpen Distro for Elasticsearch
toOpenSearch Plugins
(this is not a finalized change)What works:
What doesn't work: