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

Fixes regression that prevents maps telemetry from populating. Removes unneeded task manager logic. #54055

Merged
merged 5 commits into from
Jan 9, 2020
Merged

Fixes regression that prevents maps telemetry from populating. Removes unneeded task manager logic. #54055

merged 5 commits into from
Jan 9, 2020

Conversation

kindsun
Copy link
Contributor

@kindsun kindsun commented Jan 6, 2020

Backports the following commits to 7.5:

@elasticmachine
[Maps] Fix regression preventing maps telemetry from populating & rem…
4caf34a 
…ove task manager logic (elastic#52834)

* Remove task logic. Remove server refs and revise for np. Migrate a few files to ts

* Remove unused reference

* Update mappings

* Test usage collector register

* Update api integration tests to include maps now that telemetry is 'normal' (not using task mgr state)

* Update integration test to use stack stats

* Update integration test to look for 'maps-telemetry' instead of 'maps'

* Update jest test to reflect calls to register

* Follow the same pattern as other int tests and test reliable nested attribute

* Back out np-related changes for separate PR

* timeCaptured hasn't changed but for some reason stopped working. Getting iso string fixes issue

* Back out file shuffling for separate PR

* Remove mappings updates (handled in separate PR)

* Review feedback. Move telemetry type constant to constants file

* Consolidate imports

* Linting fix

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kindsun kindsun force-pushed the backport/7.5/pr-52834 branch from dcb6701 to 4caf34a Compare January 6, 2020 21:59
@kindsun kindsun added the blocked label Jan 8, 2020
@Bamieh Bamieh mentioned this pull request Jan 8, 2020
Closed
@kindsun
Copy link
Contributor Author

kindsun commented Jan 8, 2020

Currently blocked by #54294

@Bamieh Bamieh mentioned this pull request Jan 8, 2020
Merged
@kindsun kindsun removed the blocked label Jan 9, 2020
@kindsun
Copy link
Contributor Author

kindsun commented Jan 9, 2020 •
edited
Loading

@thomasneirynck Following up on this comment, I'd like to get your review on this backport prior to merge with 7.5.

The merge itself actually wasn't too difficult, though there were some initial test failures when I first created the PR. The bulk of them were the result of flaky tests in Spaces and were resolved separately by #54154 once these changes were merged. The one maps telemetry test that failed just required updating the syntax to reflect using pre-NP (server ref) syntax, which was an easy switch.

I ran into one other issue (characterized in #54294) and held off on merging this backport to work with @Bamieh on diagnosing and testing solutions. This issue combined with this backport reintroduced behavior we tried to avoid when we created Maps telemetry initially where telemetry was seemingly collected every 10 seconds (expensive for saved objects calls). The issue turned out to be a monitoring bug, a fix for which was merged into 7.5 in #54322.

The latest version of 7.5 including this fix is merged into this backport. The backport now behaves as expected, harvesting telemetry once initially and then as needed (like when the stats endpoint is loaded).

Before Maps are created:

image

After Maps are created:

image

@kindsun kindsun requested a review from thomasneirynck January 9, 2020 15:10
@kindsun kindsun added the [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation label Jan 9, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

thomasneirynck
thomasneirynck approved these changes Jan 9, 2020
View reviewed changes
Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

👨‍🚒

@kindsun kindsun merged commit 9fb2d7f into elastic:7.5 Jan 9, 2020
@kindsun kindsun changed the title [7.5] [Maps] Fix regression preventing maps telemetry from populating & remove task manager logic (#52834) [7.5] [Maps] Fixes regression that prevents maps telemetry from populating & removing task manager logic (#52834) Jan 9, 2020
@kindsun kindsun changed the title [7.5] [Maps] Fixes regression that prevents maps telemetry from populating & removing task manager logic (#52834) Fixes regression that prevents maps telemetry from populating & removing task manager logic Jan 9, 2020
@kindsun kindsun changed the title Fixes regression that prevents maps telemetry from populating & removing task manager logic Fixes regression that prevents maps telemetry from populating. Removes unneeded task manager logic. Jan 9, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@timroes timroes added v7.5.2 and removed 7.5.2 labels Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasneirynck thomasneirynck thomasneirynck approved these changes

Assignees
No one assigned
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:fix v7.5.2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kindsun @elasticmachine @kibanamachine @thomasneirynck @timroes