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

Remove Unnecessary Delay #25897

Merged
merged 10 commits into from
Dec 28, 2018
Merged

Conversation

pickypg
Copy link
Member

@pickypg pickypg commented Nov 19, 2018

Summary

This removes an unnecessary delay in the Telemetry code that caused the code to wait an extra minute before trying to send data per page load.

This also adds logic so that the code will wait until any previous attempt to send telemetry causes follow-up attempts to wait until it has completed sending (in the unlikely event that it takes longer than a minute, we don't want to waste time / resources).

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@pickypg pickypg added non-issue Indicates to automation that a pull request should not appear in the release notes v7.0.0 Feature:Telemetry v6.6.0 labels Nov 19, 2018
@pickypg pickypg requested review from tsullivan and legrego November 19, 2018 23:07
@elasticmachine
Copy link
Contributor

💔 Build Failed

@legrego
Copy link
Member

legrego commented Nov 26, 2018

For my own understanding, the delay you removed was originally needed when this was an opt-out decision, rather than an opt-in? I'm not terribly familiar with telemetry's history

@pickypg
Copy link
Member Author

pickypg commented Nov 26, 2018

For my own understanding, the delay you removed was originally needed when this was an opt-out decision, rather than an opt-in? I'm not terribly familiar with telemetry's history

Correct. It was intended to give time for the end user to opt out. Now that it has been opt in, they have unlimited time, so an artificial delay is unnecessary.

@pickypg
Copy link
Member Author

pickypg commented Nov 26, 2018

(The test failures in this are legitimate, but I haven't had time to fix them yet)

@pickypg
Copy link
Member Author

pickypg commented Dec 3, 2018

I'm going to block this on #26575 and instead focus this PR on the tweaking to avoid sending in parallel.

@tsullivan
Copy link
Member

Sorry, I'm not sure how I missed this in my notifications!

@tsullivan
Copy link
Member

I merged #26575 so merge conflicts now need to be resolved

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM

Tested with Charles proxy and delaying the _stats requests, to trigger overlaps in fetching the stats. This PR prevents parallel fetch calls happening on the page

@pickypg pickypg added v6.7.0 and removed v6.6.0 labels Dec 27, 2018
This removes an unnecessary delay in the Telemetry code that caused
the code to wait an extra minute before trying to send data per page
load.

This also adds logic so that the code will wait until any previous
attempt to send telemetry causes follow-up attempts to wait until
it has completed sending (in the unlikely event that it takes
longer than a minute, we don't want to waste time / resources).
@pickypg pickypg force-pushed the telemetry/remove-delay branch from 9e103d1 to 2d8417f Compare December 27, 2018 20:11
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@pickypg
Copy link
Member Author

pickypg commented Dec 28, 2018

I rewrote the original tests to be unit tests, but could not run them locally (not really sure why), which was unfortunate as it created a lot of commit churn.

@pickypg pickypg merged commit bbe6f2b into elastic:master Dec 28, 2018
@pickypg pickypg deleted the telemetry/remove-delay branch December 28, 2018 19:39
pickypg added a commit that referenced this pull request Dec 28, 2018
This adds logic so that the code will wait until any previous
attempt to send telemetry causes follow-up attempts to wait until
it has completed sending (in the unlikely event that it takes
longer than a minute, we don't want to waste time / resources).
@pickypg
Copy link
Member Author

pickypg commented Dec 28, 2018

6.x/6.7: 8b64e62

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Telemetry non-issue Indicates to automation that a pull request should not appear in the release notes v6.7.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants