Round start and end values #89030

smith · 2021-01-21T21:27:31Z

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.

@timestamp

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.

elasticmachine · 2021-01-21T21:27:35Z

Pinging @elastic/apm-ui (Team:apm)

smith · 2021-01-22T01:45:12Z

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

smith · 2021-01-25T21:18:14Z

@dgieselaar nice-ing the domain worked as you described. Also switched to scaleUtc from scaleTime.

dgieselaar · 2021-01-26T12:29:51Z

@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?

dgieselaar · 2021-01-26T12:31:27Z

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

x-pack/plugins/apm/public/context/url_params_context/helpers.test.ts

x-pack/plugins/apm/public/context/url_params_context/helpers.ts

smith · 2021-01-26T20:05:31Z

@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?

@dgieselaar the way we generate the ticks is not identical to what elastic-charts does, but they are both using scaleUtc: https://github.com/elastic/elastic-charts/blob/dce139a1e72b2ad136cb09dc4c705dcfff1720e6/src/scales/scale_continuous.ts#L52

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.

smith · 2021-01-26T20:29:11Z

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.

@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.

dgieselaar · 2021-01-26T20:36:44Z

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.

smith · 2021-01-26T22:10:19Z

@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.

sorenlouv · 2021-01-27T08:41:05Z

Agree that the refresh button should work - also for absolute ranges.

dgieselaar · 2021-01-27T08:50:22Z

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:

Instead of using start/end, use a { range: { start, end } } object that changes identity when the refresh button is pressed, regardless of whether start/end are the same. This would still require us to be very considerate about using range as a dependency rather than range.start, range.end
Refresh from inside the useFetcher hook when the refresh button is clicked. This would be an easier change and almost all of our data calls (at least our GETs) are in a useFetcher hook. Downside being that we'd need to manually refresh for our calls outside a useFetcher() hook and it's implicit rather than explicit.

x-pack/plugins/apm/public/context/url_params_context/url_params_context.tsx

smith · 2021-02-02T01:15:39Z

retest

kibanamachine · 2021-02-02T02:59:46Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`apm`	5.2MB	5.2MB	+558.0B

History

💔 Build #102834 failed 5367efa
💚 Build #101754 succeeded 7af8a39
💔 Build #101679 failed f7394c6
💚 Build #101289 succeeded 6b448d2
💚 Build #101247 succeeded 11dec6e

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

sorenlouv · 2021-02-02T11:32:23Z

x-pack/plugins/apm/public/context/url_params_context/url_params_context.tsx

@@ -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);


We already have a setCounter method in useFetcher that's invoked when calling refetch(). Do we need both or can we somehow combine them?

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?

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.

dgieselaar · 2021-02-02T13:57:07Z

x-pack/plugins/apm/public/hooks/use_fetcher.tsx

@@ -136,6 +138,7 @@ export function useFetcher<TReturn>(
  }, [
    counter,
    preservePreviousData,
+    rangeId,


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.

dgieselaar

Looks good to me, but probably good to have a look at Søren's feedback before merging.

sorenlouv

lgtm. Just one comment on setCounter and setRangeId but nothing blocking

@timestamp

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.

@timestamp

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) ...

smith added release_note:enhancement v8.0.0 v7.12.0 labels Jan 21, 2021

smith requested a review from a team as a code owner January 21, 2021 21:27

botelastic bot added the Team:APM All issues that need APM UI Team support label Jan 21, 2021

dgieselaar reviewed Jan 22, 2021

View reviewed changes

x-pack/plugins/apm/public/context/url_params_context/helpers.ts Outdated Show resolved Hide resolved

smith added 2 commits January 25, 2021 15:12

fixes

0c6021d

Merge remote-tracking branch 'upstream/master' into nls/date-round

3a9635d

smith requested a review from dgieselaar January 25, 2021 21:17

dgieselaar reviewed Jan 26, 2021

View reviewed changes

