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] Fix race on overview page query #67843

Merged
merged 9 commits into from
Jun 9, 2020

Conversation

andrewvc
Copy link
Contributor

@andrewvc andrewvc commented Jun 1, 2020

Fixes #67842 by re-querying during the refine phase to see if a newer matching doc has come in. This also simplifies the first 'potential' phase to only return monitor IDs. Getting the top check groups is no longer necessary since we determine this value in the 'refine' phase now.

Checklist

Delete any items that are not applicable to this PR.

Fixes elastic#67842 by requerying
during the refine phase to see if a newer matching doc has come in.
@andrewvc andrewvc added bug Fixes for quality problems that affect the customer experience Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Jun 1, 2020
@andrewvc andrewvc self-assigned this Jun 1, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@andrewvc andrewvc marked this pull request as ready for review June 4, 2020 14:07
@andrewvc andrewvc requested a review from a team as a code owner June 4, 2020 14:07
},
},
latestMatching: {
'@timestamp': '2020-06-04T12:39:54.698-0500',
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't you try increasing the timestamp of latestMatching to cover the use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept this case and added one for a future 'latest' timestamp

@@ -71,6 +71,7 @@ export default function ({ getService }: FtrProviderContext) {
const url = getBaseUrl(dateRangeStart, dateRangeEnd) + `&filters=${filters}`;
const apiResponse = await supertest.get(url);
const nonSummaryRes = apiResponse.body;
// expect(JSON.stringify(nonSummaryRes)).to.eql("foo");
Copy link
Contributor

Choose a reason for hiding this comment

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

if it's not applicable anymore, it's better to remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake, left over from debugging!

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.

It generates error when you select location in filters
image

@andrewvc
Copy link
Contributor Author

andrewvc commented Jun 9, 2020

@shahzad31 I believe I've addressed all feedback here.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

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 !!

@andrewvc andrewvc merged commit ccb6073 into elastic:master Jun 9, 2020
andrewvc added a commit to andrewvc/kibana that referenced this pull request Jun 9, 2020
Fixes elastic#67842 by requerying
during the refine phase to see if a newer matching doc has come in.
andrewvc added a commit to andrewvc/kibana that referenced this pull request Jun 9, 2020
Fixes elastic#67842 by requerying
during the refine phase to see if a newer matching doc has come in.
andrewvc added a commit that referenced this pull request Jun 10, 2020
Fixes #67842 by requerying
during the refine phase to see if a newer matching doc has come in.
andrewvc added a commit that referenced this pull request Jun 10, 2020
Fixes #67842 by requerying
during the refine phase to see if a newer matching doc has come in.
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 10, 2020
* master: (22 commits)
  Partial revert of "Sync Kerberos + Anonymous access tests with the latest `security/_authenticate` API (user roles now include roles of anonymous user)." (elastic#68624)
  adapt some snapshot test (elastic#68489)
  [APM] Service maps - Fix missing ML status for services with jobs but no anomalies (elastic#68486)
  [skip test] apis Kerberos security Kerberos authentication finishing SPNEGO should properly set cookie and authenticate user
  [SIEM][Exceptions] - ExceptionsViewer UI component part 2 (elastic#68294)
  Surface data streams in Index Management. (elastic#67806)
  Fix edit datasource not working following changes in elastic#67234 (elastic#68583)
  [Logs + Metrics UI] Clean up async plugin initialization (elastic#67654)
  APM Storybook fixes (elastic#68671)
  Upgrade EUI to v24.1.0 (elastic#68141)
  [ML] DF Analytics: Creation wizard part 2 (elastic#68462)
  [Uptime] Fix race on overview page query (elastic#67843)
  Prefer using npm_execpath when spawning Yarn (elastic#68673)
  [Security] [Cases] Attach timeline to existing case (elastic#68580)
  Use Search API in Vega (elastic#68257)
  [Component templates] Table view (elastic#68031)
  [Uptime] Added relative date info in cert status column (elastic#67612)
  [Endpoint] Re-enable Functional test case for Endpoint related pages (elastic#68445)
  run page_load_metrics tests in visual regresssion jobs (elastic#68570)
  Enable exhaustive-deps; correct any lint warnings (elastic#68453)
  ...
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 release_note:fix Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Uptime] Race condition leading to elision of overview page monitors
4 participants