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

[Core] Add different request lib to telemetry_analytics in place of $http #49671

Conversation

justinkambic
Copy link
Contributor

Summary

Resolves #49263.

As part of our 7.5 testing we discovered that our routing was broken. This PR aims to address that without breaking the upstream code for other consumers. The existing code is planned to be decommissioned in 7.6 anyway, and the ultimate root of the cause is due to an Angular 2 dependency.

Testing

There are two main areas to test when evaluating the performance of this patch:

Server

We need to make sure that the report still gets to the server in the same format it was before. Testing this should be pretty straightforward.

  1. Before you check out this patch, do this on master:
  2. Add the following line here:
console.log(JSON.stringify(report, null, 2));
  1. Open up the Uptime app with some monitors (ping me for help with configuring Heartbeat)
  2. Navigate to the monitor page (you should be able to observe the bug this PR is fixing on master)
  3. Look at your server output and store the result of our console.log() call
  4. Checkout this patch and repeat the process. Ensure that the reports we receive are identical

Client

You can view the broken functionality on the linked issue. Check that this doesn't happen for you on this patch branch.

  1. Open your browser's network console
  2. Load this patch with some Heartbeat monitors (ping me if you need help setting up Heartbeat)
  3. Navigate to the Monitor page for one of your monitors
  4. Wait until you see a call to report, like below:
    image
  5. If the app's route hasn't reset like in the linked issue, the patch is working as desired

Additional concerns

@Bamieh I'm interested in your feedback on the code responsible for the post. I imported Axios but if there's a preferred method of doing it please request the change. It seems like most of this code will be deleted in the next release anyway, but I want to make sure we do things appropriately.

@justinkambic justinkambic added bug Fixes for quality problems that affect the customer experience Feature:Telemetry v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.5.0 v7.6.0 labels Oct 29, 2019
@justinkambic justinkambic requested review from Bamieh and a team October 29, 2019 19:55
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@justinkambic justinkambic changed the title [Core] Add axios to telemetry_analytics in place of $http [Core] Add different request lib to telemetry_analytics in place of $http Oct 29, 2019
@andrewvc
Copy link
Contributor

Seems to work, passes the smoke test.

Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

LGTM !

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@justinkambic
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@justinkambic
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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

@justinkambic justinkambic merged commit b4ecd5a into elastic:master Oct 31, 2019
justinkambic added a commit to justinkambic/kibana that referenced this pull request Oct 31, 2019
… `$http` (elastic#49671)

* Add axios to telemetry_analytics in place of .

* Remove axios in favor of fetch.
justinkambic added a commit to justinkambic/kibana that referenced this pull request Oct 31, 2019
… `$http` (elastic#49671)

* Add axios to telemetry_analytics in place of .

* Remove axios in favor of fetch.
justinkambic added a commit that referenced this pull request Oct 31, 2019
… `$http` (#49671) (#49888)

* Add axios to telemetry_analytics in place of .

* Remove axios in favor of fetch.
justinkambic added a commit that referenced this pull request Oct 31, 2019
… `$http` (#49671) (#49887)

* Add axios to telemetry_analytics in place of .

* Remove axios in favor of fetch.
@justinkambic justinkambic deleted the core_remove-http-from-telemtry-reporter branch October 31, 2019 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Telemetry release_note:fix Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.5.0 v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Uptime] Route changes on telemetry submit
5 participants