Adding functionality to associate existing detector with a visualization

[amitgalitz]

Description

Adding a flyout to associate existing visualization with a detector, and solving all issues here #490

Screenshot:

resolves #490

Check List

• Commits are signed per the DCO using --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.

[ohltyler]

Changes LGTM, will wait to approve when #487 is merged

[amitgalitz]

@ohltyler just want to double check, did you take a look here: https://github.com/opensearch-project/anomaly-detection-dashboards-plugin/pull/484/files#diff-8ec1ba31bcc4da742d5f9cf13babe0c91adc8b00a9b5066e0c51cb47d8df60f8R260-R311?

[amitgalitz]

Right now I have the constants for the saved objects set like this:
For the plugin resource I think I should change the type here to "Anomaly Detectors" actually (using https://github.com/opensearch-project/anomaly-detection-dashboards-plugin/blob/featureAnywhere/public/expressions/constants.ts#LL9C14-L9C35)
do you agree?

const pluginResource = {
      type: 'anomaly-detection',
      id: detector.id,
    } as ISavedPluginResource;

    const savedObjectToCreate = {
      title: embeddable.vis.title,
      originPlugin: ORIGIN_PLUGIN_VIS_LAYER,
      pluginResource: pluginResource,
      visId: embeddable.vis.id,
      savedObjectType: 'visualization',
      visLayerExpressionFn: fn,
    } as ISavedAugmentVis;
   

[ohltyler]

@ohltyler just want to double check, did you take a look here: https://github.com/opensearch-project/anomaly-detection-dashboards-plugin/pull/484/files#diff-8ec1ba31bcc4da742d5f9cf13babe0c91adc8b00a9b5066e0c51cb47d8df60f8R260-R311?

This link doesn't work for me, can you point me to the file/lines you want me to look at?

[ohltyler]

Right now I have the constants for the saved objects set like this: For the plugin resource I think I should change the type here to "Anomaly Detectors" actually (using https://github.com/opensearch-project/anomaly-detection-dashboards-plugin/blob/featureAnywhere/public/expressions/constants.ts#LL9C14-L9C35) do you agree?

const pluginResource = {
      type: 'anomaly-detection',
      id: detector.id,
    } as ISavedPluginResource;

    const savedObjectToCreate = {
      title: embeddable.vis.title,
      originPlugin: ORIGIN_PLUGIN_VIS_LAYER,
      pluginResource: pluginResource,
      visId: embeddable.vis.id,
      savedObjectType: 'visualization',
      visLayerExpressionFn: fn,
    } as ISavedAugmentVis;
   

Yes. We can have this as a constant somewhere to ensure. You could add a comment about it along the lines of "this value will be exposed when listing out the different plugin resource types in the view events page"

[amitgalitz]

@ohltyler just want to double check, did you take a look here: https://github.com/opensearch-project/anomaly-detection-dashboards-plugin/pull/484/files#diff-8ec1ba31bcc4da742d5f9cf13babe0c91adc8b00a9b5066e0c51cb47d8df60f8R260-R311?

This link doesn't work for me, can you point me to the file/lines you want me to look at?

File public/components/FeatureAnywhereContextMenu/CreateAnomalyDetector/AddAnomalyDetector.tsx, line 260-311

[ohltyler]

@ohltyler just want to double check, did you take a look here: https://github.com/opensearch-project/anomaly-detection-dashboards-plugin/pull/484/files#diff-8ec1ba31bcc4da742d5f9cf13babe0c91adc8b00a9b5066e0c51cb47d8df60f8R260-R311?

This link doesn't work for me, can you point me to the file/lines you want me to look at?

File public/components/FeatureAnywhereContextMenu/CreateAnomalyDetector/AddAnomalyDetector.tsx, line 260-311

Just one comment, else LGTM.

[codecov]

Codecov Report

Merging #484 (fa45fd8) into featureAnywhere (cb349ce) will decrease coverage by 0.88%.
The diff coverage is n/a.

❗ Current head fa45fd8 differs from pull request most recent head 15dc68d. Consider uploading reports for the commit 15dc68d to get more accurate results

@@                 Coverage Diff                 @@
##           featureAnywhere     #484      +/-   ##
===================================================
- Coverage            49.00%   48.13%   -0.88%     
===================================================
  Files                  164      165       +1     
  Lines                 5401     5524     +123     
  Branches              1039     1061      +22     
===================================================
+ Hits                  2647     2659      +12     
- Misses                2494     2605     +111     
  Partials               260      260              

see 9 files with indirect coverage changes

[ohltyler]

Similar to the conditional on line 830, we can do the same here for these 2 exclusive scenarios.

[amitgalitz]

mode can also be 'associated'

[ohltyler]

What is the difference between existing and associated? I'm thinking if we can have these in constants with details about each scenario it would be helpful.

[amitgalitz]

existing is the one with the button to associate an existing detector flyout and associated is the flyout with the list of all currently associated detectors with ability to unlink. I can add some comments and constants, will do that in my final commit after fixing the dependency issue

[jackiehanyang]

what is this {' '} means here?

[amitgalitz]

removed

[jackiehanyang]

we have a setup alert button on existing AD detector overview page, is this how we currently check if alerting exist? also what does ALERTING_TRIGGER_AD_ID mean here?

[amitgalitz]

We actually don't have this logic on the setup alert button in AD overview page. If there is no alerting plugin, the button still shows and it just sends to a broken page.
ALERTING_TRIGGER_AD_ID is the trigger that David set up on the alerting code to open up the relevant flyout. If the alerting trigger doesn't exist then it means there is no alerting plugin or potentially something else going on but regardless it means the flyout for creating a monitor on top of a detector wont be opened

[ohltyler]

We actually don't have this logic on the setup alert button in AD overview page. If there is no alerting plugin, the button still shows and it just sends to a broken page.

I believe we used to have a check here. Regardless, this is bad UX. Can you open an issue on that. We can probably keep it broad, like "clean up how to detect alerting plugin is installed"

[amitgalitz]

Opened an issue #493

[jackiehanyang]

here's a way to make the toast wider:

1. Add the following style in the styles.scss file

.euiGlobalToastList {
  width: 500px;
}

.successToast {
  width: 450px;
  position: relative;
  right: 25px;
}

2. add this line inside the notifictions.toasts.addSuccess...
   className: "successToast",

[jackiehanyang]

[amitgalitz]

Thank you, made the fix

[jackiehanyang]

is this console line needed?

[jackiehanyang]

if not, you can remove it in the next pr