-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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.
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
[APM] Fix time comparison types when relative time range is selected #126567
Conversation
Pinging @elastic/apm-ui (Team:apm) |
@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. |
['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}`, {}); | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please double check that this is the behaviour we want. And would be great with more test coverage
52b94b5
to
af251d2
Compare
rangeFrom, | ||
rangeTo, | ||
}); | ||
const { start, end } = useTimeRange({ rangeFrom, rangeTo }); | ||
|
||
const { | ||
urlParams: { comparisonEnabled, comparisonType }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
start, | ||
end, | ||
}: { | ||
comparisonTypes: TimeRangeComparisonEnum[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: shouldn't this be called TimeRangeComparisonType
or TimeRangeComparisonTypeEnum
?
comparisonTypes: TimeRangeComparisonEnum[]; | |
comparisonTypes: TimeRangeComparisonTypeEnum[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll get it changed
refreshTimeRange: () => void; | ||
timeRangeId: number; | ||
} | ||
|
||
type PartialTimeRange = Pick<TimeRange, 'refreshTimeRange' | 'timeRangeId'> & | ||
Pick<Partial<TimeRange>, 'start' | 'end' | 'exactStart' | 'exactEnd'>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for removing this exactStart
and exactEnd
!
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', | ||
}, | ||
]); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like these tests!
Sure, I'll do some more thorough manual testing and merge it in if all looks good. |
comparisonEnabled, | ||
comparisonType, | ||
}); | ||
const { offset } = useComparison(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I strongly recommend using getTimeRangeComparison
here still. useComparison
(without arguments) breaks any type safety that we have from the route params.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain further? It seems that rangeFrom
, rangeTo
, comparisonEnabled
and comparisonType
are retrieved in a similar way from useComparison
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
66db590
to
367fa6b
Compare
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
…lastic#126567) * Remove exactStart and exactEnd * Add tests for comparison handling * Remove useComparison hook
Summary
Fix bugs around the comparison feature. The following comparison options are now returned:
Day before
andWeek 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:
comparisonType
,comparisonEnabled
andlatencyAggregationType
withuseApmParams
instead ofuseLegacyUrlParams
in most placeslatencyAggregationType
in the routeCloses #115247