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.

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

[ML] Transforms: Adds date picker to transform wizard for data view with time fields. #149049

Merged
merged 36 commits into from
Feb 1, 2023

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Jan 17, 2023

Summary

Part of #146187.

Adds a date picker to the transform wizard for data views with time fields.

The PR includes functionality to allow to set if the time range should get applied to the transform configuration or just be used for previews, however for now this is disabled with a hard coded feature flag. So the time range will only be applied to previews, not the final transform configuration.

  • Some code that referred to pivot but was meant to be used for both pivot/latest so was renamed to just transform*.
  • The form for latest was already in LatestFunctionForm, but the form for pivot was kept inline <StepDefineForm> component. This PR moves that inline code to PivotFunctionForm.
  • For functional tests, all date picker related code (previously already some existed to assert the date picker in Discover when displaying transform results) was moved to a new transform.datePicker service.

Checklist

@walterra walterra self-assigned this Jan 17, 2023
@walterra walterra force-pushed the 146187a-transform-date-picker branch 8 times, most recently from 5f0c069 to a0b7f86 Compare January 25, 2023 09:01
@walterra walterra force-pushed the 146187a-transform-date-picker branch 2 times, most recently from 674b552 to c01a95d Compare January 27, 2023 10:08
'The advanced editor allows you to edit the pivot configuration of the transform.',
})}{' '}
<EuiLink href={esTransformPivot} target="_blank">
{i18n.translate('xpack.transform.stepDefineForm.advancedEditorHelpTextLink', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit - is it worth getting pivot in the i18n key here, in case there is text for a help link in the Latest wizard too?

onClick={applyPivotChangesHandler}
disabled={!isAdvancedPivotEditorApplyButtonEnabled}
>
{i18n.translate('xpack.transform.stepDefineForm.advancedEditorApplyButtonText', {
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, is it worth adding pivot in the i18n key here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no reference to latest in the form for latest either so I'd prefer to keep the ids as is to keep the existing translations.

@@ -94,6 +98,18 @@ export const StepDefineSummary: FC<Props> = ({
>
<span>{searchItems.dataView.getIndexPattern()}</span>
</EuiFormRow>
{isDatePickerApplyEnabled && timeRangeMs && (
<EuiFormRow
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this time picker be made not to go full width rather than taking up the full form row?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment it's set up to get the full width like the search bar below. Suggest to revisit this once the follow ups (frozen option / full time range) are added too. I added a note in the meta issue here: #146187

@peteharverson
Copy link
Contributor

Could be an idea to suggest searching over a different time range as part of this info message:

image

qn895 added a commit to qn895/kibana that referenced this pull request Jan 30, 2023
@walterra
Copy link
Contributor Author

@peteharverson The callout for no source documents is rendered by the data grid component itself, it's not aware of outer individual options (like search or date picker), that's why it says the query is not too restrictive as the more technical term that includes all of that.

@walterra
Copy link
Contributor Author

Added an info tooltip to the time range label in 427d418.

image

@walterra
Copy link
Contributor Author

@peteharverson @qn895 Thanks for your comments, addressed and answered them, please have another look.

@@ -36,16 +36,16 @@ export default function ({ getService, loadTestFile }: FtrProviderContext) {
await transform.testResources.resetKibanaTimeZone();
});

loadTestFile(require.resolve('./permissions'));
// loadTestFile(require.resolve('./permissions'));
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to re-enable these tests back before merging

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 for catching! Fixed it in c64f1b2. (I think with the latest merge of main into this branch that whole code part was refactored away though, it's now organized differently)

@qn895
Copy link
Member

qn895 commented Jan 31, 2023

LGTM 🎉

'xpack.transform.stepDefineForm.datePickerIconTipContent',
{
defaultMessage:
'The time range will be applied to previews only, it will not be part of the final transform configuration.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest editing to The time range will be applied to previews only and will not be part of the final transform configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 2fcf33a.

@@ -47,9 +48,15 @@ export interface SimpleQuery {
};
}

export type PivotQuery = SimpleQuery | SavedSearchQuery;
export interface FilterBasedSimpleQuery {
bool: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a change from the previous 'empty' query that was applied "match_all": {} ? This is causing a side effect when cloning a transform, in that the search field shows the 'advanced' JSON version

image

rather than the preferable empty KQL bar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a note for a follow up in #146187.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR is up to fix this issue here: #151665

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
transform 274 296 +22

Async chunks

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

id before after diff
transform 348.1KB 360.4KB +12.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
transform 15.4KB 15.6KB +157.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
transform 27 31 +4

Total ESLint disabled count

id before after diff
transform 30 34 +4

History

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

cc @walterra

@walterra walterra merged commit 0085aae into elastic:main Feb 1, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Feb 1, 2023
@walterra walterra deleted the 146187a-transform-date-picker branch February 1, 2023 08:23
kqualters-elastic pushed a commit to kqualters-elastic/kibana that referenced this pull request Feb 6, 2023
…ith time fields. (elastic#149049)

Adds a date picker to the transform wizard for data views with time
fields. The time range will be applied to previews only.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Transforms ML transforms :ml release_note:enhancement v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants