[ML] Adds Field statistics embeddable as panel in Dashboard #184030
Conversation
|
/ci |
|
Done some early testing of this one (against commit cb3fd16) - overall looks good. One quick question - are you going to add in the ability to add a filter from values in the expanded rows (it's available from the panel added from a search saved in Discover)? |
| ): UiActionsActionDefinition<FieldStatsActionContext> { | ||
| return { | ||
| id: 'create-field-stats-table', | ||
| getIconType: () => 'inspect', |
There was a problem hiding this comment.
@joana-cps any thoughts on this icon?
There was a problem hiding this comment.
This icon is already in use in dashboards to inspect elements, I'll draft up a new one more tailored for field statistics. Added to this UX issue in progress
|
/ci |
|
|
||
| const [dataViewId, setDataViewId] = useState(initialInput?.dataViewId ?? ''); | ||
| const [viewType, setViewType] = useState( | ||
| initialInput?.viewType ?? FieldStatsInitializerViewType.ESQL |
There was a problem hiding this comment.
Is ES|QL the default in Discover yet? (and what about Serverless)? Just wonder if the default should match the default in Discover?
|
Pinging @elastic/ml-ui (:ml) |
qn895
added
Feature:File and Index Data Viz
...public/application/index_data_visualizer/embeddables/field_stats/field_stats_initializer.tsx
Outdated
Show resolved
Hide resolved
qn895
added
the
impact:medium
| const references: Reference[] = dataViewId | ||
| ? [ | ||
| { | ||
| type: 'index-pattern', |
There was a problem hiding this comment.
Switching from DATA_VIEW_SAVED_OBJECT_TYPE to 'index-pattern' will not have an impact on page load bundle size. x-pack/plugins/data_visualizer/public/application/index_data_visualizer/embeddables/field_stats/field_stats_factory.tsx is imported as const { getFieldStatsChartEmbeddableFactory } = await import('./field_stats_factory'); in x-pack/plugins/data_visualizer/public/application/index_data_visualizer/embeddables/field_stats/index.ts. This means that the file is not included in page load bundle and is only imported when the callback passed to registerReactEmbeddableFactory is called. This callback is not called until the embeddable is rendered in a dashbaord.
There was a problem hiding this comment.
It was a test that I intended to revert. It's now been reverted.
| ...timeRangeApi, | ||
| ...titlesApi, | ||
| ...fieldStatsControlsApi, | ||
| ...dataLoadingApi, |
There was a problem hiding this comment.
Couple questions about exposing dataLoadingApi in your API. The type FieldStatisticsTableEmbeddableApi does not include keys for onRenderComplete, onLoading, and onError. Is there a reason why these keys are getting exposed via the embeddable API? Do consumers of the api use them?
Another point dataLoading$ needs to be exposed as dataLoading. There is some inconsistencies in naming that we need to fix on the interface side.
| updateUserInput: (update: Partial<FieldStatsInitialState>) => void; | ||
| } | ||
|
|
||
| export type FieldStatisticsTableEmbeddableApi = |
There was a problem hiding this comment.
API should type should also include PublishesDataLoading and PublishesBlockingError.
| }) | ||
| ) | ||
| .subscribe((nextSelectedDataView) => { | ||
| if (nextSelectedDataView) { |
There was a problem hiding this comment.
Fetching initialDataView sets a blocking error when data view id can not be found. Here, when data view id changes and data view can not be found, the error is ignored and no blocking error is set. Is this on purpose? Should switchMap return an error in the catch? Something more like
if (fieldStatsControlsApi.dataViewId$) {
subscriptions.add(
fieldStatsControlsApi.dataViewId$
.pipe(
skip(1),
skipWhile((dataViewId) => !dataViewId && !defaultDataViewId),
switchMap(async (dataViewId) => {
try {
const dataView = await deps.data.dataViews.get(dataViewId ?? defaultDataViewId);
return { dataView };
} catch (error) {
return { dataView: undefined, error };
}
})
)
.subscribe(({ dataView, error }) => {
if (error) {
onError(error);
}
dataViews$.next([dataView]);
})
);
}
There was a problem hiding this comment.
I don't think this should be a blocking error. The table itself doesn't need any data view to render, we are fetching data views only for dashboard filters to show the values properly.
There was a problem hiding this comment.
I don't think this should be a blocking error. The table itself doesn't need any data view to render, we are fetching data views only for dashboard filters to show the values properly.
Thanks for explaining.
I think the only change needed is that switchMap function needs to be async, switchMap(async (dataViewId) => {}. And then await dataViews.get for the return return await deps.data.dataViews.get(dataViewId ?? defaultDataViewId);
|
|
||
| fieldStatsControlsApi.updateUserInput(nextUpdate); | ||
| } catch (e) { | ||
| return Promise.reject(e); |
There was a problem hiding this comment.
Promise.reject(e) will cause onEdit to throw. EditPanelAction.execute does not have any try/catch around onEdit calls, resulting in unhandled error. How about displaying a warning toast when editing fails instead of throwing?
There was a problem hiding this comment.
I added an error toast instead of warning toast, since that means the update fails for some reason and we probably would like to see the error trace. Let me know if that's not the standard and I can update it to be a warning toast.
| toasts.addWarning(ERROR_MSG.APPLY_FILTER_ERR); | ||
| return; | ||
| // eslint-disable-next-line no-console | ||
| console.error(`${ERROR_MSG.APPLY_FILTER_ERR}: APPLY_FILTER_TRIGGER is undefined`); |
There was a problem hiding this comment.
console.error can be removed. Its after a return so this code will never get run
|
Design changes LGTM, thanks @qn895 ! |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @qn895 |
peteharverson
changed the title
Summary
This PR adds Field statistics embeddable as panel in Dashboard
By default, it will enable the ES|QL editor for the field stats panel. It will allow for editing of the ES|QL query, and time range.
Screen.Recording.2024-07-01.at.14.40.16.mov
Screen.Recording.2024-07-01.at.14.41.01.mov
If and only if ES|QL is disabled, it will show the data view picker as a fallback.
Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers