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 timespan referencing to same values (#56601) #56612

Merged

Conversation

shahzad31
Copy link
Contributor

Summary

Fixes Timespan using same object for tsStart and tsEnd in Monitor List API !!

Checklist

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

For maintainers

@shahzad31 shahzad31 added 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.7.0 v7.6.0 labels Feb 3, 2020
@shahzad31 shahzad31 requested a review from andrewvc February 3, 2020 13:03
@shahzad31 shahzad31 self-assigned this Feb 3, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@justinkambic
Copy link
Contributor

What's the end result to the behavior of the application? This seems like a bug that CI should have caught before it was merged, if I'm understanding the code change correctly.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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

@shahzad31
Copy link
Contributor Author

What's the end result to the behavior of the application? This seems like a bug that CI should have caught before it was merged, if I'm understanding the code change correctly.

So because of using the same object, it was subtracting 5 minutes from both dates, hence setting timespan to 5 minutes in the past, because of that we are getting results which are always 5 minutes ago.

@justinkambic Well it's related to that timespan andrew increased and relative time handling i did.

So CI won't be able to catch it since it just increases or reduces timespan.

@shahzad31
Copy link
Contributor Author

@andrewvc can probably add more why this wasn't go caught in CI, or how can we improve the tests for it.

@andrewvc
Copy link
Contributor

andrewvc commented Feb 3, 2020

The reason this was missed, most likely, is that we do a time intersection. So, the bug here resulted in a zero length point which still intersected the most recent checks (most likely).

What we need to add is a test that checks that we still match data that's somewhat stale. So, in other words do the query on a dataset that has data with timespans ending say a minute ago.

I'm OK with merging this as-is given where we are in the FF with manual testing, but we'll need to add a CI test here as well.

@andrewvc
Copy link
Contributor

andrewvc commented Feb 3, 2020

For the CI test, to be clear, I'm OK adding it after merging this PR.

Copy link
Contributor

@andrewvc andrewvc 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 locally on this branch and master by setting a time range of last '15s'. This breaks without this patch.

@shahzad31 shahzad31 merged commit 4aa7275 into elastic:master Feb 3, 2020
@shahzad31 shahzad31 deleted the fix/timespan-refernceing-to-same-object branch February 3, 2020 18:00
andrewvc added a commit that referenced this pull request Feb 4, 2020
Add Unit tests for the QueryContext class that was missing testing. This would have caught #56612
andrewvc added a commit to andrewvc/kibana that referenced this pull request Feb 4, 2020
)

Add Unit tests for the QueryContext class that was missing testing. This would have caught elastic#56612
shahzad31 added a commit to shahzad31/kibana that referenced this pull request Feb 4, 2020
andrewvc added a commit that referenced this pull request Feb 5, 2020
…56718)

Add Unit tests for the QueryContext class that was missing testing. This would have caught #56612

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.6.0 v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants