Migrate Alerting Kibana pluign to the new Kibana Platform #209

ftianli-amzn · 2020-11-18T01:18:55Z

Issue #, if available:
#208

Description of changes:

Migrate the whole plugin to the new Kibana platform.
Compatible with Kibana 7.9.1
Reference: the offical migration guide https://github.com/elastic/kibana/blob/master/src/core/MIGRATION.md

Main changes:

Change the entry-point and definition of the the browser side code and server side code, now, there is no architectural difference between a plugin in the browser and a plugin on the server.
Create a new manifest file kibana.json, which used for identifying the plugin, instead of package.json
In browser side: Create a CoreContext to replace the existing AppContext to share the core services globally. Most common services in Kibana are provided by core now, such as chrome, http, notifications, and uiSettings etc. I'm using props to pass the values of them to other components.
In server side: Change the way of adding new routes for the plugin APIs, and the way of handling the routes to what Kibana defines, because hapi.js server and request objects are not exposed to the plugin anymore.

Testing:

Unit tests passed.
Installed the plugin in an ODFE 1.11 cluster and did manual testing:
Almost all the APIs with users in different backend role, and the style in dark mode.

Note:
Please use yarn start or yarn start -oss in the Kibana root directory to start Kibana with the pluign.
If you want to build the plugin, please do after running yarn start (to get the target folder generated).

TODOs:

Review the CSS style in public/less/main.less and migrate the necessary style to SASS, as Less is not supported in Kibana "new platform" anymore.
Correct outdated content in TRANSLATION.Md.
Review "scripts" in package.json
Review .kibana-plugin-helpers.json after upgrading to compatible with Kibana 7.10

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…lar with original

… side

…ht path, and using the correct http requst path in Monitors.test.js

…eationDestination

… about email and 1 about ad

…-vis back

ohltyler · 2020-11-18T22:39:37Z

public/pages/CreateMonitor/containers/DefineMonitor/DefineMonitor.js

@@ -279,7 +283,7 @@ class DefineMonitor extends Component {
      content = (
        <ExtractionQuery
          response={JSON.stringify(response || '', null, 4)}
-          isDarkMode={this.isDarkMode}
+          isDarkMode={this.context.isDarkMode}


any reason why you pass other core services (notifications, httpClient) via props, but get isDarkMode via context here instead?

I guess the same question goes for all places where you are using context instead of props.

Thanks for taking look at the PR.
It depends on the frequency of using these property. The property isDarkMode is used very few, so I didn't passed it by props.
httpClient is passed via props everywhere since the plugin created, and I didn't change that to prevent making the PR bigger.
For notifications, it's used frequently, so I passed it with httpClient by props. While, in the files related to "Email notification", notifications needs to be passed very deeply, where it's not needed in the middle ,so I used context instead.

It looks a little messy. If needed, I think I could pass them all via "props" to keep the PR simple.

I see, makes sense. Personally, I think for overall style it would probably be better to do one or the other - if some services need to be passed deeply, I think a good change could be removing the props and consuming everything via context like what you've done for isDarkMode. More info here.

Could be a separate PR later to try to standardize this, just my thoughts

You are right, the style should be consistent. As all the httpClient has been passed by "props", I have changed all the notifications and isDarkMode to be passed by "props". 😂. Btw, the AD related code in this repo is using the Context, and that's a reference to me.

…lder did the job

ohltyler · 2020-11-18T22:47:06Z

public/pages/Destinations/containers/CreateDestination/EmailRecipients/utils/helpers.js

-    if (response.data.ok) {
-      return response.data.emailGroups;
+    const response = await httpClient.get('../api/alerting/destinations/email_groups', {
+      query: { search: searchText, size: 200 },


unrelated to these changes - rather than hardcoding a limit of 200 here, declare as a constant somewhere instead? Seems there's a few places where limits like this are hardcoded

Thanks. Make sense. I think I can change it based on your idea in another PR.

ohltyler · 2020-11-18T23:35:18Z

public/pages/Destinations/containers/DestinationsList/DestinationsList.js

@@ -113,11 +113,14 @@ class DestinationsList extends React.Component {

  isDeleteAllowed = async (type, id) => {
    const { httpClient } = this.props;
-    const resp = await httpClient.post('../api/alerting/monitors/_search', {
+    const reuqestBody = {


minor: typo here

Good catch, I corrected it. Thanks.

ohltyler · 2020-11-18T23:37:36Z

public/plugin.js

+      id: PLUGIN_NAME,
+      title: 'Alerting',
+      description: 'Kibana Alerting Plugin',
+      // icon: `plugins/${PLUGIN_NAME}/images/alerting_icon.svg`,


minor: probably fine to remove

No problem 👍. I have removed.

server/clusters/alerting/createAlertingCluster.js

ohltyler · 2020-11-18T23:43:15Z

server/plugin.js

+    const alertingESClient = createAlertingCluster(core, globalConfig);
+    const adESClient = createAlertingADCluster(core, globalConfig);


If you remove createAlertingADCluster, you could combine these into one client as well. I guess this is more of a choice on how you want to modularize them. It seems fine the way it is too. What do you think?

I'm fine with combining the two files, if it works well. As I said, I didn't think much but try to keep as it is.😄 Yaliang added the file https://github.com/opendistro-for-elasticsearch/alerting-kibana-plugin/blame/master/server/clusters/alerting/createAlertingADCluster.js in the initial release of Anomaly Detection plugin.

Well, I will ask his idea, and I found he didn't separate the 2 plugins in 2 clusters in AD's repo. 😅

Got an idea from Yaliang. The advantage of separating the two files is that the two plugins can be modified separately, IMO, like the Header configuration. (Though AD didn't separate the 2 cluster clients ... ) This part is written by Mihir originally, I think I will remain it for now.

Sure, makes sense, sounds good

…d inconsistency

lezzago · 2020-11-19T18:50:52Z

public/pages/MonitorDetails/containers/MonitorHistory/MonitorHistory.js

-      const resp = await httpClient.get(`../api/alerting/alerts?${queryString.stringify(params)}`);
+      const resp = await httpClient.get('../api/alerting/alerts', { query: params });


Do we need to stringify this?

This is a little wired, in the HTTP request there can be params, query, and body, 3 types of value. Only the value of body needs to be formatted to JSON string. I didn't take much look into the source code, but just tried several times and find that.

Ok that is good to know, thanks

lezzago · 2020-11-19T19:05:18Z

server/routes/monitors.js

+          from: schema.number(),
+          size: schema.number(),
+          search: schema.string(),
+          sortField: schema.string(),
+          sortDirection: schema.string(),
+          state: schema.string(),


Are these fields always provided in Kibana when we call this api?

There is only one place calling this API, which aims to get all the monitors to show in the dashboard.

alerting-kibana-plugin/public/pages/Monitors/containers/Monitors/Monitors.js

Line 142 in 32e202f

const params = { from, size, search, sortField, sortDirection, state };

. The fields are all provided.
Btw, I write down all the field just make the validation to be accurate, we could use { query: schema.any()} at anytime to pass the validation.

lezzago · 2020-11-19T19:09:19Z

server/services/AlertService.js

 export default class AlertService {
  constructor(esDriver) {
    this.esDriver = esDriver;
  }

-  getAlerts = async (req, h) => {
+  getAlerts = async (context, req, res) => {


Why do we need context if it is not being used?

It's required params by the new route handler signature, details here

I just tested that there will be Error: Internal Server Error without it.

ylwu-amzn · 2020-11-20T02:54:48Z

kibana.json

@@ -0,0 +1,9 @@
+{
+  "id": "opendistroAlerting",
+  "version": "1.11.0.2",


Should we bump to 1.12.0.0 ?

I will do the version upgrade in another PR. Keeping the version number as 1.11 makes me possible to test the functionality in an ODFE 1.11 cluster after the migration. 😁

ylwu-amzn · 2020-11-20T02:56:24Z

kibana.json

+{
+  "id": "opendistroAlerting",
+  "version": "1.11.0.2",
+  "kibanaVersion": "kibana",


Specify kibana version as "kibana" will auto resolve as 7.10?

Hi Yaliang, thanks a lot for reviewing my PR. In my mind, it will resolve as the version defined in package.json. Let me find some references.

It comes from "kbn-plugin-generator", in the template of the sample Kibana plugin, the value of "kibanaVersion" is set to "kibana". (https://github.com/elastic/kibana/blob/master/packages/kbn-plugin-generator/template/kibana.json.ejs).
As far as I see, it will get the version number from package.json.
Unfortunately, there is not much explanation for the manifest from Kibana. https://github.com/elastic/kibana/blob/master/docs/development/core/server/kibana-plugin-core-server.pluginmanifest.md

Got it, it's ok if the build and yarn start test pass

Thank you.👍 For the "kibanaVersion", probably it's not getting the version from "package.json" 🤔, because when I ran yarn build, the script prompts me to input the Kibana version manually.
I will investigate more about the "kibanaVersion", but it doesn't affect building the plugin.

Interesting, you may check @yizheliu-amazon 's build changes here - looks like you will want to change the serverSourcePattern in .kibana-plugin-helpers.json, and specify the plugin and kibana version in the Kibana manifest file. Not sure how the version verification and build naming has changed, with both the npm package.json and the Kibana manifest kibana.json specifying similar things now.

Hi Tyler, thank you for your reminder. I didn't notice there is the name change in .kibana-plugin-helpers.json. 👍

I will change the "kibanaVersion" to the exact version number to avoid inputting version number manually when building through plugin-hlepers in another PR for ODFE 1.12.
As it's written in the migration guide, seems package.json doesn't relate much to the build process.

Cool. Also, per the new naming convention, can you change opendistroAlerting to opendistroAlertingKibana?

Thanks for pointing out. Sure, I just changed it in kibana.json in another branch, and will be in the following PR.

Tianli Feng added 30 commits October 23, 2020 17:33

Bump version to 1.11.0.2

5f28e97

wip: migrating to new platform

6dd8dee

wip: migrating to new platform, with API 404 server error

d65ef1a

Fix get alerts, get monitors and create monitor APIs

6cb4717

Fix update monitor api

12bd463

Temporarily add target folder into .gitignore

8fb49bd

Fix api: execute monitor

6351a01

Fix toasts and execute monitor api, change some variable names

37ca8d7

Override headers when setting up the server-side plugin in new platform

755b46e

reformat the way of creating cluster client in server-side to be simi…

3fde6e2

…lar with original

Fix Destination APIs, and correct the data path of http responses

a34d7fb

Fix Destination APIs: correct some validation of the routes in server…

fd0a9b7

… side

Fix AnomalyDetector APIs

71db812

Fix ack alert API

32e202f

Fix the unit tests for getting the data from http response in the rig…

9acf695

…ht path, and using the correct http requst path in Monitors.test.js

Remove the temporary build script

c33e819

Fix unit tests in CreateMonitor and Breadcrumbs

361394a

Fix unit tests in CreateMonitor and MonitorIndex and validation of Cr…

2ab8bd4

…eationDestination

Fix unit tests: correct the data path of http response in MonitorIndex

0e81849

Fix unit tests and add mocks for core services

045e51f

remove a console.log()

b54f75f

restore content of package.json

148f177

clean up: remove comment out codes

f135af1

update kibana-plugin-helpers to adapt with new platform

618b447

fix toast notifications escecially in email pages, and correct 2 apis…

2012070

… about email and 1 about ad

set breadcrumb for alerting plugin and import the css style for react…

71b5731

…-vis back

remove the comment out codes in app.js

d61d0e2

remove 2 console.log() that used for testing

ee4b086

refactor the core context and related props

841b4b2

refactor the core context and related props

f3f5aaa

remove unused import of 'coreMock' in unit tests

0f55cbd

ftianli-amzn requested a review from qreshi November 18, 2020 05:19

Tianli Feng added 4 commits November 18, 2020 10:24

replace remaining 'callWithRequest' to 'callAsCurrentUser'

d54a032

restore a line in 'jest.config.js' deleted by mistake

80be167

Add a missing notifications props to CreateMonitor

6c063a1

Correct the path of getting message in toast notifications

952f07f

ohltyler reviewed Nov 18, 2020

View reviewed changes

Stop putting 'public' folder into the build artifacts, as 'target' fo…

e4ecbd4

…lder did the job

ohltyler reviewed Nov 18, 2020

View reviewed changes

server/clusters/alerting/createAlertingCluster.js Show resolved Hide resolved

ohltyler reviewed Nov 18, 2020

View reviewed changes

Tianli Feng added 3 commits November 18, 2020 16:29

Use props to pass values for 'notifications' and 'isDarkMode' to avoi…

4cb2c97

…d inconsistency

remove the comment out code for assgning the icon for the plugin

7e72b0d

add a props of notifications in Home.js

f0b42b9

lezzago reviewed Nov 19, 2020

View reviewed changes

lezzago approved these changes Nov 20, 2020

View reviewed changes

ylwu-amzn reviewed Nov 20, 2020

View reviewed changes

ohltyler approved these changes Nov 21, 2020

View reviewed changes

ftianli-amzn merged commit 58172d9 into opendistro-for-elasticsearch:master Nov 21, 2020

ftianli-amzn deleted the new-platform-7.9.1 branch November 21, 2020 00:49

ftianli-amzn mentioned this pull request Jan 6, 2021

Change the query parameters 'size' and 'search' of 'getDestinations' request to be optional #231

Merged

		const alertingESClient = createAlertingCluster(core, globalConfig);
		const adESClient = createAlertingADCluster(core, globalConfig);

		const resp = await httpClient.get(`../api/alerting/alerts?${queryString.stringify(params)}`);
		const resp = await httpClient.get('../api/alerting/alerts', { query: params });

Migrate Alerting Kibana pluign to the new Kibana Platform #209

Migrate Alerting Kibana pluign to the new Kibana Platform #209

Conversation

ftianli-amzn commented Nov 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ftianli-amzn Nov 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ftianli-amzn Nov 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lezzago Nov 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ftianli-amzn Nov 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ftianli-amzn Nov 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ftianli-amzn commented Nov 18, 2020 •

edited

ftianli-amzn Nov 18, 2020 •

edited

ftianli-amzn Nov 18, 2020 •

edited

lezzago Nov 19, 2020 •

edited

ftianli-amzn Nov 20, 2020 •

edited

ftianli-amzn Nov 20, 2020 •

edited