-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Round start and end values #89030
Round start and end values #89030
Conversation
When getting the start and end times, use the d3 time scale `ticks` function to round the start and end times. Example from a query: Before: ```json { "range": { "@timestamp": { "gte": 1611262874814, "lte": 1611263774814, "format": "epoch_millis" } } }, ``` After: ```json { "range": { "@timestamp": { "gte": 1611263040000, "lte": 1611263880000, "format": "epoch_millis" } } }, ``` The `ticks` function makes it so the amount of rounding is proportional to the size of the time range, so shorter time ranges will be rounded less. Also fix a bug where invalid ranges in the query string were not handled correctly. Fixes elastic#84530.
Pinging @elastic/apm-ui (Team:apm) |
Jest is failing here because of timezone differences (it passes locally in GMT-5.) Working to fix that. |
x-pack/plugins/apm/public/context/url_params_context/helpers.ts
Outdated
Show resolved
Hide resolved
@dgieselaar |
@smith why scaleUtc() instead of scaleTime()? I assume the "nice" values should be relative to the current timezone. The ticks generated in this helper should preferably be the same as the ones in the charts. Do you happen to know if that's aligned? |
I think this also "breaks" the refresh button. Ideally if we can fix that we'll also address the issue of data not refreshing when an absolute time range is selected. |
x-pack/plugins/apm/public/context/url_params_context/url_params_context.test.tsx
Outdated
Show resolved
Hide resolved
@dgieselaar the way we generate the ticks is not identical to what elastic-charts does, but they are both using Since the charts are rendering the timeseries returned from the queries, and the start/end in those queries is based on our tick calculations here, those should line up and we aren't doing anything with anything other than the first and last values here. |
@dgieselaar The refresh button now won't do anything if the rounded start and end times haven't changed. If you set the time range to something very narrow (1-2 min) then clicking the button will "work" more often. I'm not sure how we should handle this. It might make sense to disable the refresh button if an absolute range is selected, but for a relative time range where it's rounded we would have to periodically recalculate the range and update the button based on that. Having the refresh button do nothing (but not be disabled) when there's no data to fetch is the current behavior with an absolute range. Can we ship this change which makes it do the same thing for a relative range when there's nothing new to fetch? I agree that it's somewhat confusing behavior but it does make sense. |
On my phone so apologies if it's short. I think we should definitely fix the refresh button. If the date range is the same it does not mean that there's nothing new to fetch. Eg data might be delayed. Also we are picking a date in the future. For an absolute time range we might have gotten away with it (though preferably we should fix that as well). I'm fine with fixing that separately as long as we absolutely make sure we fix it for 7.12. The other thing I think we should consider is adding an extra bucket at the end if that makes sense. Nik's recommendation was to pick a time somewhere in the future (e.g. ten minutes). I'm not sure we have to always pick at least 10m but one extra bucket/tick would provide a little bit of a cushion. |
@dgieselaar I agree with all that. I think an ok solution for right now is to only round the low end, so I'll try that. |
Agree that the refresh button should work - also for absolute ranges. |
I'm assuming this is hard to fix because start/end are dependencies for the useFetcher() hooks, and those values do not change. Two possible solutions:
|
x-pack/plugins/apm/public/context/url_params_context/url_params_context.tsx
Outdated
Show resolved
Hide resolved
retest |
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
@@ -60,7 +58,8 @@ const UrlParamsProvider: React.ComponentClass<{}> = withRouter( | |||
|
|||
const { start, end, rangeFrom, rangeTo } = refUrlParams.current; | |||
|
|||
const [, forceUpdate] = useState(''); | |||
// Counter to force an update in useFetcher when the refresh button is clicked. | |||
const [rangeId, setRangeId] = useState(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a setCounter
method in useFetcher that's invoked when calling refetch()
. Do we need both or can we somehow combine them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One is a mechanism to identify when the url changes, the other is a mechanism to refetch upon an action. We might need both but perhaps we can them improve the names so they indicate what they each do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refetch
and refreshTimeRange
are used differently like you said, and we're not using useFetcher
in the datepicker so don't have access to a counter there. I think this is sufficient as-is but if we want to further refactor and clarify later that would be good.
@@ -136,6 +138,7 @@ export function useFetcher<TReturn>( | |||
}, [ | |||
counter, | |||
preservePreviousData, | |||
rangeId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realised something, what happens if there's a fetch call that doesn't use the range, will it refresh anyway? I'd think that's fine anyway, but maybe good to think about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but probably good to have a look at Søren's feedback before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Just one comment on setCounter
and setRangeId
but nothing blocking
When getting the start and end times, use the d3 time scale `ticks` function to round the start and end times. Example from a query: Before: ```json { "range": { "@timestamp": { "gte": 1611262874814, "lte": 1611263774814, "format": "epoch_millis" } } }, ``` After: ```json { "range": { "@timestamp": { "gte": 1611263040000, "lte": 1611263880000, "format": "epoch_millis" } } }, ``` The `ticks` function makes it so the amount of rounding is proportional to the size of the time range, so shorter time ranges will be rounded less. Also fix a bug where invalid ranges in the query string were not handled correctly. Fixes elastic#84530.
When getting the start and end times, use the d3 time scale `ticks` function to round the start and end times. Example from a query: Before: ```json { "range": { "@timestamp": { "gte": 1611262874814, "lte": 1611263774814, "format": "epoch_millis" } } }, ``` After: ```json { "range": { "@timestamp": { "gte": 1611263040000, "lte": 1611263880000, "format": "epoch_millis" } } }, ``` The `ticks` function makes it so the amount of rounding is proportional to the size of the time range, so shorter time ranges will be rounded less. Also fix a bug where invalid ranges in the query string were not handled correctly. Fixes #84530.
…-ml-jobs * 'master' of github.com:elastic/kibana: (254 commits) [Security Solution] [Detections] Remove allow_no_indices to prevent error being thrown in response of field capabilities (elastic#89927) Skip test for cloud (elastic#89450) [Fleet] Fix duplicate data streams being shown in UI (elastic#89812) Bump package dependencies (elastic#90034) [App Search] DRY helper for encoding/decoding routes that can have special characters in params (elastic#89811) TypeScript project references for Observability plugin (elastic#89320) [SearchSource] Combine sort and parent fields when serializing (elastic#89808) Made imports static (elastic#89935) [ml] migrate file_data_visualizer/import route to file_upload plugin (elastic#89640) [Discover] Adapt default column behavior (elastic#89826) Round start and end values (elastic#89030) Rename getProxyAgents to getCustomAgents (elastic#89813) [Form lib] UseField `onError` listener (elastic#89895) [APM] use latency sum instead of avg for impact (elastic#89990) migrate more core-owned plugins to tsproject ref (elastic#89975) [Logs UI] Load <LogStream> entries via async searches (elastic#86899) [APM] Abort browser requests when appropriate (elastic#89557) [Alerting] Allow user to select existing connector of same type when fixing broken connector (elastic#89062) [Data Table] Use shared CSV export mechanism (elastic#89702) chore(NA): improve logic check when installing Bazel tools (elastic#89634) ...
When getting the start and end times, use the d3 time scale
ticks
function to round the start and end times.The
ticks
function makes it so the amount of rounding is proportional to the size of the time range, so shorter time ranges will be rounded less.See the test examples in the PR to see what the rounding looks like.
Add a counter in the url params handling that increments when the refresh button is clicked. This makes it so a refresh will happen if the rounded time range has not changed and also fix the refresh behavior for absolute time ranges.
Also fix a bug where invalid ranges in the query string were not handled correctly.
Fixes #84530.