[Telemetry] move x-pack/monitoring collector classes to src/server/usage by tsullivan · Pull Request #20248 · elastic/kibana

tsullivan · 2018-06-26T23:34:46Z

Part of Meta issue #20393
Closes #19534
Following along on #19691

Most of these code changes have already been reviewed. This PR moves the CollectorSet classes to src/server/usage and registers the service differently. The service is longer dependent on the Monitoring plugin, so consumers should have it easier since they don't have to watch any plugin status before registering their collectors.

Comments about needing to use the saved object client are partially dependent on #19662

pickypg

The current test failure is unrelated (PhantomJS / Reporting error), but I do have one question that might need someone else's feedback on the platform team. Otherwise LGTM.

pickypg · 2018-07-02T15:27:05Z

x-pack/plugins/monitoring/server/kibana_monitoring/collectors/get_settings_collector.js


 export function getSettingsCollector(server) {
  const config = server.config();
+  const { Collector } = server.usage;


This is creative. I wonder if this approach will work with the future plugin system / TypeScript?

Earlier ago, I talked with @epixa about these changes and he let me know that if we're registering server methods as per the conventions from the Hapi ecosystem, we should be OK for at least the foreseeable future.

I mentioned Court on this so he can correct me if I misunderstood.

Yep, this should be fine.

pickypg · 2018-07-02T15:28:40Z

x-pack/plugins/reporting/index.js

-        }
-      });
+      // Register a function with server to manage the collection of usage stats
+      const { collectorSet } = server.usage;


This is a nitpick, but why not just do

// Register a function with server to manage the collection of usage stats server.usage.collectorSet.register(getReportingUsageCollector(server));

Bringing the variable into scope seems unnecessary.

tsullivan · 2018-07-02T23:07:59Z

I'm taking the review label off this for now. After a chat with @ycombinator, I want to change some assumptions I was going with on this. This will get the changes documented in #20393

tsullivan · 2018-07-05T22:49:43Z

Once this is green, it will be ready again for review.

I made some experimental changes that have been rolled back, which I did through rebase. The changes to do next are part of a Meta issue: #20393

elasticmachine · 2018-07-05T23:54:56Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

ycombinator · 2018-07-06T10:19:23Z

src/server/usage/classes/usage_collector.js

+export class UsageCollector extends Collector {
+  constructor(server, properties) {
+    super(server, properties);
+  }


Do you even need the constructor here?

ycombinator · 2018-07-06T10:25:25Z

src/server/usage/index.js

+    collectorSet,   // consumer code calls collectorSet.register(collector) to define their own collector objects
+    Collector,      // helper class for consumer code implementing ops/stats collection
+    UsageCollector, // helper class for consumer codea implementing usage collection
+  });


You could potentially avoid exposing the helper classes themselves by providing factory methods on CollectorSet like makeCollector(server, properties) and makeUsageCollector(server, properties). Internally these methods would new up and return Collector and UsageCollector instances, respectively.

Another argument for this pattern is that there is no use case for creating Collector or UsageCollector instances without having to use them with a collectorSet anyway. By requiring their creation via a collectorSet instance, you are at least hinting at this coupling a bit more strongly.

I love it, stay tuned

elasticmachine · 2018-07-06T18:07:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ycombinator

LGTM pending green CI

elasticmachine · 2018-07-06T23:12:01Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

pickypg

LGTM

…age (elastic#20248) * [Monitoring/Telemetry] Move Usage service from Monitoring to Kibana core * fix tests * fix reporting integration * roll back more diffs * roll logger into bulk uploader to remove file duplication * fix xpack usage api * subclass constructor is not needed * collectorSet has factory methods for collector object creation * fix reporting usage jest test

tsullivan · 2018-07-18T23:49:59Z

6.x/6.4.0: #20955

…age (#20248) (#20955) * [Monitoring/Telemetry] Move Usage service from Monitoring to Kibana core * fix tests * fix reporting integration * roll back more diffs * roll logger into bulk uploader to remove file duplication * fix xpack usage api * subclass constructor is not needed * collectorSet has factory methods for collector object creation * fix reporting usage jest test

tsullivan added v7.0.0 v6.4.0 Feature:Telemetry labels Jun 26, 2018

tsullivan self-assigned this Jun 26, 2018

tsullivan force-pushed the usage/collector-set-to-core branch 3 times, most recently from 276fafa to d095edc Compare June 27, 2018 19:39

elastic deleted a comment from elasticmachine Jun 27, 2018

tsullivan force-pushed the usage/collector-set-to-core branch 2 times, most recently from 3265c73 to 510f63d Compare June 27, 2018 19:45

tsullivan changed the title ~~WIP - [Telemetry] move x-pack/monitoring collector classes to src/server/usage~~ [Telemetry] move x-pack/monitoring collector classes to src/server/usage Jun 27, 2018

tsullivan requested review from chrisronline, epixa, nreese, pickypg, spalger and stacey-gammon June 27, 2018 19:56

tsullivan added review Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// enhancement New value added to drive a business result labels Jun 27, 2018

tsullivan mentioned this pull request Jun 29, 2018

WIP [Telemetry] Pass a saved objects client to the collector fetch methods #20352

Closed

pickypg reviewed Jul 2, 2018

View reviewed changes

tsullivan removed request for chrisronline, epixa, nreese, spalger and stacey-gammon July 2, 2018 22:34

tsullivan removed the review label Jul 2, 2018

[Monitoring/Telemetry] Move Usage service from Monitoring to Kibana core

d3ae740

tsullivan added the review label Jul 5, 2018

tsullivan requested a review from ycombinator July 5, 2018 22:34

tsullivan added 2 commits July 5, 2018 15:35

roll back more diffs

b7d6662

roll logger into bulk uploader to remove file duplication

c811e57

tsullivan requested a review from chrisronline July 5, 2018 22:46

elastic deleted a comment from elasticmachine Jul 5, 2018

ycombinator reviewed Jul 6, 2018

View reviewed changes

fix xpack usage api

afd3337

tsullivan added 3 commits July 6, 2018 12:28

Merge branch 'master' into usage/collector-set-to-core

403c29d

subclass constructor is not needed

a37d659

collectorSet has factory methods for collector object creation

01b1c5f

ycombinator approved these changes Jul 6, 2018

View reviewed changes

fix reporting usage jest test

3cb5121

elastic deleted a comment from elasticmachine Jul 6, 2018

pickypg approved these changes Jul 9, 2018

View reviewed changes

tsullivan merged commit c043788 into elastic:master Jul 9, 2018

tsullivan deleted the usage/collector-set-to-core branch July 9, 2018 16:32

legrego mentioned this pull request Jul 9, 2018

Add telemetry for spaces #19260

Closed

tsullivan mentioned this pull request Jul 18, 2018

[6.x] [Telemetry] move x-pack/monitoring collector classes to src/server/usage (#20248) #20955

Merged

Conversation

tsullivan commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pickypg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsullivan commented Jul 2, 2018

Uh oh!

tsullivan commented Jul 5, 2018

Uh oh!

elasticmachine commented Jul 5, 2018

💔 Build Failed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycombinator Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 6, 2018

💚 Build Succeeded

Uh oh!

ycombinator left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 6, 2018

💚 Build Succeeded

Uh oh!

pickypg left a comment

Choose a reason for hiding this comment

Uh oh!

tsullivan commented Jul 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tsullivan commented Jun 26, 2018 •

edited

Loading

ycombinator Jul 6, 2018 •

edited

Loading