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

Add some collector classes for objects that get registered in a CollectorSet #19098

Conversation

Projects
None yet
3 participants
@tsullivan
Copy link
Contributor

tsullivan commented May 15, 2018

Pulled out from #18894

  • Moves CollectorSet class definition file to ./classes in plugins/monitoring/server/kibana_monitoring
  • Adds 2 classes, Collector, a base class for collection objects, and UsageCollector, a collector specific to collecting usage stats
  • Changes some wiring to the collector instances created in plugins/monitoring/server/kibana_monitoring/collectors to use the new Collector class.
@@ -139,6 +137,19 @@ export class CollectorSet {
});
}

async bulkFetchUsage() {

This comment has been minimized.

Copy link
@tsullivan

tsullivan May 15, 2018

Author Contributor

Note that this isn't used yet. See #18894 for where it'll be used from an HTTP API

cc @ruflin the API will call this function from the handler, meaning that the stats will get collected whenever the API is called. No internal caching

setLogger,
fetch
};
async fetch() {

This comment has been minimized.

Copy link
@tsullivan

tsullivan May 15, 2018

Author Contributor

had to move this function into the object literal for context to work

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented May 15, 2018

@tsullivan

This comment has been minimized.

Copy link
Contributor Author

tsullivan commented May 17, 2018

cc @stacey-gammon, since you chimed in about requested that the collector objects be class instances

@pickypg
Copy link
Member

pickypg left a comment

What do you think about moving the logger?


export class Collector {
/*
* @param {String} type.type - property name as the key for the data

This comment has been minimized.

Copy link
@pickypg

pickypg May 17, 2018

Member

Nitpick: all of these have an extraneous type. prefix now.

This comment has been minimized.

Copy link
@tsullivan

tsullivan May 18, 2018

Author Contributor

we're adding a server param before this object in the args, so I'm keeping the prefix but calling it properties which matches what it's called in UsageCollector

this.init = init;
this.fetch = fetch;
this.cleanup = cleanup;

This comment has been minimized.

Copy link
@pickypg

pickypg May 17, 2018

Member

Weird blank line makes this extra field seem special.

* Allows using `server.log('debug', message)` as `this.log.debug(message)`.
* Works for info and warn logs as well.
*/
setLogger(logger) {

This comment has been minimized.

Copy link
@pickypg

pickypg May 17, 2018

Member

You could get this without making the Collector mutable by passing in the server object to the constructor, then avoid having a custom logger altogether. It would also avoid the implicit requirement that it gets defined after construction.

@tsullivan tsullivan force-pushed the tsullivan:monitoring/collector-classes-for-collectorset branch from 0ca11a2 to 0aca12d May 18, 2018

@tsullivan

This comment has been minimized.

Copy link
Contributor Author

tsullivan commented May 18, 2018

What do you think about moving the logger?

++ on it. It wasn't great before because the logger that CollectorSet got was an inline-defined function passed to the constructor.

Collector objects now have server object passed in and set their logger property with a helper function. This is better because it means that a collector can log something any time in its lifecycle, not have to wait until the CollectorSet sets a logger for it.

@pickypg
Copy link
Member

pickypg left a comment

LGTM!

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented May 18, 2018

@tsullivan tsullivan merged commit a7c3255 into elastic:master May 18, 2018

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
kibana-ci Build finished.
Details

@tsullivan tsullivan deleted the tsullivan:monitoring/collector-classes-for-collectorset branch May 18, 2018

tsullivan added a commit to tsullivan/kibana that referenced this pull request May 18, 2018

Add some collector classes for objects that get registered in a Colle…
…ctorSet (elastic#19098)

* Add some collector classes for objects that get registered in a CollectorSet

* comment cleanup

* don't pass an inline-defined logger to collectorSet

* add a helper logger function so collector has access to logger at construction

tsullivan added a commit that referenced this pull request May 18, 2018

Add some collector classes for objects that get registered in a Colle…
…ctorSet (#19098) (#19230)

* Add some collector classes for objects that get registered in a CollectorSet

* comment cleanup

* don't pass an inline-defined logger to collectorSet

* add a helper logger function so collector has access to logger at construction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.