-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ES|QL] Dashboard variables #202875
base: main
Are you sure you want to change the base?
[ES|QL] Dashboard variables #202875
Conversation
@ThomThomson @dej611 can you please continue with your review? Thanx! |
@nreese will be finishing the review from the Presentation team side! |
reviewing today |
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.
This is looking really good. Thank you for moving the ESQLService calls out of controls and dashboard plugins and instead just publishing ESQL variables via a publishing subject.
src/platform/plugins/shared/controls/public/controls/esql_control/get_esql_control_factory.tsx
Outdated
Show resolved
Hide resolved
src/platform/plugins/shared/controls/public/controls/esql_control/register_esql_control.ts
Outdated
Show resolved
Hide resolved
src/platform/plugins/shared/controls/public/controls/esql_control/types.ts
Outdated
Show resolved
Hide resolved
...tform/plugins/shared/controls/public/controls/esql_control/get_esql_control_factory.test.tsx
Outdated
Show resolved
Hide resolved
...tform/plugins/shared/controls/public/controls/esql_control/get_esql_control_factory.test.tsx
Outdated
Show resolved
Hide resolved
...tform/plugins/shared/controls/public/controls/esql_control/get_esql_control_factory.test.tsx
Outdated
Show resolved
Hide resolved
src/platform/plugins/shared/dashboard/public/dashboard_api/unified_search_manager.ts
Show resolved
Hide resolved
src/platform/plugins/shared/controls/public/controls/esql_control/get_esql_control_factory.tsx
Outdated
Show resolved
Hide resolved
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.
kibana-presentation changes LGTM. ESQL variables is a powerful feature. Can't wait to see how these are used.
code review, tested in chrome
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.
x-pack/platform/plugins/shared/lens/public/app_plugin/shared/edit_on_the_fly/helpers.ts
Outdated
Show resolved
Hide resolved
...m/plugins/shared/lens/public/app_plugin/shared/edit_on_the_fly/lens_configuration_flyout.tsx
Outdated
Show resolved
Hide resolved
...m/plugins/shared/lens/public/app_plugin/shared/edit_on_the_fly/lens_configuration_flyout.tsx
Outdated
Show resolved
Hide resolved
...m/plugins/shared/lens/public/app_plugin/shared/edit_on_the_fly/lens_configuration_flyout.tsx
Outdated
Show resolved
Hide resolved
@@ -519,6 +586,9 @@ export function LensEditConfigurationFlyout({ | |||
isDisabled={false} | |||
allowQueryCancellation | |||
isLoading={isVisualizationLoading} | |||
onSaveControl={onSaveControl} |
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.
This component is becoming a props beast. Perhaps, as a follow up, would be worth to explore how to use the context for this kind of things and composition to enhance its features.
x-pack/platform/plugins/shared/lens/public/react_embeddable/data_loader.ts
Outdated
Show resolved
Hide resolved
@@ -58,7 +62,12 @@ function getSearchContext(parentApi: unknown) { | |||
timeRange$: new BehaviorSubject(undefined), | |||
}; | |||
|
|||
const esqlVariables = apiPublishesESQLVariables(parentApi) |
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 move this into internalApi
?
Type guards like these are performed at initialization level and the logic here should just use them.
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.
Marco is this what you had in mind? d3fde2a
const panel = useMemo(() => { | ||
if (!panelId) { | ||
return; | ||
} | ||
return dashboardPanels?.[panelId] as { | ||
updateAttributes: (attributes: TypedLensSerializedState['attributes']) => void; | ||
onEdit: () => Promise<void>; | ||
}; | ||
}, [dashboardPanels, panelId]); | ||
|
||
const onSaveControl = useCallback( | ||
async (controlState: Record<string, unknown>, updatedQuery: string) => { | ||
if (!panelId) { | ||
return; | ||
} | ||
|
||
// add a new control | ||
controlGroupApi?.addNewPanel({ | ||
panelType: 'esqlControl', | ||
initialState: { | ||
...controlState, | ||
id: uuidv4(), | ||
}, | ||
}); | ||
if (panel && updatedQuery) { | ||
panel.updateAttributes({ | ||
...attributes, | ||
state: { | ||
...attributes.state, | ||
query: { esql: updatedQuery }, | ||
needsRefresh: true, | ||
}, | ||
}); | ||
// open the edit flyout to continue editing | ||
await panel.onEdit(); | ||
} | ||
}, | ||
[attributes, controlGroupApi, panel, panelId] | ||
); | ||
|
||
const onCancelControl = useCallback(() => { | ||
closeFlyout?.(); | ||
if (panel) { | ||
panel.onEdit(); | ||
} | ||
}, [closeFlyout, panel]); |
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 would really like to not leak any embeddable specific logic in here, nor dashboard specific one.
It would be great to lift up all this logic outside of the component.
I understand this is already in an advanced phase, so I'd propose to move all this logic into a useESQLVariables(dashboardApi, panelId)
hook in another file for now.
As a follow up perhaps all of this should live somewhere else outside of this component.
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.
Done d2dbebb
.../plugins/shared/esql/public/triggers/esql_controls/control_flyout/shared_form_components.tsx
Outdated
Show resolved
Hide resolved
x-pack/platform/plugins/shared/lens/public/app_plugin/shared/edit_on_the_fly/types.ts
Outdated
Show resolved
Hide resolved
const onVariableNameChange = useCallback( | ||
(e: { target: { value: React.SetStateAction<string> } }) => { | ||
let text = String(e.target.value).replace('?', ''); | ||
if (text.charAt(0) === '_') { | ||
text = text.substring(1); | ||
} | ||
setVariableName(text); | ||
}, | ||
[] | ||
); |
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.
There is no validation on the variable name and it cause both ESQL exceptions and also unfixable lens configuration until you don't remove completely the control.
If you try to remove the wrong variable from the ESQL query you are not allow to save it and the error is unrecoverable if you don't remove the control from the dashboard first.
This happens for example if you use a variable name that contains white spaces or that starts with a /
.
You should harden the validation and blocks or disallow using special characters, or, if possible, escape it.
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.
Thanx Marco, done here 031386f and added a test too
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.
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.
This is irrelavant with this PR (this PR doesnt add variables support, we did that in previous PRs.
I will create an issue to track it on the kibana side, the server validation wrong highlight seems to be a bug, but again irrelevant with this PR
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.
Done #208142
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.
It seems as a bug in ES, this is why our highlighter doesnt work as expected. I pinged the folks.
Update: Yes it is, I created an issue on their repo elastic/elasticsearch#120778
@dej611 thanx a lot for the review, I addressed everything. About the async bundle I dont see this to create any problems and the sync bundle doesnt have any significant increat but I will investigate in a new PR if we can do something better here |
const esqlVariables$ = apiPublishesESQLVariables(parentApi) | ||
? parentApi.esqlVariables$ | ||
: undefined; |
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.
Perfect here thanks.
The only change I would ask is this change:
const esqlVariables$ = apiPublishesESQLVariables(parentApi) | |
? parentApi.esqlVariables$ | |
: undefined; | |
const esqlVariables$ = buildObservableVariable(apiPublishesESQLVariables(parentApi) ? parentApi.esqlVariables$ : undefined); |
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.
Had to tweak it a bit but here it is 014cba1
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
|
Summary
Closes #203967
Supports dashboard variables in ES|QL charts.
This PR introduces the first phase of ES|QL controls. In this phase:
For more info check this deck
Implementation details
Types of ES|QL variables
We have 2 types:
Example of a control creation from the editor
Release note
ES|QL charts now allow the creation of controls in dashboards. You can control a part of the query such as a field, an interval or a value.
Checklist
release_note:breaking
label should be applied in these situations.release_note:*
label is applied per the guidelines