Remove comparisonStart and comparisonEnd from API endpoints #127812

gbamparop · 2022-03-15T18:47:03Z

Summary

Replace comparisonStart and comparisonEnd with offset in all the API endpoints
Replace comparisonType url param with offset so the helper is no longer used

Closes #127307

gbamparop · 2022-03-16T18:34:52Z

Buildkite, test this

gbamparop · 2022-03-16T18:36:21Z

@elasticmachine merge upstream

x-pack/plugins/apm/public/components/routing/home/index.tsx

ogupte · 2022-03-17T19:30:00Z

I wonder if it makes sense to remove comparisonEnabled as well...It seems like we could just assume that if offset has a value, then comparisons are enabled. What do you think?

gbamparop · 2022-03-18T14:33:46Z

I wonder if it makes sense to remove comparisonEnabled as well...It seems like we could just assume that if offset has a value, then comparisons are enabled. What do you think?

@ogupte That's a good point, we've discussed it with @sqren before. In this case we'll need to store the selected option somewhere. I could do it with local state but we'll lose the previously selected option when navigating away e.g.

Select an option (other than default) from the comparison select
Disable comparison
Navigate to a page that doesn't have the comparison select
Navigate to a page that has the comparison select
Comparison is still disabled but the default option is selected

Created #128146

elasticmachine · 2022-03-21T11:55:27Z

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

gbamparop · 2022-03-21T12:02:14Z

@elasticmachine merge upstream

x-pack/plugins/apm/public/components/app/service_inventory/service_inventory.stories.tsx

...k/plugins/apm/public/components/app/service_overview/service_overview_errors_table/index.tsx

dgieselaar

I think this is a great change! Makes things simpler. I see that we (probably me) implemented offset as "time difference moving backwards", but I think it should be the other way around, e.g. 1d should be one day in the future, and -1d should be one day into the past. What do you think? I haven't reviewed all of it, would like to wait until we have figured that question out, and the redirects.

gbamparop · 2022-03-22T10:15:53Z

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

+      rangeTo: rangeTo as string,
+    });
+
+    const momentStart = moment(start);


Instead of getting rangeFrom / rangeTo and calculating the diff I can just map TimeRangeComparisonEnum.DayBefore to 1d and TimeRangeComparisonEnum.WeekBefore to 1w. Then the value is PeriodBefore we don't have any other options which means even without redirects we will still get the default value. @dgieselaar WDYT?

sgtm, although I'm not sure how that helps with redirect - if users have bookmarks or whatever it will still include the literal string no? am I missing something?

It would still calculate the right offset because it'll be the default option, but comparisonType=period will remain in the url. I only mentioned it as it'll simplified the redirect a bit. I'm find to leave it as is if you think it looks ok.

oh I see now I think, I assumed you were talking about changing the value of the enum. You're talking about mapping it here? would that also remove the need to call getSelectOptions below?

Yes, it would be removed. It'll be like if (comparisonType === 'day') offset = '1d') (same for the week). For period we just leave it continue and the time comparison component will calculate the default.

Done in de4d405

gbamparop · 2022-03-22T10:46:07Z

I think it should be the other way around, e.g. 1d should be one day in the future, and -1d should be one day into the past. What do you think?

That could be an improvement, although I don't necessarily think there's a big issue with what we have right now. Do you think that's something we could raise and address in a follow up?

dgieselaar · 2022-03-22T14:52:25Z

That could be an improvement, although I don't necessarily think there's a big issue with what we have right now. Do you think that's something we could raise and address in a follow up?

Sounds good to me, I don't think it's that much of an issue and there's some work involved flipping all the offsets around. No need to make an issue AFAIC either.

dgieselaar · 2022-03-23T10:53:25Z

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

+    return (
+      route.path === '/services' ||
+      route.path === '/traces' ||
+      route.path === '/service-map' ||
+      route.path === '/backends' ||
+      route.path === '/services/{serviceName}' ||
+      route.path === '/service-groups' ||
+      location.pathname === '/' ||
+      location.pathname === ''
+    );


