Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[APM] Enabling yesterday option when 24 hours is selected #90017

Merged
merged 5 commits into from
Feb 4, 2021
Merged

[APM] Enabling yesterday option when 24 hours is selected #90017

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
14d8cd4
enabling yesterday option when 24 hours is selected
cauemarcondes Feb 2, 2021
7619127
addressing PR comments
cauemarcondes Feb 3, 2021
f80a99d
addressing PR comments
cauemarcondes Feb 4, 2021
ed89e4b
Merge branch 'master' of github.com:elastic/kibana into apm-time-comp…
cauemarcondes Feb 4, 2021
b3362b4
enabling select box
cauemarcondes Feb 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions x-pack/plugins/apm/public/components/shared/time_comparison/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ const PrependContainer = styled.div`
padding: 0 ${px(unit)};
`;

const weekDurationInHours = moment.duration(7, 'days').asHours();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: There's always the vanilla option. Might also be the simplest to read:

Suggested change
const weekDurationInHours = moment.duration(7, 'days').asHours();
const weekDurationInHours = 24 * 7

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO we should consider using moment here, but scoped to the current time, to deal with issues like DST. E.g.:

const aDayAgo = momentEnd.subtract(1, 'days').getTime();
const isAtLeastADayAgo = momentStart.getTime() <= aDayAgo;
const aWeekAgo = momentEnd.subtract(1, 'week').getTime();
const isAtLeastAWeekAgo = momentStart.getTime() <= aWeekAgo;

(we should figure out how exactly moment.subtract() deals with DST ofc)

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll admit I'm not quite sure why we are using hours here, are we rounding?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, should this not be "a day before" and "a week before" (the current time range), rather than yesterday/a week ago?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, should this not be "a day before" and "a week before" (the current time range), rather than yesterday/a week ago?

Good point. The current copy is only applicable if the end range is now.
I think we could keep the labels if we add a check like:

const = end === Date.now();
if (dateDiffInHours <= 24 && isEndNow) {

We might have to add some padding like:

const isEndNow = end + 1000 >= Date.now();
if (dateDiffInHours <= 24 && isEndNow) {
  return [yesterdayOption, aWeekAgoOption];
}

If isEndNow is false we'll fall back to prevPeriodOption which might be ok.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, should this not be "a day before" and "a week before" (the current time range), rather than yesterday/a week ago?

Good catch! And I agree, the more clear we can be with the relative time the easier it is to parse for the user. So if we can add a check as @sqren proposes, I'm very happy 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

End is rounded.

That's why I thought we could add padding. But since rounding is up to 5 minutes we'd need a lot of padding so I agree, not a great solution.

But maybe we can use the query parameter? E.g. if rangeTo=='now'

Much better idea. Let's try to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll admit I'm not quite sure why we are using hours here, are we rounding?

@dgieselaar I switched to use hours because when I used days and the difference between the dates is for example 25 hours the days difference still returns 1. But I only want to show the yesterday option when the difference is less or equal to 24 hours.

Screenshot 2021-02-03 at 17 30 13

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can pass a third argument, a boolean, to diff to get a floating point number instead of an integer. then you could just use <= 1. https://jsfiddle.net/8whg49qj/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks will do it


function formatPreviousPeriodDates({
momentStart,
momentEnd,
Expand All @@ -38,7 +40,6 @@ function formatPreviousPeriodDates({
function getSelectOptions({ start, end }: { start?: string; end?: string }) {
const momentStart = moment(start);
const momentEnd = moment(end);
const dateDiff = getDateDifference(momentStart, momentEnd, 'days');

const yesterdayOption = {
value: 'yesterday',
Expand All @@ -59,13 +60,15 @@ function getSelectOptions({ start, end }: { start?: string; end?: string }) {
text: formatPreviousPeriodDates({ momentStart, momentEnd }),
cauemarcondes marked this conversation as resolved.
Show resolved Hide resolved
};

const dateDiffInHours = getDateDifference(momentStart, momentEnd, 'hours');

// Less than one day
if (dateDiff < 1) {
if (dateDiffInHours <= 24) {
return [yesterdayOption, aWeekAgoOption];
}

// Less than one week
if (dateDiff <= 7) {
if (dateDiffInHours <= weekDurationInHours) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test for this component?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/elastic/kibana/blob/master/x-pack/plugins/apm/public/components/shared/time_comparison/index.test.tsx

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I'm still wondering why the test doesn't need updating when the implementation changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a few more tests, also to cover what was discussed in the comments below.

return [aWeekAgoOption];
}

Expand Down