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.

Sign up for GitHub

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

Jump to bottom

[Telemetry] Migrate ui_metric plugin to NP under usageCollection #51972

Merged
merged 37 commits into from
Dec 13, 2019
Merged

[Telemetry] Migrate ui_metric plugin to NP under usageCollection #51972

merged 37 commits into from
Dec 13, 2019

Conversation

Bamieh
Copy link
Member

@Bamieh Bamieh commented Dec 2, 2019 •
edited
Loading

Summary

Migrates ui_metric to New platform

  • Move ui_metric under usage_collection plugin.
  • Remove legacy ui_metric plugin code.
  • Add new_platform mocks for NP ui_metrics
  • Add karma mocks for NP ui_metrics
  • Fix any failing tests

Dev Docs

Migrates ui_metrics to NP under usageCollection.

@Bamieh Bamieh added Feature:New Platform Feature:Telemetry release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.6.0 v8.0.0 labels Dec 2, 2019
@Bamieh Bamieh marked this pull request as ready for review December 9, 2019 15:50
@Bamieh Bamieh requested a review from a team December 9, 2019 15:50
@Bamieh Bamieh requested a review from a team as a code owner December 9, 2019 15:50
@Bamieh Bamieh requested a review from a team as a code owner December 10, 2019 20:09
sorenlouv
sorenlouv approved these changes Dec 11, 2019
View reviewed changes
Copy link
Member

@sorenlouv sorenlouv 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 lgtm

@Bamieh
Copy link
Member Author

Bamieh commented Dec 11, 2019

@elasticmachine merge upstream

@Bamieh Bamieh changed the title Telemetry/ui metric to usage collection [Telemetry] Migrate ui_metric plugin to NP under usageCollection Dec 11, 2019
@elastic elastic deleted a comment from elasticmachine Dec 11, 2019
@jloleysens jloleysens mentioned this pull request Dec 12, 2019
Merged
@Bamieh
Copy link
Member Author

Bamieh commented Dec 12, 2019

waiting on this PR to get merged first before merging mine: #52251

To resolve renaming configs issue between legacy and NP plugins.

@Bamieh Bamieh requested a review from a team as a code owner December 13, 2019 14:22
@Bamieh Bamieh removed request for a team December 13, 2019 14:23
TinaHeiligers
TinaHeiligers reviewed Dec 13, 2019
View reviewed changes
Copy link
Contributor

@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.

A few nits and comments, otherwise the code looks good.
I'm doing a smoke test in Chrome and will re-review once that's done.

src/plugins/usage_collection/public/services/create_reporter.ts Outdated Show resolved Hide resolved
src/plugins/usage_collection/public/services/create_reporter.ts Outdated Show resolved Hide resolved
src/plugins/usage_collection/public/services/create_reporter.ts Outdated Show resolved Hide resolved
src/plugins/usage_collection/server/config.ts Outdated Show resolved Hide resolved
src/plugins/usage_collection/server/plugin.ts Outdated Show resolved Hide resolved
src/plugins/usage_collection/server/plugin.ts Outdated Show resolved Hide resolved
src/plugins/usage_collection/server/routes/report_metrics.ts Show resolved Hide resolved
src/plugins/usage_collection/server/store_report.ts Outdated Show resolved Hide resolved
src/plugins/usage_collection/server/plugin.ts Outdated Show resolved Hide resolved
@Bamieh Bamieh requested a review from TinaHeiligers December 13, 2019 18:21
TinaHeiligers
TinaHeiligers approved these changes Dec 13, 2019
View reviewed changes
Copy link
Contributor

@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.

The code changes look good and make the types much clearer. Thanks for giving the default max wait time for all collectors such a descriptive name, it'll be very obvious now!
I also ran a test in Chrome and Safari and the move of the ui_metric code works as expected.
LGTM!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

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

@Bamieh Bamieh merged commit 3658220 into elastic:master Dec 13, 2019
@Bamieh Bamieh deleted the telemetry/ui_metric_to_usage_collection branch December 13, 2019 19:41
@Bamieh Bamieh mentioned this pull request Dec 13, 2019
Merged
@afharo afharo mentioned this pull request Aug 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgayvallet pgayvallet pgayvallet approved these changes

@TinaHeiligers TinaHeiligers TinaHeiligers approved these changes

@crob611 crob611 crob611 approved these changes

@sorenlouv sorenlouv sorenlouv approved these changes

Assignees
No one assigned
Labels
Feature:New Platform Feature:Telemetry release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.6.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Bamieh @elasticmachine @sorenlouv @crob611 @pgayvallet @TinaHeiligers