Skip to content

[Monitoring] Use async/await #81200

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
Merged
merged 3 commits into from
Oct 21, 2020
Merged

[Monitoring] Use async/await #81200

merged 3 commits into from
Oct 21, 2020

Conversation

@chrisronline
Copy link
Contributor

@chrisronline chrisronline commented Oct 20, 2020 •
edited
Loading

Resolves #81104
Resolves #72007
Resolves #81166
Resolves #81084
Resolves #81225
Resolves #81312
Resolves #53159
Resolves #81340
Resolves #81113

I'm not really sure what's going on here.

I ran tests locally on 7.x and didn't experience any failure. Then, I decided to load up the es archive data and play around with the app to make sure things looked fine. I noticed that I got into an infinite navigation loop by simply loading the cluster overview page and then clicking Clusters in the top left. It went from #/home to #/overview back and forth forever.

I added some debugging and found that this promise was never resolving (successfully or with an error) for some reason when we went from the #/home page to the #/overview. I decided to try and remove the promises to make it easier to read and reason and that seemed to solve it for me, but I welcome someone else to try and reproduce this.

Testing

  1. Load up 7.x Kibana and ES (without SSL because I don't know how to load archive data with SSL)
  2. From the x-pack folder, load a sample archive:
    node ../scripts/es_archiver.js load monitoring/singlecluster-three-nodes-shard-relocation --kibana-url http://elastic:changeme@localhost:5601 --es-url http://elastic:changeme@localhost:9200
  3. Navigate to the Stack Monitoring UI and adjust the time picker to a range that includes the archive data (for this example, 10/5/17 -> 10/6/17 works)
  4. Allow the cluster overview page to naturally load
  5. Click Clusters in the top left and observe the infinite navigation loop

@chrisronline chrisronline added review Team:Monitoring Stack Monitoring team v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v7.11.0 labels Oct 20, 2020
@chrisronline chrisronline requested a review from a team October 20, 2020 16:36
@chrisronline chrisronline self-assigned this Oct 20, 2020
@elasticmachine
Copy link
Contributor

elasticmachine commented Oct 20, 2020

Pinging @elastic/stack-monitoring (Team:Monitoring)

igoristic
igoristic approved these changes Oct 20, 2020
View reviewed changes
Copy link
Contributor

@igoristic igoristic left a comment

Choose a reason for hiding this comment

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

@chrisronline This works, but I don't really know why, since the only thing that was changed here was converting some angular promises into async/await (with try/catch) functions (unless I'm missing something else?)

What I never really like in our code base is that we have logic in several places eg:
x-pack/plugins/monitoring/public/views/cluster/listing/index.js that points to #/overview and our routeInit() function (which is used with all our routes' resolve method) can also redirect you back to #/home. Which I'm sure under the right conditions can cause the infinite loop mentioned above.

But, I honestly don't know if there's a better way of solving this

@tylersmalley
Copy link
Contributor

tylersmalley commented Oct 20, 2020

@chrisronline thanks for addressing this - I ultimately had to skip these tests for now so you will want to revert that as part of this PR: #81166

@chrisronline chrisronline mentioned this pull request Oct 21, 2020
Closed
@kibanamachine
Copy link
Contributor

kibanamachine commented Oct 21, 2020

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
monitoring 1.1MB 1.1MB +3.3KB

History

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

@chrisronline chrisronline merged commit 7017ca6 into elastic:master Oct 21, 2020
@chrisronline chrisronline deleted the monitoring/async_better branch October 21, 2020 18:01
chrisronline added a commit to chrisronline/kibana that referenced this pull request Oct 21, 2020
@chrisronline
[Monitoring] Use async/await (elastic#81200)
e46a3e4 
* Use async/await

* Make sure we tell angular what's up
@chrisronline chrisronline mentioned this pull request Oct 21, 2020
Merged
chrisronline added a commit to chrisronline/kibana that referenced this pull request Oct 21, 2020
@chrisronline
[Monitoring] Use async/await (elastic#81200)
d32637c 
* Use async/await

* Make sure we tell angular what's up
# Conflicts:
#	x-pack/plugins/monitoring/public/services/clusters.js
@chrisronline chrisronline mentioned this pull request Oct 21, 2020
Merged
@spalger spalger mentioned this pull request Oct 21, 2020
Closed
This was referenced Oct 21, 2020
Merged
Closed
chrisronline added a commit that referenced this pull request Oct 21, 2020
@chrisronline
[Monitoring] Use async/await (#81200) (#81370)
70c73ff 
* Use async/await

* Make sure we tell angular what's up
# Conflicts:
#	x-pack/plugins/monitoring/public/services/clusters.js
chrisronline added a commit that referenced this pull request Oct 21, 2020
@chrisronline
[Monitoring] Use async/await (#81200) (#81369)
74697d1 
* Use async/await

* Make sure we tell angular what's up
@chrisronline
Copy link
Contributor Author

chrisronline commented Oct 21, 2020

Backport:

7.10: 70c73ff
7.x: 74697d1

@tylersmalley tylersmalley mentioned this pull request Oct 21, 2020
Merged
@spalger spalger mentioned this pull request Oct 21, 2020
Open
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 22, 2020
@gmmorris
Merge branch 'master' into task-manager/health
12e7866 
* master: (63 commits)
  [KP] Fix Headers timeout issue (elastic#81140)
  [ML] Functional tests - stabilize typing with checks service method (elastic#81338)
  tabify - support docs (elastic#80351)
  [Security Solution][Detections] Look-back time logic fix (elastic#81383)
  [Workplace Search] Add top-level tests for Groups (elastic#81215)
  [Fleet] Fix agent action observable for long polling (elastic#81376)
  [Maps] fix feature tooltip remains open when zoom level change hides layer (elastic#81373)
  skip flaky suite (elastic#78689)
  chore(NA): add spec-to-console and plugin-helpers as devOnly dependencies (elastic#81357)
  Ensure some data is returned (elastic#81375)
  Change dumb-init to tini (elastic#81126)
  [Reporting/Tech Debt] Convert PdfMaker class to TypeScript (elastic#81242)
  Use Storybook Controls instead of Knobs (elastic#80705)
  [junit] make sure that report paths are unique (elastic#81255)
  bump elastic/elasticsearch-js version to 7.10.0-rc1 (elastic#81288)
  run ssl tests on CI (elastic#81320)
  Fix alert defaults (elastic#81207)
  [ML] DF Analytics wizard: ensure user can set mml manually or select to use given estimate (elastic#81078)
  Add UI notifier to indicate secret fields and to remember / reenter values (elastic#80657)
  [Monitoring] Use async/await (elastic#81200)
  ...
@chrisronline chrisronline mentioned this pull request Oct 23, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@igoristic igoristic igoristic approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@chrisronline chrisronline

Labels

release_note:skip Skip the PR/issue when compiling release notes review Team:Monitoring Stack Monitoring team v7.10.0 v7.11.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Failing test: Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/monitoring/elasticsearch/nodes·js - Monitoring app Elasticsearch nodes listing with only online nodes "before all" hook for "should have an Elasticsearch Cluster Summary Status with correct info" Failing test: Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/monitoring/elasticsearch/node_detail·js - Monitoring app Elasticsearch node detail Advanced Active Nodes "before all" hook for "should show node summary of master node with 20 indices and 38 shards" Failing test: Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/monitoring/elasticsearch/index_detail·js - Monitoring app Elasticsearch index detail Deleted Index "before all" hook for "should have an index summary with NA for deleted index" Failing test: Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/monitoring/kibana/overview·js - Monitoring app Kibana overview "before all" hook for "should have Kibana Cluster Summary Status showing correct info" Failing test: Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/monitoring/kibana/instance·js - Monitoring app Kibana instance detail "before all" hook for "should have Instance Summary Status showing correct info"
Failing test: Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/monitoring/elasticsearch/node_detail·js - Monitoring app Elasticsearch node detail Active Nodes "before all" hook for "should show node summary of master node with 20 indices and 38 shards" Failing test: Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/monitoring/elasticsearch/node_detail·js - Monitoring app Elasticsearch node detail Offline Node "before all" hook for "should show node summary of NA for offline node" Failing test: Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/monitoring/logstash/pipelines·js - Monitoring app Logstash pipelines "before all" hook for "should have Logstash Cluster Summary Status showing correct info" Failing test: Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/monitoring/kibana/instances·js - Monitoring app Kibana instances listing "before all" hook for "should have Kibana Cluster Summary Status showing correct info"

5 participants

@chrisronline @elasticmachine @tylersmalley @kibanamachine @igoristic