Skip to content
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

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

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented May 8, 2018

Closes #12504

  • Registers the reporting collector in plugins/reporting/index.js instead of plugins/monitoring/server/kibana_monitoring/collectors/get_reporting_collector.js
  • KIBANA_REPORTING_TYPE moved from the Monitoring plugin to the Reporting plugin

Depends on:

@tsullivan tsullivan added review v7.0.0 v6.4.0 Team:Monitoring Stack Monitoring team (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead Feature:Telemetry labels May 8, 2018
@@ -21,7 +21,8 @@ export async function initKibanaMonitoring(kbnServer, server) {
plugins: [monitoringBulk]
});

startCollector(kbnServer, server, client);
const collector = startCollector(kbnServer, server, client);
server.expose('typeCollector', collector); // NOTE: consumers need to wait on Monitoring plugin status go green, and that this object was successfully exposed
Copy link
Member Author

@tsullivan tsullivan May 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The note is relevant to line 163 in x-pack/plugins/reporting/index.js

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tsullivan tsullivan added WIP Work in progress and removed review labels May 8, 2018
@tsullivan
Copy link
Member Author

I've removed review, added WIP label.

Reporting is not registering its collector in this code yet. It looks like the monitoring plugin needs to wait until the registration method is exposed before the plugin can go green. Since exposing the collector object that has the registration method becomes a first-class concern of Monitoring, I want to make Monitoring's green status held up by that, to resolve the race condition.

@tsullivan
Copy link
Member Author

Added depends on #18939

@tsullivan tsullivan changed the title [Monitoring/Kibana Stats] Expose typeCollector.register method for external plugins to register their stats collector [Monitoring/Reporting Stats] Move hardcoded Reporting stats type collection out of monitoring plugin May 8, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@tsullivan tsullivan force-pushed the reporting/register-usage-collector-with-monitoring branch 2 times, most recently from c8b0454 to 21f95d5 Compare May 9, 2018 20:10
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tsullivan tsullivan force-pushed the reporting/register-usage-collector-with-monitoring branch 2 times, most recently from 6f4906a to bba238c Compare May 10, 2018 23:02
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@@ -146,7 +147,8 @@ export const reporting = (kibana) => {
validateConfig(config, message => server.log(['reporting', 'warning'], message));
logConfiguration(config, message => server.log(['reporting', 'debug'], message));

const xpackMainPlugin = server.plugins.xpack_main;
const { xpack_main: xpackMainPlugin, monitoring: monitoringPlugin } = server.plugins;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs testing on what happens when Monitoring is disabled but Reporting is enabled. cc @pickypg

@elasticmachine
Copy link
Contributor

💔 Build Failed

@tsullivan tsullivan force-pushed the reporting/register-usage-collector-with-monitoring branch from 11e0f44 to 6e6e6ae Compare May 22, 2018 00:07
@@ -1,44 +0,0 @@
/*
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test wasn't really doing very much, and hasn't been replaced with a similar mocha test in the Reporting plugin.

@@ -1,18 +0,0 @@
/*
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been moved to Reporting and replaces reporting/server/usage/get_usage.js

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

} from '../../../common/constants';
import { KIBANA_REPORTING_TYPE } from '../../../../reporting/common/constants';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a step two planned to get rid of this and the abstraction leak below? Or maybe you can get rid of it now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module that combines collection types together into a bulk payload unfortunately does have to stay, but just for the short term. It's to support the behavior of Kibana Monitoring that sends kibana stats to the monitoring/_bulk endpoint in ES. Unfortunately, this module needs to have a lot of awareness of the usage collectors because in the current schema, usage is a field of kibana_stats.

image

We will take the responsibility of sending stats to ES out of Kibana and have Metricbeats used for monitoring all the systems. Once we do that, we can remove get_collector_types_combiner

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tsullivan tsullivan added review and removed WIP Work in progress labels May 22, 2018
@tsullivan
Copy link
Member Author

I merged in master to get #18986 into this branch. CI is running right now and I will also run some manual tests before merging.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tsullivan tsullivan merged commit ce61ae4 into elastic:master May 24, 2018
@tsullivan tsullivan deleted the reporting/register-usage-collector-with-monitoring branch May 24, 2018 18:14
tsullivan added a commit to tsullivan/kibana that referenced this pull request May 24, 2018
…ection out of monitoring plugin (elastic#18894)

* [Monitoring/Telemetry collection] use `typeCollector.registerType` in Reporting

* a few cleanup changes
tsullivan added a commit that referenced this pull request May 25, 2018
…ection out of monitoring plugin (#18894) (#19409)

* [Monitoring/Telemetry collection] use `typeCollector.registerType` in Reporting

* a few cleanup changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead Feature:Telemetry review Team:Monitoring Stack Monitoring team v6.4.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants