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

Removes deprecated telemetry.url and telemetry.optInStatusUrl from telemetry plugin config #114737

Conversation

TinaHeiligers
Copy link
Contributor

@TinaHeiligers TinaHeiligers commented Oct 12, 2021

Telemetry urls configured via uiSettings were deprecated in favor of a telemetry.sendUsageTo config setting in #107396.

We were also still supporting old configurations from uiSettings (telemetry:optIn and xPackMonitoring:allowReport) in parallel with the new configuration using saved objects.

This PR handles removing support for those settings from 8.0, adds to the 8.0 uiSettings migration to remove telemetry:optIn and xPackMonitoring:allowReport and cleans up code related to handling legacy configurations.

Resolves #82432 and cleanup task from #96761

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
  • If a plugin configuration key changed, check if it needs to be updated in the docker list
  • Ensure QA and Cloud teams are aware and have updated deployment configurations to use the new settings/values.

For maintainers

To reviewers:

These settings are for internal use only and the breaking change isn't end-user facing, hence no release note.

@TinaHeiligers TinaHeiligers added backport:skip This commit does not require backporting Breaking Change Feature:Telemetry release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0 labels Oct 13, 2021
/**
* config options opt into telemetry
*/
export const CONFIG_TELEMETRY = 'telemetry:optIn';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer supported.

* config description for opting into telemetry
*/
export const getConfigTelemetryDesc = () => {
// Can't find where it's used but copying it over from the legacy code just in case...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the comment says, the translation doesn't seem to be used anywhere.

this.handleOldUiSettings(uiSettings);
this.startFetcherWhenOldSettingsAreHandled(core, telemetryCollectionManager);
// Not catching nor awaiting this promise because it should never reject
this.startFetcher(core, telemetryCollectionManager);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not strictly needed but we're removing handling the old settings and the old name didn't make sense anymore.

core: CoreStart,
telemetryCollectionManager: TelemetryCollectionManagerPluginStart
) {
await this.oldUiSettingsHandled$.pipe(take(1)).toPromise(); // Wait for the old settings to be handled
// We start the fetcher having updated everything we need to using the config settings
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not strictly needed but the wording wouldn't have made sense anymore.

@@ -42,7 +42,6 @@ export default async function ({ readConfigFile }) {
defaults: {
'accessibility:disableAnimations': true,
'dateFormat:tz': 'UTC',
'telemetry:optIn': false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed support for uiSetting

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

Self review again.

@TinaHeiligers TinaHeiligers marked this pull request as ready for review October 14, 2021 20:51
@TinaHeiligers TinaHeiligers requested review from a team as code owners October 14, 2021 20:51
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

LGTM. this is great! Thanks.
This reduces the plugin start time considerably!

Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

Left a small comment, but otherwise LGTM

@TinaHeiligers TinaHeiligers enabled auto-merge (squash) October 15, 2021 16:22
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
telemetry 24.1KB 23.7KB -349.0B

History

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

@TinaHeiligers TinaHeiligers merged commit 22d07ed into elastic:master Oct 15, 2021
@TinaHeiligers TinaHeiligers deleted the kbn-82432-handle-telemetry-config-changes branch October 15, 2021 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Breaking Change Feature:Telemetry release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Breaking change] Move xpack.telemetry.* config to telemetry.*
7 participants