[APM] Fixing time comparison types #101423

cauemarcondes · 2021-06-04T19:00:13Z

Before:

After:

Problem:

When Last 24 hours is selected
This is the how the url looks like apm/services?rangeFrom=now-24h/h&rangeTo=now
And this is the conversion from relative to absolute time start: "2021-06-02T16:00:00.000Z", end: "2021-06-03T16:02:52.650Z"
When I get the difference in DAYS between the start and end I get 1.001998263888889
The result is more than one day, even though last 24 hours is selected.
What causes this? The extra /h on rangeFrom=now-24h/h rounds the start date.
If we remove the extra /h this is the result of the conversion: start: "2021-06-02T16:07:23.280Z", end: "2021-06-03T16:07:23.280Z" now we have exactly 1 day.
The same thing happens when Last 7 days is selected, because of the extra /d we the difference is always bigger than 7 days.

Solution

The right way to calculate the difference between the start and end dates is to use both absolute values instead of the rounded ones. So a new property was added inside the routeParams to represent the exact start and end dates, and these values are used to get comparison types available withing the time raange

elasticmachine · 2021-06-04T19:00:17Z

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

cauemarcondes · 2021-06-07T12:25:54Z

@elasticmachine merge upstream

dgieselaar · 2021-06-08T07:38:45Z

@cauemarcondes I'm having a little trouble understanding the changes 😄 do you mind helping me out by writing down some notes about why certain changes were necessary? Also happy to zoom about it later today.

cauemarcondes · 2021-06-08T13:04:18Z

@cauemarcondes I'm having a little trouble understanding the changes 😄 do you mind helping me out by writing down some notes about why certain changes were necessary? Also happy to zoom about it later today.

Sorry, @dgieselaar I added more details in the description.

dgieselaar · 2021-06-09T06:57:38Z

I wonder whether we are making it too hard on ourselves here by trying to make decisions upfront about what would be valuable to the user. Can we simplify this by allowing the user to pick any offset, regardless of the currently selected time range? cc @sqren @formgeist

sorenlouv · 2021-06-09T12:02:56Z

Can we simplify this by allowing the user to pick any offset, regardless of the currently selected time range?

Are you proposing to have a secondary date picker, where the user can select an arbitrary start date for the comparison range?

I'm a bit on the fence on this one. The advantages is that it would reduce our (code) logic, and allow the user to make any kind of comparisons they want.

At the same time I think reducing the users choice to simply a list of comparison options (instead of selecting a date in a date picker) is better UX. Especially if the user is looking at "Today", it's handy to allow them to compare with "Yesterday" and "Last week" with as few clicks (and as little cognitive load) as possible.

WDYT @formgeist?

formgeist · 2021-06-09T12:12:11Z

I agree that we have these specific options in place to avoid cognitive load for the user. We did discuss at one point to allow for offset intervals such it would always use the same interval as selected but you could essentially choose whether it was +1 or +4 weeks. I think this came up in a discuss comment on our feedback request. I think we should open this up for discussion and potential iteration but I wouldn't consider this a prerequisite before merging this PR that essentially makes our logic more consistent.

dgieselaar · 2021-06-09T12:31:21Z

Are you proposing to have a secondary date picker, where the user can select an arbitrary start date for the comparison range?

No, just to not filter the predefined options. So just always show "day before", "week before", "period before".

sorenlouv · 2021-06-09T12:58:49Z

No, just to not filter the predefined options. So just always show "day before", "week before", "period before".

If a user is viewing the time range 14th to 21st of May, and selects "week before", the comparison should be against 7th to 14th of May.

I don't understand what "day before" should compare against

(calendar to reduce mental gymnastics)

dgieselaar · 2021-06-09T13:40:05Z

@sqren it's a 1 day offset. so if you select last 7 days, you'd be comparing it against last 8 days - last day. It feels somewhat useless, but IMHO there's some complexity involved here in trying to make the displayed options super relevant, and I'm not sure if we've solved all edge cases now.

cauemarcondes · 2021-06-09T18:55:18Z

@elasticmachine merge upstream

sorenlouv · 2021-06-10T08:03:39Z

I think we should open this up for discussion and potential iteration but I wouldn't consider this a prerequisite before merging this PR that essentially makes our logic more consistent.

Agree, let's keep what we have now but open a separate discussion about how this can be improved.

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

sorenlouv · 2021-06-10T08:23:33Z

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

