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.

Sign up for GitHub

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

Jump to bottom

[Uptime] Update relative time handling #55693

Merged
merged 16 commits into from
Jan 31, 2020
Merged

[Uptime] Update relative time handling #55693

merged 16 commits into from
Jan 31, 2020

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Jan 23, 2020 •
edited
Loading

Summary

Fixes part of #53991
When use selected this week, it translate to to="now/w" and from = "now/w" in relative time.

This PR will handle relative time handling in Server for MonitorList API.

When end date is set in future, it will reset it to now for es queries.

Checklist

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

For maintainers

@shahzad31 shahzad31 requested a review from andrewvc January 23, 2020 13:14
@shahzad31 shahzad31 self-assigned this Jan 23, 2020
@shahzad31 shahzad31 added release_note:fix Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.6.0 v7.7.0 v8.0.0 labels Jan 23, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@shahzad31 shahzad31 added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:fix labels Jan 23, 2020
@shahzad31
Copy link
Contributor Author

@elasticmachine merge upstream

justinkambic
justinkambic reviewed Jan 29, 2020
View reviewed changes
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.

I'm unsure if I'm seeing a desired behavior - are we ok with this week showing future dates? I was under the impression we were trying to avoid this but I might've misunderstood. Also asked for tests for a new function.

image

@shahzad31
Copy link
Contributor Author

I'm unsure if I'm seeing a desired behavior - are we ok with this week showing future dates? I was under the impression we were trying to avoid this but I might've misunderstood. Also asked for tests for a new function.

@justinkambic in UI it's fine to show and select future date, but i have made sure, ES queries doesn't cause error if that happens.

@andrewvc
Copy link
Contributor

@justinkambic In the discover tab this week also shows a future date. I agree it's weird, but it's a strange UX problem to solve. SS below from discover:

image

andrewvc
andrewvc requested changes Jan 30, 2020
View reviewed changes
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.

Seems to work right in manual testing, but code could use some improvements

@shahzad31
Copy link
Contributor Author

@andrewvc have improved the code and have revised unnecessary changes.

andrewvc
andrewvc requested changes Jan 31, 2020
View reviewed changes
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.

Looking much better, but could still use some minor tweaks.

andrewvc
andrewvc approved these changes Jan 31, 2020
View reviewed changes
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 WFG

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@shahzad31 shahzad31 merged commit 49e9c79 into elastic:master Jan 31, 2020
@shahzad31 shahzad31 deleted the fix/monitor-list-time-range-error branch January 31, 2020 23:20
This was referenced Feb 2, 2020
Merged
Merged
shahzad31 added a commit to shahzad31/kibana that referenced this pull request Feb 2, 2020
@shahzad31 @elasticmachine
[Uptime] Update relative time handling (elastic#55693)
0c0f6e7 
* update relative time handling

* fix type

* update logic to handle end date

* fix test

* PR feedback

* refactor code

* refactor interval time

* fix tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
shahzad31 added a commit that referenced this pull request Feb 3, 2020
@shahzad31 @elasticmachine
[Uptime] Update relative time handling (#55693) (#56590)
2ce494f 
* update relative time handling

* fix type

* update logic to handle end date

* fix test

* PR feedback

* refactor code

* refactor interval time

* fix tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
shahzad31 added a commit that referenced this pull request Feb 3, 2020
@shahzad31 @elasticmachine
[Uptime] Update relative time handling (#55693) (#56589)
2c87f52 
* update relative time handling

* fix type

* update logic to handle end date

* fix test

* PR feedback

* refactor code

* refactor interval time

* fix tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinkambic justinkambic justinkambic left review comments

@andrewvc andrewvc andrewvc approved these changes

Assignees

@shahzad31 shahzad31

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@shahzad31 @elasticmachine @andrewvc @kibanamachine @justinkambic