[Telemetry] Add possibility of registering exclusive collectors for each collection #62665

afharo · 2020-04-06T17:52:52Z

Summary

In the PR #56675, the kibana_stats provided by the OSS and X-Pack collections were renamed to oss_kibana_stats to avoid them clashing with the kibana_stats stats reported by the monitoring plugin.
This was done because, after the NP migration, the order in which the collectors are registered is not ensured anymore, leading to the Monitoring plugin obtaining the kibana_stats from OSS and vice-versa.

But that change led to a few issues:

The OSS code is expecting kibana_stats to exist and it is crashing when trying to collect the usage when reporting the telemetry.
If we fix the OSS code to read from oss_kibana_stats, when running X-Pack, but monitoring is disabled, the kibana_stats reported by the monitoring plugin make it through the telemetry payload. And, on the other side: when monitoring is enabled, oss_kibana_stats is added to the telemetry payload.

This PR attempts to fix that issue by merging the collectors in the OSS side. According to the current implementation of each collector in the Monitoring end, they don't look to me like Monitoring or even X-Pack specific (except the "kibana_settings" collector: that one depends on the Monitoring config, so it makes sense to live there).

Fixes #62587

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

…lections

pgayvallet

I'm not a collector expert, but what is the main reason to be able to register multiple providers for the same type? From an external point of view, this looks like a strange pattern and this PR a workaround to address the associated issues? (but once again, I don't have any context so please don't feel offended by the honest question 😄 )

afharo · 2020-04-07T16:28:18Z

I'm not a collector expert, but what is the main reason to be able to register multiple providers for the same type? From an external point of view, this looks like a strange pattern and this PR a workaround to address the associated issues? (but once again, I don't have any context so please don't feel offended by the honest question 😄 )

It is a very valid comment! The main reason why we may have 2 collectors with the same type is that there may be different ways of obtaining the stats depending on the enabled plugin.

The monitoring plugin is the perfect example for it (and, actually, the only one using this feature). As far as I understand it: for OSS and X-Pack, the collectors can assume Kibana is connected to one cluster. For Monitoring, 1 Kibana may be receiving monitoring information from multiple clusters, hence the need to overwrite some collectors.

Anyway, after taking a further look at the actual implementation of the collectors defined in the monitoring plugin. I believe they should belong in the OSS implementation rather than monitoring.

I've migrated the collectors to the OSS end and reverted the changes including the new flag onlyForCollectionName.

That will much simplify the future fix for the issue #58249.

afharo · 2020-04-07T17:27:55Z

Pinging @elastic/stack-monitoring-ui because it looks like the CODEOWNERS file is not properly updated after their NP migration and they are not mentioned

afharo · 2020-04-07T18:36:21Z

@elasticmachine merge upstream

TinaHeiligers

A ran the code locally and confirmed that this PR resolves the original issue.
Now that telemetry is enabled by default, the changes make sense.
LGTM.

chrisronline · 2020-04-07T19:02:28Z

@afharo Thanks for the ping and doing this work!

Apologies for this:

The OSS code is expecting kibana_stats to exist and it is crashing when trying to collect the usage when reporting the telemetry.

I'm going to review this today or tomorrow

chrisronline

LGTM! Nice work here!

kibanamachine · 2020-04-07T20:15:23Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 0bce132

History

