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

[Task names in TaskManager] Rename "telemetry" to "usage" #78129

Conversation

afharo
Copy link
Member

@afharo afharo commented Sep 22, 2020

Summary

Rename telemetry to usage to avoid confusion in support and users.

We've had a few open requests asking why they could see entries in the logs about tasks named after "telemetry" when they explicitly set telemetry.enabled: false or telemetry.optIn: false.

To avoid annoying users, let's use an alternative name when naming the tasks used for usage collection.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@afharo afharo added Team:APM All issues that need APM UI Team support Feature:Telemetry Feature:Alerting v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes Feature:Actions Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.10.0 labels Sep 22, 2020
@afharo
Copy link
Member Author

afharo commented Sep 22, 2020

@elastic/kibana-alerting-services is it safe to simply rename the tasks like in this PR or do we need to apply any sort of migration to avoid the ones with the previous names to keep on running?

@afharo afharo force-pushed the telemetry/rename-in-other-plugins-to-usage/task-names-in-taskManager branch from 20d8740 to 470f3ba Compare September 22, 2020 09:31
@afharo afharo added Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Sep 22, 2020
@afharo
Copy link
Member Author

afharo commented Sep 22, 2020

@elasticmachine merge upstream

@afharo afharo marked this pull request as ready for review September 22, 2020 13:41
@afharo afharo requested review from a team as code owners September 22, 2020 13:41
@afharo afharo requested a review from a team September 22, 2020 13:41
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-telemetry (Team:KibanaTelemetry)

@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@mikecote
Copy link
Contributor

@elastic/kibana-alerting-services is it safe to simply rename the tasks like in this PR or do we need to apply any sort of migration to avoid the ones with the previous names to keep on running?

A couple of things:

  • Renaming the task definition: This will cause existing tasks that reference the old name to fail at their next execution
  • Renaming the task id: This will cause a secondary task to be created and leaving the old task failing

There isn't a good workaround that I can think of at this time other than creating back the old task definition to delete the old task. I think it might be easier to leave the ids and task definition names as they are.

@afharo
Copy link
Member Author

afharo commented Sep 23, 2020

Oh! @mikecote, thank you for confirming.

That's unfortunate. Maybe adding await taskManager.remove(oldId) is not that difficult. However, for how long are we supposed to keep that delete clause there (customers don't necessarily upgrade from minor-1 to minor (not even from major-1 to major)?

I'll close this PR for now.

Should we open an issue to improve the Task Manager to clean up tasks that have been registered on a previous version and not registered again in the new one after startup? Does it make sense?

@mikecote
Copy link
Contributor

Maybe adding await taskManager.remove(oldId) is not that difficult. However, for how long are we supposed to keep that delete clause there (customers don't necessarily upgrade from minor-1 to minor (not even from major-1 to major)?

I think that could work, I think users are suppose to upgrade to the latest minor before upgrading to a major. This would mean in 8.0 you could cleanup the code.

Should we open an issue to improve the Task Manager to clean up tasks that have been registered on a previous version and not registered again in the new one after startup? Does it make sense?

This could be interesting, an issue can at least be opened to explore such option. If we feel it's best to go an alternate path, we can then close it. With the tight bandwidth in 7.x, there's no promise this would be done in the 7.x timeframe.

@TinaHeiligers
Copy link
Contributor

@afharo since we can't easily rename the tasks, we need to find some other way to share knowledge to users about what telemetry tasks are actually for. @mikecote is there a way for us to add a description to a task when registering it?

@mikecote
Copy link
Contributor

mikecote commented Sep 23, 2020

@TinaHeiligers you can use the title attribute in the task definition to set a description.

https://github.com/elastic/kibana/blob/master/x-pack/plugins/task_manager/server/README.md

Example:

taskManager.registerTaskDefinitions({
      // clusterMonitoring is the task type, and must be unique across the entire system
      clusterMonitoring: {
        // Human friendly name, used to represent this task in logs, UI, etc
        title: 'Human friendly name',
        ...
      },
});

@TinaHeiligers
Copy link
Contributor

you can use the title attribute in the task definition to set a description.

@afharo for now, we can keep the changes to the title.

@afharo
Copy link
Member Author

afharo commented Sep 23, 2020

@TinaHeiligers the only problem is that we've seen errors like ES claiming something in the line of Failed to GET .kibana_task_manager/_doc/task:Actions-actions_telemetry or Failed cancelling task ID Actions-actions_telemetry, so the title is not helping much here.

I'll update them anyway in case we ever provide a UI management for the tasks 🙂

@afharo
Copy link
Member Author

afharo commented Sep 24, 2020

As agreed, updated titles only and leaving task IDs and types unchanged for now.

@afharo afharo added the review label Sep 24, 2020
@afharo
Copy link
Member Author

afharo commented Sep 28, 2020

@elasticmachine merge upstream

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

Alerting changes LGTM

@afharo
Copy link
Member Author

afharo commented Sep 30, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

Copy link
Contributor

@smith smith left a comment

Choose a reason for hiding this comment

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

APM changes look good.

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

KibanaApp Code LGTM, since just a title changed, didn't test, trust in Jenkins

@afharo afharo merged commit 65cf639 into elastic:master Oct 1, 2020
afharo added a commit to afharo/kibana that referenced this pull request Oct 1, 2020
)

* [Task names in TaskManager] Rename "telemetry" to "usage"

* Revert task IDs but leaving renamed titles

Co-authored-by: Elastic Machine <[email protected]>
@afharo afharo deleted the telemetry/rename-in-other-plugins-to-usage/task-names-in-taskManager branch October 1, 2020 09:05
afharo added a commit that referenced this pull request Oct 1, 2020
…) (#79069)

* [Task names in TaskManager] Rename "telemetry" to "usage"

* Revert task IDs but leaving renamed titles

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 1, 2020
* master: (36 commits)
  [I18n] fix eui tokens (elastic#78951)
  Changed the color of the confirm button in trusted app deletion dialog. (elastic#78768)
  Make the actual Vislib import async (elastic#78949)
  Fix ML conditionals links Cypress tests (elastic#78568)
  [Drilldowns][Docs] Communicate the visualization types that support drilldowns (elastic#78761)
  [UX] Improve page-load axis (elastic#78392)
  [SECURITY SOLUTIONS] Map embeddable working with index patterns selection (elastic#78610)
  Data plugin README (elastic#78750)
  [TSVB] Request validation error: [panels.0.series.0.metrics.0.percentiles.1.value] (elastic#79009)
  fixing api test (elastic#78964)
  [Task names in TaskManager] Rename "telemetry" to "usage" (elastic#78129)
  [Loggers] Rename "telemetry" to "usage" (elastic#78130)
  [Usage Collection] [schema] `ui_metric` (elastic#78827)
  [Actions][Jira] Set parent issue for Sub-task issue type (elastic#78772)
  [Discover] Unskip doc link functional test (elastic#78600)
  [ML] Functional tests - stabilize calendar edit tests (elastic#78950)
  [UX] Improve page responsive  (elastic#78759)
  [QA][Code Coverage] Team Assignment Docs Update (elastic#78890)
  [ML] Migrate machine learning URLs to BrowserRouter format for APM, Security, and Infra  (elastic#78209)
  [ts] enable "resolveJsonModule" and disable existing failures (elastic#78855)
  ...
@lukeelmers lukeelmers added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Oct 1, 2021
@elasticmachine
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Actions Feature:Alerting Feature:Lens Feature:Telemetry release_note:skip Skip the PR/issue when compiling release notes review Team:APM All issues that need APM UI Team support Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants