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

[ES|QL] Supports custom formatters in charts #201540

Merged
merged 19 commits into from
Dec 11, 2024

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Nov 25, 2024

Summary

adds labeling and formatting supports to esql charts

image

@ppisljar ppisljar added Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:ESQL ES|QL related features in Kibana labels Nov 25, 2024
@ppisljar ppisljar marked this pull request as ready for review November 26, 2024 09:44
@ppisljar ppisljar requested review from a team as code owners November 26, 2024 09:44
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-esql (Team:ESQL)

@@ -315,7 +315,14 @@ export const getEsqlFn = ({ getStartDependencies }: EsqlFnArguments) => {
(body.all_columns ?? body.columns)?.map(({ name, type }) => ({
id: name,
name,
meta: { type: esFieldTypeToKibanaFieldType(type), esType: type },
meta: {
Copy link
Member

Choose a reason for hiding this comment

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

Being not familiar with this are of the code, and Lukas being on PTO, I wonder, how can this be tested in the DataDiscovery world? thx

Copy link
Member Author

Choose a reason for hiding this comment

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

Go to discover, switch to esql mode and enter a query which contains | WHERE @timestamp >= ?_tstart AND @timestamp <= ?_tend | STATS ...., save this to a new dashboard
now change a query to add a new field, remove the auto added dimension, manually add a new dimension, or change an existing one ... the field list is empty on main, but contains correct fields on this branch

@ppisljar ppisljar requested a review from a team as a code owner December 3, 2024 08:16
Comment on lines 177 to 179
{!isFullscreen && (
<div className="lnsIndexPatternDimensionEditor--padded lnsIndexPatternDimensionEditor--collapseNext">
{selectedField && (
Copy link
Member

Choose a reason for hiding this comment

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

nit: you can bring selectedField up to the initial condition and remove it from the rest of this code.

Suggested change
{!isFullscreen && (
<div className="lnsIndexPatternDimensionEditor--padded lnsIndexPatternDimensionEditor--collapseNext">
{selectedField && (
{!isFullscreen && selectedField && (
<div className="lnsIndexPatternDimensionEditor--padded lnsIndexPatternDimensionEditor--collapseNext">

Comment on lines 48 to 51
.filter((col) => col.params?.format)
.map((col) => {
// TODO: improve the type handling here
const format = col.params!.format!;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.filter((col) => col.params?.format)
.map((col) => {
// TODO: improve the type handling here
const format = col.params!.format!;
.filter((col): col is TextBasedLayerColumn & {params: {format: ValueFormatConfig}} => col.params?.format)
.map((col) => {
const format = col.params.format;

Copy link
Member Author

Choose a reason for hiding this comment

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

this doesnt seem to work
Type 'ValueFormatConfig | undefined' is not assignable to type 'boolean'.
Type 'undefined' is not assignable to type 'boolean'.

Copy link
Member

Choose a reason for hiding this comment

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

 .filter((col): col is TextBasedLayerColumn & { params: { format: ValueFormatConfig } } =>
      Boolean(col.params?.format)
    )

Copy link
Member Author

Choose a reason for hiding this comment

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

is this really better from what we currently have ?

Copy link
Member

Choose a reason for hiding this comment

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

This creates a type guard that is the way to fix col.params!.format!; in this place.
You can move the filter/typeguard function outside this code to make it a bit less verbose.

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

ResponseOps LGTM!

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

I havent tested yet but the change in fetchFieldsFromESQL seems to affect the editor

packages/kbn-esql-editor/src/fetch_fields_from_esql.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

This works great and is a nice addition, kudos Peter

I updated the labels as I think it should go to 8.18 and also be added on our release notes

@stratoula stratoula added release_note:enhancement v9.0.0 backport:version Backport to applied version labels v8.18.0 Feature:ES|QL ES|QL related features in Kibana and removed release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Dec 9, 2024
@stratoula stratoula changed the title adds labeling and formatting support to esql charts [ES|QL] Supports custom formatters in charts Dec 9, 2024
@markov00 markov00 self-requested a review December 9, 2024 08:50
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

I left a last minor change as comment, it will be nice if that can be fixed, otherwise we can revisit it later.

@kertal kertal requested review from kertal and a team December 9, 2024 09:33
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

DataDiscovery Code LGTM, tested locally and works as expected. It's a nice enhancement 👍

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
discover 784.9KB 784.9KB +19.0B
esql 202.5KB 202.5KB +35.0B
lens 1.5MB 1.5MB +2.5KB
stackAlerts 73.5KB 73.5KB -48.0B
total +2.5KB

Page load bundle

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

id before after diff
data 421.0KB 421.0KB -12.0B
stackAlerts 25.2KB 25.1KB -139.0B
total -151.0B

History

@ppisljar ppisljar merged commit 168e67d into elastic:main Dec 11, 2024
9 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12269844696

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 11, 2024
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Dec 11, 2024
# Backport

This will backport the following commits from `main` to `8.x`:
- [[ES|QL] Supports custom formatters in charts
(#201540)](#201540)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Peter
Pisljar","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-11T04:45:05Z","message":"[ES|QL]
Supports custom formatters in charts
(#201540)","sha":"168e67d50d2d36110ba26f130f381da96ddf163e","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:enhancement","Team:Visualizations","v9.0.0","Feature:ES|QL","Team:ESQL","backport:version","v8.18.0"],"title":"[ES|QL]
Supports custom formatters in
charts","number":201540,"url":"https://github.com/elastic/kibana/pull/201540","mergeCommit":{"message":"[ES|QL]
Supports custom formatters in charts
(#201540)","sha":"168e67d50d2d36110ba26f130f381da96ddf163e"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/201540","number":201540,"mergeCommit":{"message":"[ES|QL]
Supports custom formatters in charts
(#201540)","sha":"168e67d50d2d36110ba26f130f381da96ddf163e"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Peter Pisljar <[email protected]>
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels Feature:ES|QL ES|QL related features in Kibana release_note:enhancement Team:ESQL ES|QL related features in Kibana Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants