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

[Uptime][Synthetics integration] remove optional tag from timeout field #99410

Conversation

dominiqueclarke
Copy link
Contributor

Summary

Fixes #99373

Removes Optional tag from timeout field

Screen Shot 2021-05-05 at 4 18 03 PM

@dominiqueclarke dominiqueclarke added bug Fixes for quality problems that affect the customer experience v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v7.13.0 auto-backport Deprecated - use backport:version if exact versions are needed labels May 5, 2021
@dominiqueclarke dominiqueclarke requested a review from a team as a code owner May 5, 2021 20:30
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@justinkambic
Copy link
Contributor

Now that I'm seeing this I'm wondering if it should be Optional after all. You can omit the timeout field from a Heartbeat YAML file and it will just use its default 16s. cc @andrewvc any thoughts?

@dominiqueclarke
Copy link
Contributor Author

dominiqueclarke commented May 10, 2021

@justinkambic @andrewvc I'm handling some of the defaults by matching what is defaulted in heartbeat to the default value that is populated in the fields on mount. Since the default timeout is 16 seconds, I use 16 seconds as the default value. I think it would be confusing, in some ways, if a user omitted this entirely and yet the timeout applied was still the default 16 seconds. I think current UX better aids in ensuring the policy that is created is the policy the user intended.

@dominiqueclarke
Copy link
Contributor Author

@elasticmachine merge upstream

@dominiqueclarke
Copy link
Contributor Author

@elasticmachine merge upstream

@justinkambic
Copy link
Contributor

it would be confusing, in some ways, if a user omitted this entirely and yet the timeout applied was still the default 16 seconds. I think current UX better aids in ensuring the policy that is created is the policy the user intended.

That's a good point. Ok, I'm fine with it being required, esp. since we give the default value HB will use anyway.

We also discussed offline the implications of requiring the timeout to be lower than the interval. When configuring HB manually, it will allow the interval to be below the timeout.

We feel that since this is not a frequent configuration choice, and that it's probably not a good practice anyway, we will leave in the requirement that the request timeout be faster than the ping interval.

Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
uptime 1.2MB 1.2MB -85.0B
Unknown metric groups

References to deprecated APIs

id before after diff
canvas 29 25 -4
crossClusterReplication 8 6 -2
fleet 4 2 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 5 3 -2
licensing 18 15 -3
monitoring 109 56 -53
total -73

History

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

@dominiqueclarke dominiqueclarke merged commit 4b22037 into elastic:master May 12, 2021
@dominiqueclarke dominiqueclarke deleted the fix/99373-synthetics-remove-optional-tag-from-timeout-field branch May 12, 2021 15:35
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request May 12, 2021
…tion (elastic#99410)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request May 12, 2021
…tion (elastic#99410)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.13
7.x

The backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request May 12, 2021
…tion (#99410) (#99947)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Dominique Clarke <doclarke71@gmail.com>
kibanamachine added a commit that referenced this pull request May 12, 2021
…tion (#99410) (#99948)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Dominique Clarke <doclarke71@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.13.0 v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Uptime] Synthetics http timeout field listed as optional
4 participants