-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[Reporting] Fix timestamp override for ES|QL CSV scheduled reports with relative time ranges #248169
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
[Reporting] Fix timestamp override for ES|QL CSV scheduled reports with relative time ranges #248169
Changes from 6 commits
577c414
2db7e1f
50e088a
ed06b25
35dd7f4
6afa934
a633bf5
e6b857e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| import type { IUiSettingsClient, SavedObjectsClientContract } from '@kbn/core/server'; | ||
| import { coreMock, httpServerMock } from '@kbn/core/server/mocks'; | ||
| import type { ISearchStartSearchSource } from '@kbn/data-plugin/common'; | ||
| import { dataPluginMock } from '@kbn/data-plugin/server/mocks'; | ||
| import type { LocatorServicesDeps as Services } from '.'; | ||
| import { timeFieldNameFromLocatorFactory } from './time_field_name_from_locator'; | ||
|
|
||
| const coreStart = coreMock.createStart(); | ||
| let uiSettingsClient: IUiSettingsClient; | ||
| let soClient: SavedObjectsClientContract; | ||
| let searchSourceStart: ISearchStartSearchSource; | ||
| let mockServices: Services; | ||
|
|
||
| beforeAll(async () => { | ||
| const dataStartMock = dataPluginMock.createStartContract(); | ||
| const request = httpServerMock.createKibanaRequest(); | ||
| soClient = coreStart.savedObjects.getScopedClient(request); | ||
| uiSettingsClient = coreMock.createStart().uiSettings.asScopedToClient(soClient); | ||
| searchSourceStart = await dataStartMock.search.searchSource.asScoped(request); | ||
|
|
||
| mockServices = { | ||
| searchSourceStart, | ||
| savedObjects: soClient, | ||
| uiSettings: uiSettingsClient, | ||
| }; | ||
| }); | ||
|
|
||
| test(`returns timeFieldName from DiscoverAppLocatorParams`, async () => { | ||
| const params = { dataViewSpec: { timeFieldName: '@timestamp' } }; | ||
| const timeFieldNameFromLocatorFn = timeFieldNameFromLocatorFactory(mockServices); | ||
| const timeField = await timeFieldNameFromLocatorFn(params); | ||
| expect(timeField).toBe('@timestamp'); | ||
| }); | ||
|
|
||
| test(`returns undefined if there is no timeFieldName in DiscoverAppLocatorParams`, async () => { | ||
| const params = { dataViewSpec: {} }; | ||
| const timeFieldNameFromLocatorFn = timeFieldNameFromLocatorFactory(mockServices); | ||
| const timeField = await timeFieldNameFromLocatorFn(params); | ||
| expect(timeField).toBeUndefined(); | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,31 @@ | ||||||||||
| /* | ||||||||||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||||||||||
| * or more contributor license agreements. Licensed under the "Elastic License | ||||||||||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||||||||||
| * Public License v 1"; you may not use this file except in compliance with, at | ||||||||||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||||||||||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||||||||||
| */ | ||||||||||
|
|
||||||||||
| import type { LocatorServicesDeps } from '.'; | ||||||||||
| import type { DiscoverAppLocatorParams } from '../../common'; | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * @internal | ||||||||||
| */ | ||||||||||
| export const timeFieldNameFromLocatorFactory = (services: LocatorServicesDeps) => { | ||||||||||
| /** | ||||||||||
| * @public | ||||||||||
| */ | ||||||||||
| const timeFieldNameFromLocator = async ( | ||||||||||
| params: DiscoverAppLocatorParams | ||||||||||
| ): Promise<string | undefined> => { | ||||||||||
| if (params.dataViewSpec && params.dataViewSpec.timeFieldName) { | ||||||||||
| return params.dataViewSpec?.timeFieldName; | ||||||||||
| } | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think the original suggestion was probably to drop the if statement entirely in favour of optional chaining, which makes sense because shouldn't need both. On a separate note, I have some reservations about relying on
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh okay, that's my bad. I misunderstood Patrick's comment Sorry for not being clear, sometimes when they user specifies a relative time range it get's saved as an absolute time range. We received an SDH about it and I can replicate this in 8.19, but in main rn I can't get it to save as the absolute time range. In the verification steps I provide a scheduled report that forces it to be an absolute time range, so you can verify that the code overrides the time range.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries, and thanks for explaining! I understand better now and think we're good to merge this. FWIW the SDH bug sounds like this one: #223171. We merged a fix for it in #223249 (8.19 backport in #223346), but it was overwritten by a separate backport and missed the 8.19 release. We then merged a separate fix for it to 8.19.7 in #241931. So users who encounter it in earlier versions of 8.19 will need to upgrade to 8.19.7+ to fix it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh nice, thanks! |
||||||||||
| }; | ||||||||||
|
|
||||||||||
| return timeFieldNameFromLocator; | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| export type TimeFieldNameFromLocatorFn = ReturnType<typeof timeFieldNameFromLocatorFactory>; | ||||||||||
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 we change this to
return params.dataViewSpec?.timeFieldName;?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.
Yeah! Updated in this commit, ed06b25