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

[Telemetry] Move to OSS #45769

Merged
merged 31 commits into from
Oct 16, 2019
Merged

[Telemetry] Move to OSS #45769

merged 31 commits into from
Oct 16, 2019

Conversation

Bamieh
Copy link
Member

@Bamieh Bamieh commented Sep 16, 2019

Moving telemetry from xpack to oss.

Closes https://github.com/elastic/telemetry/issues/131
Closes https://github.com/elastic/dev/issues/1247

Overview:

So i've added in OSS a telemetryCollectionManager:
https://github.com/elastic/kibana/blob/c01fd26af4b0379234558d185667d6b5bb4933b8/src/legacy/core_plugins/telemetry/server/collection_manager.ts

this is a class that keeps track of which stats getter to use, it does not know about monitoring or any other plugin.

in the init function of telemetry OSS it gets called with this:

telemetryCollectionManager.setStatsGetter(getStatsOSS);

This registeres the oss telemetry stats getter which fetches "local" collectors just like it should.

OSS stats getter: https://github.com/elastic/kibana/blob/c01fd26af4b0379234558d185667d6b5bb4933b8/src/legacy/core_plugins/telemetry/server/collectors/get_stats_oss.ts

In the monitoring plugin init function we set a new stats getter:

telemetryCollectionManager.setStatsGetter(getStatsWithMonitoring, 1);

This registers the getStatsWithMonitoring under a higher priority (1 > 0) to be used instead of the one set in the telemetry init.

This is the old stats getter which checks if monitoring is enabled to fetch monitoring stats, and defaults to the local collectors if that is false or fails. (no code changes here)

Monitoring stats getter: https://github.com/elastic/kibana/blob/c01fd26af4b0379234558d185667d6b5bb4933b8/x-pack/legacy/plugins/monitoring/server/telemetry/get_stats_with_monitoring.ts

At the API endpoint; instead of calling the getStats function directly to fetch the stats, we call this:

const getStats = telemetryCollectionManager.getStatsGetter();
const usageData = await getStats(req, config, start, end, unencrypted);

so technically any plugin can now override how we fetch the stats, and specify a priority to know which one should be used when telemetry stats endpoint is being called.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@bmcconaghy bmcconaghy left a comment

Choose a reason for hiding this comment

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

This is coming along nicely. There are some imports that probably need to change, and one spot in OSS that references xpack things, but otherwise good. I would suggest having someone from platform look at this and make sure they are OK with how the xpack overriding OSS function works.

@rudolf
Copy link
Contributor

rudolf commented Sep 30, 2019

I would suggest having someone from platform look at this and make sure they are OK with how the xpack overriding OSS function works.

This is similar to how security & spaces override the saved objects client with savedObjects.setScopedSavedObjectsClientFactory or extend it's functionality with a composition based inheritance that's called "wrapping" using savedObjects.addScopedSavedObjectsClientWrapperFactory.

Telemetry's requirements and implementation is a bit simpler which is a good thing, but it follows the same pattern of X-Pack overriding OSS so I would say 👍

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Bamieh Bamieh requested a review from jbudz October 9, 2019 10:26
@Bamieh
Copy link
Member Author

Bamieh commented Oct 14, 2019

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@epixa
Copy link
Contributor

epixa commented Nov 20, 2019

Since we've followed up with a change to deprecate the xpack.telemetry. prefixed configuration paths rather than remove them entirely in 7.x, I've replaced the breaking label with enhancement. @gchaps is there anything else we need to do to make sure this gets into the proper place in release notes?

@gchaps
Copy link
Contributor

gchaps commented Nov 20, 2019

@epixa We'll make sure this PR is in the Enhancements section in the Release Notes

@KOTungseth
Copy link
Contributor

KOTungseth commented Nov 20, 2019

@epixa @gchaps looks like the Release Notes script listed this PR as an enhancement, so it already appears in the Enhancements section.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants