ui: fix timescale for rolling window #106623
Conversation
|
This change is |
maryliag
force-pushed
the
timestamp-label
branch
from
31893f8 to
3b74bb6
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @maryliag)
a discussion (no related file):
Should requestTime be a part of timeScale at all?
My assumption that:
- it is a part of SQL Activity redux state where we can track the time last request/response was made and have dedicated selector to show this value on UI.
timeScaleis aware about selected options and overloading it with custom cases like this might lead to more regressions. IMO TimeScale and date picker is very fragile for any changes
maryliag
force-pushed
the
timestamp-label
branch
from
3b74bb6 to
fe4ee70
Compare
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, koorosh (Andrii Vorobiov) wrote…
Should
requestTimebe a part oftimeScaleat all?
My assumption that:
- it is a part of SQL Activity redux state where we can track the time last request/response was made and have dedicated selector to show this value on UI.
timeScaleis aware about selected options and overloading it with custom cases like this might lead to more regressions. IMO TimeScale and date picker is very fragile for any changes
My idea was that this parameter was saving the time you changed the option, and this can be used in more pages then just SQL Activity, also Insights, Index Details, and so on, so is the responsibility of the component using the timescale to decide if that value is required or not and what it should be. But the component itself is just preparing to receive and do the proper label formatting for this case.
This way this value doesn't affect at all the behaviour of the component, is just about having a label, but at the same time is uniform against all usage.
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, maryliag (Marylia Gutierrez) wrote…
My idea was that this parameter was saving the time you changed the option, and this can be used in more pages then just SQL Activity, also Insights, Index Details, and so on, so is the responsibility of the component using the timescale to decide if that value is required or not and what it should be. But the component itself is just preparing to receive and do the proper label formatting for this case.
This way this value doesn't affect at all the behaviour of the component, is just about having a label, but at the same time is uniform against all usage.
I can update the name if there is something more appropriate, such as "selectionTime", since that is what it means anyway.
maryliag
force-pushed
the
timestamp-label
branch
from
fe4ee70 to
7075c07
Compare
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, maryliag (Marylia Gutierrez) wrote…
I can update the name if there is something more appropriate, such as "selectionTime", since that is what it means anyway.
It is stored in src/redux/timeScale.ts as a single value so it cannot be reused in multiple places.
When requestTime is set on SQL Activity page, it will be updated on other pages as well because timeScale is a global thing.
maryliag
force-pushed
the
timestamp-label
branch
from
7075c07 to
2c12834
Compare
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, koorosh (Andrii Vorobiov) wrote…
It is stored in src/redux/timeScale.ts as a single value so it cannot be reused in multiple places.
WhenrequestTimeis set on SQL Activity page, it will be updated on other pages as well becausetimeScaleis a global thing.
By reuse I just meant the feature itself, not the we would have several values. Sorry about the confusion.
Seems like that solution was causing more confusion, so I switched to store the value on local storage instead.
PTAL
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, maryliag (Marylia Gutierrez) wrote…
By reuse I just meant the feature itself, not the we would have several values. Sorry about the confusion.
Seems like that solution was causing more confusion, so I switched to store the value on local storage instead.
PTAL
I thought timeScale should already have correct end value for non-custom options (ie it is set to current time)? If it's true then it can be used directly?
Maybe I'm missing something important in current logic of date range selector, need to learn how it works rn.
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, koorosh (Andrii Vorobiov) wrote…
I thought
timeScaleshould already have correctendvalue for non-custom options (ie it is set to current time)? If it's true then it can be used directly?
Maybe I'm missing something important in current logic of date range selector, need to learn how it works rn.
For non-custom, the end value was always null, which allow rolling window to keep going (Past 1h keep getting the end updated to "now").
What I have done previously was setting up a value for the end value even for those cases, since we pause the requests on SQL Activity page. The problem was that this also made the rolling window pause on the Metrics page, and we don't want that. So my attempt to reuse the end parameter was not correct, so I had to move away from it.
I tried to keep using something still inside TimeScale but seems to be causing more confusion, so I used something separate, and should be good now!
maryliag
force-pushed
the
timestamp-label
branch
from
2c12834 to
dd9cdc5
Compare
There was a problem hiding this comment.
Reviewed 15 of 28 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status:
a discussion (no related file):
Previously, maryliag (Marylia Gutierrez) wrote…
For non-custom, the end value was always null, which allow rolling window to keep going (Past 1h keep getting the end updated to "now").
What I have done previously was setting up a value for the end value even for those cases, since we pause the requests on SQL Activity page. The problem was that this also made the rolling window pause on the Metrics page, and we don't want that. So my attempt to reuse the end parameter was not correct, so I had to move away from it.
I tried to keep using something still inside TimeScale but seems to be causing more confusion, so I used something separate, and should be good now!
Generally, I think I like the idea of separating the request time from the timescale. Why is it that we want different request times for statements/transactions pages? I thought the idea more generally was to keep the same time window for both ?
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, THardy98 (Thomas Hardy) wrote…
Generally, I think I like the idea of separating the request time from the timescale. Why is it that we want different request times for statements/transactions pages? I thought the idea more generally was to keep the same time window for both ?
The requests don't always align, if you refresh Transaction not necessarily updates the Statement, so I want to make it clear the values for each one.
To clarify this PR doesn't change anything about when/how the requests are made. Is only storing when they were made so we can then surface on the UI.
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, maryliag (Marylia Gutierrez) wrote…
The requests don't always align, if you refresh Transaction not necessarily updates the Statement, so I want to make it clear the values for each one.
To clarify this PR doesn't change anything about when/how the requests are made. Is only storing when they were made so we can then surface on the UI.
Ah ok. So for these pages, the timescale is moreso acting as a starting time, and the end time is the request time (in the default, non-custom case).
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, THardy98 (Thomas Hardy) wrote…
Ah ok. So for these pages, the timescale is moreso acting as a starting time, and the end time is the request time (in the default, non-custom case).
Yes, that's correct (since we stopped auto refreshing those pages)
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, maryliag (Marylia Gutierrez) wrote…
Yes, that's correct (since we stopped auto refreshing those pages)
Cool, then 
Thanks for the fix!
The change introduced on cockroachdb#105157 had an undesired change on the Metrics page. We want to show the end period of the select, but when the fixed window end for that this caused the Metrics page to stop loading new data. We want the metrics page to keep updating when any of the rolling windows is selected, and we also want to know the time a time period was requested, so it can be used to provide more information on SQL Activity pages. This commit remove the setting of the fixedWindowEnd, since that was not the best approach and instead creates a value on local storage for the requestTime, that can be used to create the label for the timescale, without interferring on Metrics page (or any other that have an automatic update). Epic: none Release note (bug fix): Fix the Metrics page that was not updating automatically on rolling window options.
maryliag
force-pushed
the
timestamp-label
branch
from
dd9cdc5 to
51f5a93
Compare
|
bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 51f5a93 to blathers/backport-release-23.1-106623: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
Hello, I have a customer that is still facing this UI issue and it is getting to the point where its impacting their ability to further develop their app and troubleshoot issues as they arise, if this drags on much longer its going to impact the production timeline and the overall customer confidence, reqeusting a follow up on when this UI issue will get fixed, thank you |
|
Not seeing that this github issue is linked to the Zendesk ticket so dropping this link here to the ticket in case it is helpful: https://cockroachdb.zendesk.com/agent/tickets/18003 Seems this pull request has been merged but the issue is still happening ... unclear what's happening .. maybe this is already fixed but the serverless cluster they're running is not on the proper version yet? .. .. thank you CC @world2mark |
|
This fix is not yet on Serverless cluster, make take a few more days to get there. I can update the ticket once is available there. |
The change introduced on #105157 had an undesired
change on the Metrics page. We want to show the end period
of the select, but when the fixed window end for that this
caused the Metrics page to stop loading new data.
We want the metrics page to keep updating when any of the
rolling windows is selected, and we also want to know
the time a time period was requested, so it can be used
to provide more information on SQL Activity pages.
This commit remove the setting of the fixedWindowEnd, since
that was not the best approach and instead creates a value
on local storage for the requestTime, that can be used to create
the label for the timescale, without interferring on
Metrics page (or any other that have an automatic update).
Epic: none
Release note (bug fix): Fix the Metrics page that was not
updating automatically on rolling window options.
Release note (bug fix): Statement Diagnostics not always
showing is now fixed and they show for the correct time period selected.