x-pack/plugins/apm/public/context/url_params_context/url_params_context.test.tsx Outdated Show resolved Hide resolved

sorenlouv reviewed Jan 26, 2021

View reviewed changes

x-pack/plugins/apm/public/context/url_params_context/helpers.test.ts Show resolved Hide resolved

sorenlouv reviewed Jan 26, 2021

View reviewed changes

x-pack/plugins/apm/public/context/url_params_context/helpers.ts Show resolved Hide resolved

smith added 2 commits January 26, 2021 14:07

test fix

ba1a4b7

Merge remote-tracking branch 'upstream/master' into nls/date-round

0c8556f

add test

11dec6e

only round lower bound, fix tests

6b448d2

smith changed the title ~~Round start and end values~~ Round start values Jan 26, 2021

smith requested review from sorenlouv and dgieselaar January 26, 2021 22:29

range id and more tests

f7394c6

smith changed the title ~~Round start values~~ Round start and end values Jan 28, 2021

smith added 2 commits January 28, 2021 08:36

fix test

f10a3fa

Merge remote-tracking branch 'upstream/master' into nls/date-round

7af8a39

dgieselaar reviewed Jan 29, 2021

View reviewed changes

x-pack/plugins/apm/public/context/url_params_context/url_params_context.tsx Outdated Show resolved Hide resolved

smith added 3 commits February 1, 2021 15:22

Merge remote-tracking branch 'upstream/master' into nls/date-round

874f943

Don't expose increment

378c08f

Merge remote-tracking branch 'upstream/master' into nls/date-round

5367efa

sorenlouv reviewed Feb 2, 2021

View reviewed changes

dgieselaar reviewed Feb 2, 2021

View reviewed changes

dgieselaar approved these changes Feb 2, 2021

View reviewed changes

sorenlouv approved these changes Feb 2, 2021

View reviewed changes

smith merged commit f317316 into elastic:master Feb 2, 2021

smith mentioned this pull request Feb 2, 2021

[7.x] Round start and end values (#89030) #90035

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Round start and end values #89030

Round start and end values #89030

smith commented Jan 21, 2021 •

edited

Loading

elasticmachine commented Jan 21, 2021

smith commented Jan 22, 2021

smith commented Jan 25, 2021

dgieselaar commented Jan 26, 2021

dgieselaar commented Jan 26, 2021

smith commented Jan 26, 2021

smith commented Jan 26, 2021

dgieselaar commented Jan 26, 2021

smith commented Jan 26, 2021

sorenlouv commented Jan 27, 2021

dgieselaar commented Jan 27, 2021

smith commented Feb 2, 2021

kibanamachine commented Feb 2, 2021

sorenlouv Feb 2, 2021 •

edited

Loading

sorenlouv Feb 2, 2021

smith Feb 2, 2021

dgieselaar Feb 2, 2021

dgieselaar left a comment

sorenlouv left a comment

Round start and end values #89030

Round start and end values #89030

Conversation

smith commented Jan 21, 2021 • edited Loading

elasticmachine commented Jan 21, 2021

smith commented Jan 22, 2021

smith commented Jan 25, 2021

dgieselaar commented Jan 26, 2021

dgieselaar commented Jan 26, 2021

smith commented Jan 26, 2021

smith commented Jan 26, 2021

dgieselaar commented Jan 26, 2021

smith commented Jan 26, 2021

sorenlouv commented Jan 27, 2021

dgieselaar commented Jan 27, 2021

smith commented Feb 2, 2021

kibanamachine commented Feb 2, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

sorenlouv Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

sorenlouv Feb 2, 2021

Choose a reason for hiding this comment

smith Feb 2, 2021

Choose a reason for hiding this comment

dgieselaar Feb 2, 2021

Choose a reason for hiding this comment

dgieselaar left a comment

Choose a reason for hiding this comment

sorenlouv left a comment

Choose a reason for hiding this comment

smith commented Jan 21, 2021 •

edited

Loading

sorenlouv Feb 2, 2021 •

edited

Loading