Maybe we should extract this into a function, something like isRouteWithTimeRange()? I think we're using it somewhere else too. I'm not a fan (I can say that because I wrote it) but I don't really have a better way of doing this right now.

Good shout, indeed it's being used here as well. I can extract it to a function.

Added in 9337e5d

dgieselaar · 2022-03-23T10:57:45Z

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

+    const momentStart = moment(start);
+    const momentEnd = moment(end);
+
+    const offset = getSelectOptions({


nit: perhaps the relevant part should be abstracted out of the getSelectOptions function, calling this here seems kind of odd.

Discussed another option in #127812 (comment)

dgieselaar · 2022-03-24T08:34:36Z

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

@@ -41,13 +41,13 @@ export function TimeComparison() {
  const { start, end } = useTimeRange({ rangeFrom, rangeTo });

  const {
-    urlParams: { comparisonEnabled, comparisonType },
+    urlParams: { comparisonEnabled, offset },


any reason why we're still getting this from useLegacyUrlParams()?

We've discussed this previously here #126567 (comment). Basically if we get it from apm params it'll be false the first time instead of undefined and we wouldn't know if it was actually in the url or we should query the advanced settings. Will the redirect you've suggested overcome this issue?

Are you referring to comparisonEnabled or offset? I was referring to the latter. I'm hoping we can make comparisonEnabled required and redirect in RedirectWithOffset when taking the value of the Kibana Advanced Setting into account - would that make sense?

I was referring to comparisonEnabled. Removed the call to useLegacyUrlParams, made comparisonEnabled required and added redirect in f0c8d72.

dgieselaar · 2022-03-24T08:35:36Z

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

+  if (comparisonEnabled === undefined || offset === undefined) {
    urlHelpers.replace(history, {


do we still need this redirect?

Removed it in f0c8d72 as we now redirect from RedirectWithOffset

dgieselaar · 2022-03-25T10:22:01Z

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

+
+  if (
+    matchesRoute &&
+    ('comparisonType' in query || !('comparisonEnabled' in query))


this is a little hard for me to parse, do you mind leaving a comment why we're redirecting in this scenario? (IIRC, we redirect if comparisonType is set, because we no longer support it, and we redirect if comparisonEnabled is not set, because it is now required)

dgieselaar · 2022-03-25T10:22:47Z

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

+          url: location.pathname,
+          query: {
+            comparisonEnabled,
+            offset: dayAndWeekBeforeToOffsetMap[comparisonTypeEnumValue] ?? '',


what happens if we set offset to an empty string? is offset optional? (if it is required I don't think we should set it to an empty string)

Offset is optional, we set it to empty string if comparisonType is not day or week. TimeComparison component then switches to the default offset for the current time range.

ok, should we maybe remove it then from the query entirely? either way this looks fine to me then.

Updated so we add it to the query only if it's day or week.

dgieselaar · 2022-03-25T10:23:31Z

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

 }

+export const dayAndWeekBeforeToOffsetMap = {


maybe a little verbose? just a nit though.

dgieselaar

Thanks! I think this cleans up nicely. I left a couple of comments, perhaps the one about offset being an empty string requires a good look, but looks good other than that.

…son-date-improvements

kibana-ci · 2022-03-28T13:30:56Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`apm`	1182	1185	+3

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.9MB	2.9MB	-1.7KB

History

💔 Build #33913 failed c0276e6
💔 Build #33588 failed f0c8d72
💚 Build #33449 succeeded de4d405
💚 Build #32539 succeeded 6adb314
💚 Build #32135 succeeded 66bbdc0

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

gbamparop added Team:APM All issues that need APM UI Team support release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.2.0 labels Mar 15, 2022

gbamparop force-pushed the comparison-date-improvements branch from 26f4c25 to a585624 Compare March 16, 2022 09:14

gbamparop force-pushed the comparison-date-improvements branch 2 times, most recently from 682a2c8 to 80f5d27 Compare March 17, 2022 16:26

gbamparop commented Mar 17, 2022

View reviewed changes

x-pack/plugins/apm/public/components/routing/home/index.tsx Show resolved Hide resolved

gbamparop marked this pull request as ready for review March 21, 2022 11:55

gbamparop requested a review from a team as a code owner March 21, 2022 11:55

gbamparop requested a review from dgieselaar March 21, 2022 11:55

gbamparop mentioned this pull request Mar 21, 2022

[APM] Investigate whether we can remove comparisonEnabled from url #128146

Closed

dgieselaar reviewed Mar 21, 2022

View reviewed changes

x-pack/plugins/apm/public/components/app/service_inventory/service_inventory.stories.tsx Show resolved Hide resolved

dgieselaar reviewed Mar 21, 2022

View reviewed changes

...k/plugins/apm/public/components/app/service_overview/service_overview_errors_table/index.tsx Show resolved Hide resolved

dgieselaar reviewed Mar 21, 2022

View reviewed changes

gbamparop commented Mar 22, 2022

View reviewed changes

dgieselaar reviewed Mar 23, 2022

View reviewed changes

dgieselaar reviewed Mar 24, 2022

View reviewed changes

gbamparop added 2 commits March 24, 2022 11:30

Use offset instead of comparisonStart / comparisonEnd

44b9aa4

Fix API tests

e659a89

gbamparop added 4 commits March 24, 2022 11:30

Fix bucket size and remove comparisonStart / comparisonEnd

09623c4

Replace comparisonType url param with offset

0d13a94

Add redirect for comparisonType

a5f8a3f

Create isRouteWithTimeRange helper function

9337e5d

gbamparop force-pushed the comparison-date-improvements branch from 6adb314 to 9337e5d Compare March 24, 2022 11:59

gbamparop added 2 commits March 24, 2022 12:29

Remove getSelectOptions call from redirect

de4d405

Add redirect for comparisonEnabled

f0c8d72

dgieselaar reviewed Mar 25, 2022

View reviewed changes

dgieselaar approved these changes Mar 25, 2022

View reviewed changes

gbamparop added 3 commits March 28, 2022 10:55

Merge branch 'main' of https://github.com/elastic/kibana into compari…

a0fa40d

…son-date-improvements

Fix build errors and address feedback

c0276e6

Add offset param in redirect only if comparisonType is day or week

80344a7

gbamparop merged commit c059ead into elastic:main Mar 28, 2022

gbamparop added apm:test-plan-done Pull request that was successfully tested during the test plan and removed apm:test-plan-done Pull request that was successfully tested during the test plan labels Apr 4, 2022

gbamparop mentioned this pull request Apr 6, 2022

[APM] Fix broken e2e tests #129475

Merged

gbamparop deleted the comparison-date-improvements branch April 12, 2022 09:44

		if (comparisonEnabled === undefined \|\| offset === undefined) {
		urlHelpers.replace(history, {

Remove comparisonStart and comparisonEnd from API endpoints #127812

Remove comparisonStart and comparisonEnd from API endpoints #127812

Conversation

gbamparop commented Mar 15, 2022 • edited Loading

Summary

gbamparop commented Mar 16, 2022

gbamparop commented Mar 16, 2022

ogupte commented Mar 17, 2022

gbamparop commented Mar 18, 2022 • edited Loading

elasticmachine commented Mar 21, 2022

gbamparop commented Mar 21, 2022

dgieselaar left a comment

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

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 22, 2022

dgieselaar commented Mar 22, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgieselaar Mar 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbamparop Mar 28, 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

dgieselaar left a comment

Choose a reason for hiding this comment

kibana-ci commented Mar 28, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

gbamparop commented Mar 15, 2022 •

edited

Loading

gbamparop commented Mar 18, 2022 •

edited

Loading

dgieselaar Mar 24, 2022 •

edited

Loading

gbamparop Mar 28, 2022 •

edited

Loading