+        .mockReturnValue(moment('2021-06-02T17:00:00.000Z').utc());
+      expect(helpers.getExactDate('now/d')?.toISOString()).toEqual(
+        '2021-06-02T17:00:00.000Z'


How does this test verify that the date is not being rounded when the original date is already rounded.

yeah, it actually doesn't make much sense to expect for a specific date. I'd rather spy the datemath.parse and check if it was called correctly.

sorenlouv · 2021-06-10T08:26:42Z

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

+      jest
+        .spyOn(datemath, 'parse')
+        .mockReturnValue(moment('2021-06-02T17:56:49.260Z').utc());
+      expect(helpers.getExactDate('now-24h/h')?.toISOString()).toEqual(
+        '2021-06-02T17:56:49.260Z'
+      );


You should also call datemath.parse('now-24h/h') directly to show that the default behaviour is to round.

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

x-pack/plugins/apm/public/components/shared/time_comparison/index.tsx

sorenlouv

Remove isRangeToNow should be enough to solve the problem. Still, the refactoring and added tests should stay.

…comparison

cauemarcondes · 2021-06-14T15:47:28Z

Remove isRangeToNow should be enough to solve the problem. Still, the refactoring and added tests should stay.

@sqren are you sure? this is what I got by only removing the isRangeToNow and selecting last 24hours:

Both Day before and Week before should be available, but instead only Week before is. That's because the difference is 1.031450949074074.

sorenlouv · 2021-06-14T21:40:17Z

@sqren are you sure? this is what I got by only removing the isRangeToNow and selecting last 24hours:

This is what I see:

That's because the difference is 1.031450949074074.

How do you get that number? According to my logic it should be 24 hours - 1 millisecond = 0.999999988 days (thanks Google)

This is also what I'm seeing when logging the dateDiff:

cauemarcondes · 2021-06-15T13:10:42Z

@sqren are you sure? this is what I got by only removing the isRangeToNow and selecting last 24hours:

This is what I see:

That's because the difference is 1.031450949074074.

How do you get that number? According to my logic it should be 24 hours - 1 millisecond = 0.999999988 days (thanks Google)

This is also what I'm seeing when logging the dateDiff:

And what do you see when you select Last 7 days?

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

cauemarcondes · 2021-06-16T14:32:31Z

jenkins, retest this please

cauemarcondes · 2021-06-16T16:21:03Z

@elasticmachine merge upstream

kibanamachine · 2021-06-16T18:24:14Z

💚 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`	4.3MB	4.3MB	+686.0B

History

💔 Build #131704 failed c30cfdf
💔 Build #131681 failed c30cfdf
💚 Build #131000 succeeded 74b9619
💔 Build #130959 failed 2f3442d
💚 Build #130271 succeeded a81cb9f

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

* fixing time comparison types * fixing ts issues * addressing PR comments * addressing PR comments * addressing PR comments Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-06-16T18:34:48Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

* fixing time comparison types * fixing ts issues * addressing PR comments * addressing PR comments * addressing PR comments Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Cauê Marcondes <55978943+cauemarcondes@users.noreply.github.com>

…egrations-to-global-search * 'master' of github.com:elastic/kibana: (46 commits) [Lens] Add some more documentation for dynamic coloring (elastic#101369) hide not searchable results when no term (elastic#102401) [Lens] Fix Formula functional test with multiple suggestions (elastic#102378) Fix trusted apps modified by field displayed as a date field (elastic#102377) [Lens] Docs for time shift (elastic#102048) update readme of logs-metrics-ui (elastic#101968) Refactor observability plugin breadcrumbs (elastic#102290) [Index Patterns] Move rollup config to index pattern management v2 (elastic#102285) [Security Solution][Endpoint] Isolate Action should only be available to Platinum+ licenses (elastic#102374) [build] Updates Ironbank templates (elastic#102407) Update security best practices document (elastic#100814) [Enterprise Search] Set up initial KibanaPageTemplate (elastic#102170) [Reporting/Docs] Add section to troubleshooting guide to explain the StatusCodeError logs (elastic#102278) [DOCS] Updating Elastic Security Overview topic (elastic#101922) [Uptime] refactor Synthetics Integration package UI (elastic#102080) [Task Manager] Log at different levels based on the state (elastic#101751) [APM] Fixing time comparison types (elastic#101423) [RAC] Update alert documents in lifecycle rule type helper (elastic#101598) [ML] Functional tests - fix and re-activate alerting flyout test (elastic#102368) [Reporting] remove unused reference to path.data config (elastic#102267) ... # Conflicts: # x-pack/plugins/fleet/kibana.json

smith · 2021-07-16T21:33:27Z

Tests OK. Comparisons working as expected.

fixing time comparison types

606475b

cauemarcondes added release_note:fix v7.14.0 apm:comparison labels Jun 4, 2021

cauemarcondes requested a review from a team as a code owner June 4, 2021 19:00

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

fixing ts issues

f33f4f4

Merge branch 'master' into apm-fix-time-comparison

baf0425

Merge branch 'master' into apm-fix-time-comparison

a81cb9f

sorenlouv reviewed Jun 10, 2021

View reviewed changes

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

sorenlouv reviewed Jun 10, 2021

View reviewed changes

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

sorenlouv reviewed Jun 10, 2021

View reviewed changes

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

addressing PR comments

2f3442d

sorenlouv reviewed Jun 14, 2021

View reviewed changes

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

sorenlouv reviewed Jun 14, 2021

View reviewed changes

x-pack/plugins/apm/public/components/shared/time_comparison/index.tsx Outdated Show resolved Hide resolved

sorenlouv requested changes Jun 14, 2021

View reviewed changes

cauemarcondes added 2 commits June 14, 2021 11:25

addressing PR comments

e923195

Merge branch 'master' of github.com:elastic/kibana into apm-fix-time-…

74b9619

…comparison

sorenlouv reviewed Jun 15, 2021

View reviewed changes

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

sorenlouv reviewed Jun 15, 2021

View reviewed changes

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

sorenlouv reviewed Jun 15, 2021

View reviewed changes

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

sorenlouv approved these changes Jun 16, 2021

View reviewed changes

addressing PR comments

c30cfdf

Merge branch 'master' into apm-fix-time-comparison

74ee2d1

cauemarcondes added the auto-backport Deprecated - use backport:version if exact versions are needed label Jun 16, 2021

cauemarcondes merged commit adc95c1 into elastic:master Jun 16, 2021

cauemarcondes deleted the apm-fix-time-comparison branch June 16, 2021 18:32

kibanamachine mentioned this pull request Jun 16, 2021

[7.x] [APM] Fixing time comparison types (#101423) #102399

Merged

sorenlouv added the apm:test-plan-7.14.0 label Jul 5, 2021

smith added the apm:test-plan-done Pull request that was successfully tested during the test plan label Jul 16, 2021

cauemarcondes mentioned this pull request Jul 21, 2021

APM Comparison date range displays incorrect values when Absolute Date Filter is applied #106097

Closed

gbamparop mentioned this pull request Feb 3, 2022

[APM] Comparison always shows day/week when relative time range is selected #115247

Closed

cauemarcondes mentioned this pull request Mar 1, 2022

[APM] Fix time comparison types when relative time range is selected #126567

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Fixing time comparison types #101423

[APM] Fixing time comparison types #101423

cauemarcondes commented Jun 4, 2021 •

edited

Loading

elasticmachine commented Jun 4, 2021

cauemarcondes commented Jun 7, 2021

dgieselaar commented Jun 8, 2021

cauemarcondes commented Jun 8, 2021

dgieselaar commented Jun 9, 2021

sorenlouv commented Jun 9, 2021 •

edited

Loading

formgeist commented Jun 9, 2021

dgieselaar commented Jun 9, 2021

sorenlouv commented Jun 9, 2021 •

edited

Loading

dgieselaar commented Jun 9, 2021

cauemarcondes commented Jun 9, 2021

sorenlouv commented Jun 10, 2021

sorenlouv Jun 10, 2021

cauemarcondes Jun 10, 2021

sorenlouv Jun 10, 2021

sorenlouv left a comment

cauemarcondes commented Jun 14, 2021

sorenlouv commented Jun 14, 2021 •

edited

Loading

cauemarcondes commented Jun 15, 2021

cauemarcondes commented Jun 16, 2021

cauemarcondes commented Jun 16, 2021

kibanamachine commented Jun 16, 2021

kibanamachine commented Jun 16, 2021

smith commented Jul 16, 2021

[APM] Fixing time comparison types #101423

[APM] Fixing time comparison types #101423

Conversation

cauemarcondes commented Jun 4, 2021 • edited Loading

Problem:

Solution

elasticmachine commented Jun 4, 2021

cauemarcondes commented Jun 7, 2021

dgieselaar commented Jun 8, 2021

cauemarcondes commented Jun 8, 2021

dgieselaar commented Jun 9, 2021

sorenlouv commented Jun 9, 2021 • edited Loading

formgeist commented Jun 9, 2021

dgieselaar commented Jun 9, 2021

sorenlouv commented Jun 9, 2021 • edited Loading

dgieselaar commented Jun 9, 2021

cauemarcondes commented Jun 9, 2021

sorenlouv commented Jun 10, 2021

sorenlouv Jun 10, 2021

Choose a reason for hiding this comment

cauemarcondes Jun 10, 2021

Choose a reason for hiding this comment

sorenlouv Jun 10, 2021

Choose a reason for hiding this comment

sorenlouv left a comment

Choose a reason for hiding this comment

cauemarcondes commented Jun 14, 2021

sorenlouv commented Jun 14, 2021 • edited Loading

cauemarcondes commented Jun 15, 2021

cauemarcondes commented Jun 16, 2021

cauemarcondes commented Jun 16, 2021

kibanamachine commented Jun 16, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

kibanamachine commented Jun 16, 2021

💚 Backport successful

smith commented Jul 16, 2021

cauemarcondes commented Jun 4, 2021 •

edited

Loading

sorenlouv commented Jun 9, 2021 •

edited

Loading

sorenlouv commented Jun 9, 2021 •

edited

Loading

sorenlouv commented Jun 14, 2021 •

edited

Loading