💔 Build #39358 failed 257732f
💚 Build #39308 succeeded 0c63efc
💚 Build #39298 succeeded 29476aa
💔 Build #38991 failed 3b49f82

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…ach collection (elastic#62665) * [Telemetry] Add posibility of regitering exclusive collectors for collections * [Telemetry] Filter unwanted fields from the kibana.os telemetry payload * Filter the collectors properly in bulkFetch * Move "kibana" usage collector from Monitoring to OSS Telemetry * Remove exclusivity of the "kibana_settings" collector * Unify "kibana_stats" collector from Monitoring and Legacy * Remove unused legacy constants * Proper type for UsageCollectionSetup in monitoring * Missed one undo * Add unit tests to the migrated collectors Co-authored-by: Elastic Machine <[email protected]>

…ach collection (#62665) (#62912) * [Telemetry] Add posibility of regitering exclusive collectors for collections * [Telemetry] Filter unwanted fields from the kibana.os telemetry payload * Filter the collectors properly in bulkFetch * Move "kibana" usage collector from Monitoring to OSS Telemetry * Remove exclusivity of the "kibana_settings" collector * Unify "kibana_stats" collector from Monitoring and Legacy * Remove unused legacy constants * Proper type for UsageCollectionSetup in monitoring * Missed one undo * Add unit tests to the migrated collectors Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

…ach collection (#62665) (#62913) * [Telemetry] Add posibility of regitering exclusive collectors for collections * [Telemetry] Filter unwanted fields from the kibana.os telemetry payload * Filter the collectors properly in bulkFetch * Move "kibana" usage collector from Monitoring to OSS Telemetry * Remove exclusivity of the "kibana_settings" collector * Unify "kibana_stats" collector from Monitoring and Legacy * Remove unused legacy constants * Proper type for UsageCollectionSetup in monitoring * Missed one undo * Add unit tests to the migrated collectors Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

…ach collection (#62665) * [Telemetry] Add posibility of regitering exclusive collectors for collections * [Telemetry] Filter unwanted fields from the kibana.os telemetry payload * Filter the collectors properly in bulkFetch * Move "kibana" usage collector from Monitoring to OSS Telemetry * Remove exclusivity of the "kibana_settings" collector * Unify "kibana_stats" collector from Monitoring and Legacy * Remove unused legacy constants * Proper type for UsageCollectionSetup in monitoring * Missed one undo * Add unit tests to the migrated collectors Co-authored-by: Elastic Machine <[email protected]>

afharo added 2 commits April 6, 2020 18:27

[Telemetry] Add posibility of regitering exclusive collectors for col…

fd36ace

…lections

[Telemetry] Filter unwanted fields from the kibana.os telemetry payload

cc04a95

afharo added bug Fixes for quality problems that affect the customer experience Feature:Telemetry v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 :Telemetry v7.8.0 labels Apr 6, 2020

Filter the collectors properly in bulkFetch

3b49f82

afharo marked this pull request as ready for review April 6, 2020 18:06

afharo requested review from a team as code owners April 6, 2020 18:06

pgayvallet reviewed Apr 7, 2020

View reviewed changes

afharo added 3 commits April 7, 2020 16:07

Move "kibana" usage collector from Monitoring to OSS Telemetry

29476aa

Remove exclusivity of the "kibana_settings" collector

0c63efc

Unify "kibana_stats" collector from Monitoring and Legacy

f2e33d6

afharo added 4 commits April 7, 2020 17:32

Remove unused legacy constants

595e200

Proper type for UsageCollectionSetup in monitoring

49b2e1a

Missed one undo

c841909

Add unit tests to the migrated collectors

257732f

afharo requested a review from a team April 7, 2020 17:27

afharo added the review label Apr 7, 2020

afharo requested a review from pgayvallet April 7, 2020 17:28

Merge branch 'master' into telemetry/no-usage-when-oss

0bce132

TinaHeiligers approved these changes Apr 7, 2020

View reviewed changes

chrisronline approved these changes Apr 7, 2020

View reviewed changes

pgayvallet approved these changes Apr 8, 2020

View reviewed changes

afharo merged commit 028313a into elastic:master Apr 8, 2020

afharo deleted the telemetry/no-usage-when-oss branch April 8, 2020 08:09

This was referenced Apr 8, 2020

[7.x] [Telemetry] Add possibility of registering exclusive collectors for each collection (#62665) #62912

Merged

[7.7] [Telemetry] Add possibility of registering exclusive collectors for each collection (#62665) #62913

Merged

jfsiii mentioned this pull request Apr 8, 2020

[Telemetry] Calls to send are failing #63061

Closed

chrisronline mentioned this pull request Aug 28, 2020

[Monitoring] Move stats collector back into the monitoring plugin #76230

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Telemetry] Add possibility of registering exclusive collectors for each collection #62665

[Telemetry] Add possibility of registering exclusive collectors for each collection #62665

afharo commented Apr 6, 2020 •

edited

Loading

pgayvallet left a comment

afharo commented Apr 7, 2020

afharo commented Apr 7, 2020

afharo commented Apr 7, 2020

TinaHeiligers left a comment

chrisronline commented Apr 7, 2020

chrisronline left a comment

kibanamachine commented Apr 7, 2020

[Telemetry] Add possibility of registering exclusive collectors for each collection #62665

[Telemetry] Add possibility of registering exclusive collectors for each collection #62665

Conversation

afharo commented Apr 6, 2020 • edited Loading

Summary

Checklist

For maintainers

pgayvallet left a comment

Choose a reason for hiding this comment

afharo commented Apr 7, 2020

afharo commented Apr 7, 2020

afharo commented Apr 7, 2020

TinaHeiligers left a comment

Choose a reason for hiding this comment

chrisronline commented Apr 7, 2020

chrisronline left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 7, 2020

💚 Build Succeeded

History

afharo commented Apr 6, 2020 •

edited

Loading