-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Lens] Move Lens functions to common
#105455
Conversation
Pinging @elastic/kibana-app (Team:KibanaApp) |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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've done a cursory scan over the code here and I have a few questions, if necessary we should have a meeting to get everyone aligned. Here are the questions:
- Why do we need to move the render functions to the server? I understand moving the intermediate functions like
time_scale
, but whylens_xy_chart
? - How will time zones be handled?
- Why do we need to register the functions to the server in this PR, when the alternative approach is to only move to
common
in this PR?
// the datemath plugin always parses dates by using the current default moment time zone. | ||
// to use the configured time zone, we are switching just for the bounds calculation. | ||
const defaultTimezone = moment().zoneName(); | ||
moment.tz.setDefault(timeInfo.timeZone); |
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.
What I'm seeing is that timeInfo.timeZone
is undefined most of the time, because the time zone is not provided on esaggs by default. The time zone information might need to be sourced from an alternative place. For example, if the alerting task starts it, it might need to store a timezone parameter that gets passed in as part of the expression context. Have you considered that here?
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've created a new argument for time_scale
to pass a timeZone
to be used on the server side: you think it would be better to extend the context instead?
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'm not sure how this will be handled, I think it will require discussion with the app services & alerting teams.
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.
Left a comment on the main PR thread: this topic will be discussed in a follow up PR completely dedicated to the server side registration.
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.
@dej611 I've tried to manually review this and mostly the changes LGTM, splitting out the huge types.ts
files into multiple files is a good change. The main thing I'm confused about is that you've moved some, but not all, expression renderers into common
along with the core types like LensMultiTable
. I thought you were going to do a smaller change?
DatatableRender | ||
> => ({ | ||
name: 'lens_datatable', | ||
type: 'render', |
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.
Note: this lens_databable
renderer is moved, trying to understand why.
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.
Moved back lens_datatable
function and all its dependencies to public
. Will dedicate a PR to this as follow up.
export type LensGridDirection = 'none' | Direction; | ||
|
||
export interface ColumnConfig { | ||
columns: Array< |
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.
Why have you changed this type, it no longer uses columns: ColumnConfigArg[]
which means you're missing the summaryRowValue
type?
Seems like there's no reason to copy+paste the definition here
originalColumnId?: string; | ||
originalName?: string; | ||
bucketValues?: Array<{ originalBucketColumn: DatatableColumn; value: unknown }>; |
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 can see that you've copied the ColumnState definition from datatable/visualization.tsx
but these three types appear to be completely unused. Can you verify that and maybe remove 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.
All of these types are used in the transposeTable
function flow within the lens_datatable
expression function
sortingColumnId: string | undefined; | ||
sortingDirection: 'asc' | 'desc' | 'none'; |
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 change LGTM, previously it was called sorting?: SortingState
in datatable_visualization/visualization.tsx
import { HeatmapGridConfigResult } from './heatmap_grid'; | ||
import { HeatmapLegendConfigResult } from './heatmap_legend'; | ||
|
||
export type ChartShapes = 'heatmap'; |
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 type has been simplified from typeof CHART_SHAPES[keyof typeof CHART_SHAPES]
to just heatmap
. Seems fine but it's the only change to heatmap types I can find
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.
Correct. I assumed that in case of generalisation it could be augmented later on here.
type OriginalColumn = { id: string; label: string } & ( | ||
| { operationType: 'date_histogram'; sourceField: string } | ||
| { operationType: string; sourceField: never } | ||
); |
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.
Not entirely sure what this is about?
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.
Unfortunately the type OriginalColumn
depends on all the operation definitions (it's a union of them) and moving them here it is not necessary, or useful now.
The goal of this type is to make it work correctly the getColumnName
which has a specific logic for date_histogram
, therefore the type has been simplified to cover that specific case.
// the datemath plugin always parses dates by using the current default moment time zone. | ||
// to use the configured time zone, we are switching just for the bounds calculation. | ||
const defaultTimezone = moment().zoneName(); | ||
moment.tz.setDefault(timeInfo.timeZone); |
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'm not sure how this will be handled, I think it will require discussion with the app services & alerting teams.
d: 1000 * 60 * 60 * 24, | ||
}; | ||
|
||
export const timeScale: ExpressionFunctionDefinition< |
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.
So you're no longer passing in the dataPluginPublicStart
object which was previously used to get the auto interval, you're directly importing 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.
Correct. I had a look at the code and assumed there were no strict dependencies in the used function with its context, and that the function could have been used independently.
Talked offline with @ppisljar and sorted out a roadmap for this:
So I'm proceeding in this PR with 1 and create a follow up PR with 2. |
Moved the render functions back, so step 1 completed. |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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.
Based on previous review and a new pass this LGTM. Did not run the code, just read through it.
@elasticmachine merge upstream |
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.
The code LGTM, I tested briefly in Chrome and didn't notice anything suspicious.
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Unknown metric groupsAPI count
References to deprecated APIs
History
To update your PR or re-run it, just comment with: |
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.
code LGTM
* 🚚 First move batch to common * 🚚 Second batch of move * 🏷️ Import types only * 🚚 Third batch * 🚚 Fourth batch move * 🚚 Another module moved * 🚚 More function moved * 🚚 Last bit of move * ⚡ Reduce page load bundle size * 🐛 Fix import issue * 🐛 More import fix * ✨ Registered functions on the server * 🐛 Expose datatable_column as well * ✅ Add server side expression test * 🚚 Moved back render functions to public * ✨ Add a timezone arg to time_scale * 🔥 Remove timezone arg * 🔥 Remove server side code for now * 👌 Integrated feedback * 🚚 Move back datatable render function * 🏷️ Fix imports * 🏷️ Fix missing export * 🚚 Move render functions back! Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
* 🚚 First move batch to common * 🚚 Second batch of move * 🏷️ Import types only * 🚚 Third batch * 🚚 Fourth batch move * 🚚 Another module moved * 🚚 More function moved * 🚚 Last bit of move * ⚡ Reduce page load bundle size * 🐛 Fix import issue * 🐛 More import fix * ✨ Registered functions on the server * 🐛 Expose datatable_column as well * ✅ Add server side expression test * 🚚 Moved back render functions to public * ✨ Add a timezone arg to time_scale * 🔥 Remove timezone arg * 🔥 Remove server side code for now * 👌 Integrated feedback * 🚚 Move back datatable render function * 🏷️ Fix imports * 🏷️ Fix missing export * 🚚 Move render functions back! Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
* 🚚 First move batch to common * 🚚 Second batch of move * 🏷️ Import types only * 🚚 Third batch * 🚚 Fourth batch move * 🚚 Another module moved * 🚚 More function moved * 🚚 Last bit of move * ⚡ Reduce page load bundle size * 🐛 Fix import issue * 🐛 More import fix * ✨ Registered functions on the server * 🐛 Expose datatable_column as well * ✅ Add server side expression test * 🚚 Moved back render functions to public * ✨ Add a timezone arg to time_scale * 🔥 Remove timezone arg * 🔥 Remove server side code for now * 👌 Integrated feedback * 🚚 Move back datatable render function * 🏷️ Fix imports * 🏷️ Fix missing export * 🚚 Move render functions back! Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
Fixes #97134 (first part)
List of functions:
lens_time_scale
lens_counter_rate
lens_rename_columns
lens_format_column
lens_merge_tables
lens_suffix_formatter
lens_datatable_column
lens_datatable
lens_metric_chart
lens_pie
lens_xy_legendConfig
lens_xy_yConfig
lens_xy_tickLabelsConfig
lens_xy_gridlinesConfig
lens_xy_axisTitlesVisibilityConfig
lens_xy_layer
lens_xy_chart
lens_heatmap
lens_heatmap_grid
lens_heatmap_legendConfig
More tasks:
Probably need a follow up on this PR to split the datatable function into 2 separate expression functions (1 data processing + 1 rendering)Checklist
Delete any items that are not applicable to this PR.
For maintainers