[Monitoring] Add a yellow status phase in plugin init #18939

tsullivan · 2018-05-08T21:56:53Z

Pulled out from #18894

This PR adds a yellow status phase to the Monitoring app plugin startup, and pulls a few other changes out of #18894 to show the intent of this preparatory change.

The collector object, which has a registerType method to incorporate collectors, is returned from start_collector.js and put in scope in the Monitoring init method.
- registerType was just register before, but renamed because it was confusing that a collector object registers "collectors".
- Should be more clear now that there is 1 collector and it gets "types" registered into it.
- registerType is called with a type object that defines how the collector can fetch data for it
Monitoring status starts as yellow and goes to green after the collector object is exposed as a server method.
Some comments have been added to the areas in Monitoring where we're hardcoding for Reporting (seen as an external plugin) that will get cleaned up in 18894

elasticmachine · 2018-05-08T23:27:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-05-09T21:07:15Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

tsullivan · 2018-05-10T05:00:30Z

jenkins test this

elasticmachine · 2018-05-10T05:59:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tsullivan · 2018-05-10T16:20:34Z

x-pack/plugins/monitoring/init.js

 * @param monitoringPlugin {Object} Monitoring UI plugin
 * @param server {Object} HapiJS server instance
 */
 export const init = (monitoringPlugin, server) => {
+  monitoringPlugin.status.yellow('Initializing');


Starting with a yellow status and changing to green after typeCollector is exposed on the server allows plugins to wait for Monitoring to be ready before trying to consume the typeCollector and add their own collection methods.

tsullivan · 2018-05-10T16:22:01Z

@pickypg @stacey-gammon @kobelb would you mind taking a look at this? It's a prep change needed to begin working on #12504

elasticmachine · 2018-05-10T19:07:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tsullivan · 2018-05-10T20:46:57Z

pulled out the register => registerType function name changes. Issues with semantics will be addressed in a later PR (#18987, which depends on this one)

elasticmachine · 2018-05-10T21:52:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

pickypg

My comments are more thoughts than issues.

pickypg · 2018-05-14T16:37:53Z

x-pack/plugins/monitoring/server/kibana_monitoring/init_kibana_monitoring.js

 */
-export async function initKibanaMonitoring(kbnServer, server) {
+export function initKibanaMonitoring(kbnServer, server) {


I think we should change this function's name now that it returns something. Perhaps createKibanaCollector?

I think could also do @return {TypeCollector} ? If I'm following the code correctly, that is what is returned.

Sure, but I'd like to handle this in the next PR, which depends on this one: #18987

It's a small one with strictly renaming changes

pickypg · 2018-05-14T16:46:37Z

x-pack/plugins/monitoring/init.js

    }

-    // Send Kibana usage / server ops to the monitoring bulk api
    if (config.get('xpack.monitoring.kibana.collection.enabled')) {


As a side note, outside code that registers its own collectors will need to effectively do this too:

if (config.get('xpack.monitoring.enabled') && config.get('xpack.monitoring.kibana.collection.enabled')) { // or if (server.plugins.monitoring && server.plugins.monitoring.typeCollector) {

for any usage of this server object.

I've started this work in the 2nd PR to come after this one: #18894 It might be awhile before that gets filed, but I'll be sure to test what happens when Monitoring is disabled but Reporting is enabled.

pickypg · 2018-05-14T17:01:47Z

x-pack/plugins/monitoring/server/kibana_monitoring/lib/type_collector.js

-   * - init {Function} (optional)
-   * - fetch {Function}
-   * - cleanup {Function} (optional)
+   * @param {String} type.type


I wonder if we should make this an object parameter that makes these fields clearer? That or make type a class that users are expected to implement to take advantage of?

register({ type, fetch, init, cleanup }) {

I would personally vote for the class because it makes it easier to require specific methods with the less opportunity for misunderstanding their purpose.

I like the class implementation approach too, it's what I've been following for my pluggable things, like embeddables and pluggable panel actions. Eventually when we move to typescript we'll be able to use their interfaces. But this is fine too, if you want to leave as is.

I had the same thought :D , but planned to work that out in a later PR: 60ba195

It's in a PR now: #19098

pickypg · 2018-05-14T17:02:31Z

x-pack/plugins/monitoring/server/kibana_monitoring/lib/type_collector.js

   */
-  register(collectorOptions) {
-    this._collectors.push(collectorOptions);
+  register(type) {


One downside to this approach is that a plugin that registers late could technically miss [at least] the first collection cycle.

I don't have a better approach in mind, but it's something to keep in mind as we add more.

stacey-gammon

LGTM with minor comments.

stacey-gammon · 2018-05-15T13:45:09Z

x-pack/plugins/monitoring/server/kibana_monitoring/init_kibana_monitoring.js

 */
-export async function initKibanaMonitoring(kbnServer, server) {
+export function initKibanaMonitoring(kbnServer, server) {


I think could also do @return {TypeCollector} ? If I'm following the code correctly, that is what is returned.

stacey-gammon · 2018-05-15T13:45:55Z

x-pack/plugins/monitoring/server/kibana_monitoring/start_collector.js

@@ -53,4 +53,6 @@ export function startCollector(kbnServer, server, client, _sendBulkPayload = sen
  server.plugins.elasticsearch.status.on('red', () => {
    collector.cleanup();
  });
+
+  return collector;


add @return {TypeCollector} to jsdoc?

TypeCollector will get renamed to CollectorSet in my next PR, so I'll handle it there.

TypeCollector seemed like a bad name because it is a thing that keeps a set of collectors, not a collector itself.

stacey-gammon · 2018-05-15T13:46:23Z

x-pack/plugins/monitoring/server/kibana_monitoring/lib/type_collector.js

-   * - init {Function} (optional)
-   * - fetch {Function}
-   * - cleanup {Function} (optional)
+   * @param {String} type.type


I like the class implementation approach too, it's what I've been following for my pluggable things, like embeddables and pluggable panel actions. Eventually when we move to typescript we'll be able to use their interfaces. But this is fine too, if you want to leave as is.

* [Monitoring] Add a yellow status phase in the startup lifecycle * comments * more comments * more comment * undo register => registerType function name change

[Monitoring] Add a yellow status phase in the startup lifecycle

8108891

tsullivan added review v7.0.0 v6.4.0 Team:Monitoring Stack Monitoring team refactoring labels May 8, 2018

tsullivan requested a review from Gunnerva May 8, 2018 21:56

tsullivan mentioned this pull request May 8, 2018

[Monitoring/Reporting Stats] Move hardcoded Reporting stats type collection out of monitoring plugin #18894

Merged

4 tasks

Merge branch 'master' into monitoring/add-yellow-status

6d41238

tsullivan removed the request for review from Gunnerva May 9, 2018 19:31

tsullivan force-pushed the monitoring/add-yellow-status branch from f50d596 to f1982e6 Compare May 9, 2018 19:43

tsullivan requested a review from pickypg May 9, 2018 20:00

comments

53ddfa0

tsullivan force-pushed the monitoring/add-yellow-status branch from f1982e6 to 53ddfa0 Compare May 9, 2018 20:05

more comments

cd69309

tsullivan requested review from kobelb and stacey-gammon May 10, 2018 16:19

tsullivan commented May 10, 2018

View reviewed changes

tsullivan added 2 commits May 10, 2018 11:06

more comment

e266943

Merge branch 'master' into monitoring/add-yellow-status

360e8a3

undo register => registerType function name change

d72c7f8

tsullivan mentioned this pull request May 10, 2018

[Monitoring] Rename TypeCollector to CollectorSet for semantics #18987

Merged

tsullivan mentioned this pull request May 10, 2018

Allow plugins to register their own stats for an _xpack/usage API #12504

Closed

pickypg reviewed May 14, 2018

View reviewed changes

stacey-gammon approved these changes May 15, 2018

View reviewed changes

tsullivan merged commit 9e0a6e2 into elastic:master May 15, 2018

tsullivan deleted the monitoring/add-yellow-status branch May 15, 2018 16:32

tsullivan mentioned this pull request May 15, 2018

[6.x] [Monitoring] Add a yellow status phase in plugin init (#18939) #19075

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Monitoring] Add a yellow status phase in plugin init #18939

[Monitoring] Add a yellow status phase in plugin init #18939

tsullivan commented May 8, 2018

elasticmachine commented May 8, 2018

elasticmachine commented May 9, 2018

tsullivan commented May 10, 2018

elasticmachine commented May 10, 2018

tsullivan May 10, 2018

tsullivan commented May 10, 2018

elasticmachine commented May 10, 2018

tsullivan commented May 10, 2018 •

edited

Loading

elasticmachine commented May 10, 2018

pickypg left a comment

pickypg May 14, 2018

stacey-gammon May 15, 2018

tsullivan May 15, 2018

tsullivan May 15, 2018

pickypg May 14, 2018

tsullivan May 15, 2018

pickypg May 14, 2018

stacey-gammon May 15, 2018

tsullivan May 15, 2018

tsullivan May 15, 2018

pickypg May 14, 2018

stacey-gammon left a comment

stacey-gammon May 15, 2018

stacey-gammon May 15, 2018

tsullivan May 15, 2018

tsullivan May 15, 2018

stacey-gammon May 15, 2018

[Monitoring] Add a yellow status phase in plugin init #18939

[Monitoring] Add a yellow status phase in plugin init #18939

Conversation

tsullivan commented May 8, 2018

elasticmachine commented May 8, 2018

💚 Build Succeeded

elasticmachine commented May 9, 2018

💔 Build Failed

tsullivan commented May 10, 2018

elasticmachine commented May 10, 2018

💚 Build Succeeded

Choose a reason for hiding this comment

tsullivan commented May 10, 2018

elasticmachine commented May 10, 2018

💚 Build Succeeded

tsullivan commented May 10, 2018 • edited Loading

elasticmachine commented May 10, 2018

💚 Build Succeeded

pickypg left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon left a 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

tsullivan commented May 10, 2018 •

edited

Loading