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

gbamparop · 2022-03-01T11:03:49Z

Summary

Fix bugs around the comparison feature. The following comparison options are now returned:

Day before and Week before if the time difference between the end and start dates is less than 25 hours. This is because relative dates may be rounded when selecting a single day which can result in a duration > 24h. (e.g. Last 24 hours results in rangeFrom: now-24h/h, rangeTo: now by default)
Week before if the time difference between the end and start dates is less than 8 days. This is because relative dates may be rounded when selecting a single week which can result in a duration > 7d. (e.g. Last 7 days results in rangeFrom: now-7d/d, rangeTo: now by default)
Period before when the difference between the end and start dates is equal or greater than 8 days. This difference will be equal with the difference between the comparison end date and comparison start date.

Changes:

Remove exactStart and exactEnd
Remove the rounding of the start date
Replace calls of getTimeRangeComparison with useComparison hook
Add more tests for comparison
Get comparisonType, comparisonEnabled and latencyAggregationType with useApmParams instead of useLegacyUrlParams in most places
Add default value for latencyAggregationType in the route

Closes #115247

elasticmachine · 2022-03-01T11:03:51Z

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

cauemarcondes · 2022-03-01T15:27:48Z

@gbamparop like @sqren has already mentioned, this part has already been changed a lot of times. Do you mind taking a look at my PR and see if this is an exception rule on what was implemented there? #101423

gbamparop · 2022-03-01T17:53:44Z

@gbamparop like @sqren has already mentioned, this part has already been changed a lot of times. Do you mind taking a look at my PR and see if this is an exception rule on what was implemented there? #101423

Thanks @cauemarcondes, after the discussion I will look into adding some more information and tests in the PR.

sorenlouv · 2022-03-01T23:16:55Z

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

    ['s', 'm', 'h', 'd', 'w'].map((roundingOption) =>
-      it(`removes /${roundingOption} rounding option from relative time`, () => {
+      it(`doesn't remove /${roundingOption} rounding option when time is not added or subtracted`, () => {
        const spy = jest.spyOn(datemath, 'parse');
        helpers.getExactDate(`now/${roundingOption}`);
-        expect(spy).toHaveBeenCalledWith('now', {});
+        expect(spy).toHaveBeenCalledWith(`now/${roundingOption}`, {});
      })


Please double check that this is the behaviour we want. And would be great with more test coverage

gbamparop · 2022-03-09T15:49:31Z

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

-    rangeFrom,
-    rangeTo,
-  });
+  const { start, end } = useTimeRange({ rangeFrom, rangeTo });

  const {
    urlParams: { comparisonEnabled, comparisonType },


If we get comparisonEnabled from useAnyOfApmParams it defaults to false instead of undefined because of the toBooleanRt type. This means that getComparisonEnabled will return false and not get the value from the advanced settings the first time. We could have a look at this when useLegacyUrlParams is removed.

You can also add this in a redirect component. The ergonomics are not great, we basically run into the issue that routing is static/sync, but some default values need to be fetched asynchronously. I don't really have a great solution for that right now.

sorenlouv

I haven't tested this out but code looks good. If you can do some manual testing I think it's good to go. I want to get it merged well before FF to get as many eyes as possible on it in case we've missed something

sorenlouv · 2022-03-10T16:04:24Z

x-pack/plugins/apm/public/components/shared/time_comparison/get_comparison_options.ts

+  start,
+  end,
+}: {
+  comparisonTypes: TimeRangeComparisonEnum[];


nit: shouldn't this be called TimeRangeComparisonType or TimeRangeComparisonTypeEnum?

Suggested change

comparisonTypes: TimeRangeComparisonEnum[];

comparisonTypes: TimeRangeComparisonTypeEnum[];

Good point, I'll get it changed

sorenlouv · 2022-03-10T16:07:31Z

x-pack/plugins/apm/public/hooks/use_time_range.ts

  refreshTimeRange: () => void;
  timeRangeId: number;
 }

 type PartialTimeRange = Pick<TimeRange, 'refreshTimeRange' | 'timeRangeId'> &
-  Pick<Partial<TimeRange>, 'start' | 'end' | 'exactStart' | 'exactEnd'>;


Thanks for removing this exactStart and exactEnd!

sorenlouv · 2022-03-10T16:09:58Z

x-pack/plugins/apm/public/components/shared/time_comparison/comparison.test.ts

+  describe('When "Today" is selected', () => {
+    let expectation: ReturnType<typeof getExpectedTimesAndComparisons>;
+
+    beforeAll(() => {
+      expectation = getExpectedTimesAndComparisons({
+        rangeFrom: 'now/d',
+        rangeTo: 'now/d',
+      });
+    });
+
+    it('should return the correct start and end date', () => {
+      expect(expectation.start).toBe('2022-01-14T00:00:00.000Z');
+      expect(expectation.end).toBe('2022-01-14T23:59:59.999Z');
+    });
+
+    it('should return comparison by day and week', () => {
+      expect(expectation.comparisons).toEqual([
+        {
+          value: TimeRangeComparisonEnum.DayBefore,
+          text: 'Day before',
+          comparisonStart: '2022-01-13T00:00:00.000Z',
+          comparisonEnd: '2022-01-13T23:59:59.999Z',
+          offset: '1d',
+        },
+        {
+          value: TimeRangeComparisonEnum.WeekBefore,
+          text: 'Week before',
+          comparisonStart: '2022-01-07T00:00:00.000Z',
+          comparisonEnd: '2022-01-07T23:59:59.999Z',
+          offset: '1w',
+        },
+      ]);
+    });
+  });


I really like these tests!

gbamparop · 2022-03-10T16:20:08Z

I haven't tested this out but code looks good. If you can do some manual testing I think it's good to go. I want to get it merged well before FF to get as many eyes as possible on it in case we've missed something

Sure, I'll do some more thorough manual testing and merge it in if all looks good.

dgieselaar · 2022-03-10T16:21:02Z

...s/apm/public/components/app/backend_inventory/backend_inventory_dependencies_table/index.tsx

-    comparisonEnabled,
-    comparisonType,
-  });
+  const { offset } = useComparison();


I strongly recommend using getTimeRangeComparison here still. useComparison (without arguments) breaks any type safety that we have from the route params.

Can you please explain further? It seems that rangeFrom, rangeTo, comparisonEnabled and comparisonType are retrieved in a similar way from useComparison

I think the problem @dgieselaar is mentioning, is that start, end, comparisonEnabled, comparisonType used to come from the type safe useApmParams('/backends') whereas in useComparison only start and end are type safe (they are retrieved via const { query } = useApmParams('/*')).

I hope it's possible to retrieve comparisonEnabled and comparisonType from useApmParams('/*') although they might be optional in this case, meaning we'll have to have a runtime check for them, which I think is fine.

tldr: I hope we can keep const { offset } = useComparison(); as is, but change the implementation of useComparison slightly.

Giorgos and I had a call to talk about it already, he would look into using getTimeRangeComparison where possible. IMHO we should avoid depending on runtime checks, because it means more (automated or manual) tests will be necessary - it won't be the first time we've forgot about requiring certain parameters on a route. rangeFrom/rangeTo are pretty widely available, but I think these principles should be applied consistently where possible.

As discussed with @dgieselaar, useComparison hook was removed in 367fa6b. I've suggested to pass the route in the hook but we would end up handling it differently than useTimeRange.

…/ comparisonEnd

kibana-ci · 2022-03-14T17:32:56Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`apm`	1150	1149	-1

Async chunks

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

id	before	after	diff
`apm`	2.8MB	2.8MB	-507.0B

History

💔 Build #30019 failed 367fa6b
💔 Build #28893 failed 5e9f3c5
💚 Build #28500 succeeded af251d2
💚 Build #26766 succeeded 52b94b5

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

…lastic#126567) * Remove exactStart and exactEnd * Add tests for comparison handling * Remove useComparison hook

gbamparop added Team:APM All issues that need APM UI Team support release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v8.2.0 labels Mar 1, 2022

gbamparop requested a review from a team as a code owner March 1, 2022 11:03

gbamparop marked this pull request as draft March 1, 2022 17:52

sorenlouv reviewed Mar 1, 2022

View reviewed changes

gbamparop force-pushed the update-time-comparison branch from 52b94b5 to af251d2 Compare March 8, 2022 17:57

gbamparop commented Mar 9, 2022

View reviewed changes

gbamparop marked this pull request as ready for review March 9, 2022 16:15

gbamparop requested review from a team and sorenlouv March 9, 2022 16:15

sorenlouv approved these changes Mar 10, 2022

View reviewed changes

dgieselaar reviewed Mar 10, 2022

View reviewed changes

gbamparop added 6 commits March 14, 2022 15:05

Fix getExactDate and remove exactEnd

4c88961

Update test description

ad2ac4a

Update comparison date handling

c40c887

Update tests

cef8b3f

Add tests for the time difference of start / end and comparisonStart …

ad01aeb

…/ comparisonEnd

Remove useComparison hook

367fa6b

gbamparop force-pushed the update-time-comparison branch from 66db590 to 367fa6b Compare March 14, 2022 15:14

gbamparop added the ci:deploy-cloud label Mar 14, 2022

Add missing url param

2b77de2

gbamparop added backport:skip This commit does not require backporting and removed auto-backport Deprecated - use backport:version if exact versions are needed labels Mar 14, 2022

gbamparop merged commit feb6416 into elastic:main Mar 14, 2022

gbamparop mentioned this pull request Mar 24, 2022

Remove comparisonStart and comparisonEnd from API endpoints #127812

Merged

tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gbamparop commented Mar 1, 2022 •

edited

Loading

elasticmachine commented Mar 1, 2022

cauemarcondes commented Mar 1, 2022

gbamparop commented Mar 1, 2022

sorenlouv Mar 1, 2022

gbamparop Mar 9, 2022 •

edited

Loading

dgieselaar Mar 10, 2022

sorenlouv left a comment •

edited

Loading

sorenlouv Mar 10, 2022 •

edited

Loading

gbamparop Mar 10, 2022

sorenlouv Mar 10, 2022

sorenlouv Mar 10, 2022

gbamparop commented Mar 10, 2022

dgieselaar Mar 10, 2022

gbamparop Mar 10, 2022

sorenlouv Mar 14, 2022 •

edited

Loading

dgieselaar Mar 14, 2022

gbamparop Mar 14, 2022

kibana-ci commented Mar 14, 2022

	comparisonTypes: TimeRangeComparisonEnum[];
	comparisonTypes: TimeRangeComparisonTypeEnum[];

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

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

Conversation

gbamparop commented Mar 1, 2022 • edited Loading

Summary

elasticmachine commented Mar 1, 2022

cauemarcondes commented Mar 1, 2022

gbamparop commented Mar 1, 2022

Choose a reason for hiding this comment

gbamparop Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv left a comment • edited Loading

Choose a reason for hiding this comment

sorenlouv Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbamparop commented Mar 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Mar 14, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

gbamparop commented Mar 1, 2022 •

edited

Loading

gbamparop Mar 9, 2022 •

edited

Loading

sorenlouv left a comment •

edited

Loading

sorenlouv Mar 10, 2022 •

edited

Loading

sorenlouv Mar 14, 2022 •

edited

Loading