-
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
[Lens] Formula time shift #101718
[Lens] Formula time shift #101718
Conversation
…a into lens/formula-error-handling
Pinging @elastic/kibana-app (Team:KibanaApp) |
@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.
@@ -603,6 +607,8 @@ export function DimensionEditor(props: DimensionEditorProps) { | |||
const formulaTab = ParamEditor ? ( | |||
<ParamEditor | |||
layer={state.layers[layerId]} | |||
layerId={layerId} | |||
activeData={props.activeData} |
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 looks like it'll cause extra re-renders of the formula panel and all the others, any idea how much of an impact this has?
extraArgs += i18n.translate('xpack.lens.formula.shiftExtraArguments', { | ||
defaultMessage: '[shift]?: string', | ||
}); | ||
} |
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 logic here breaks when transitioning to a shifted count when using the en-xa
locale, it produces count(Recordsshift='3h')
instead of count(shift='3h')
. Maybe we're doing some string equality?
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.
Because now an extra argument is allowed on functions, the lucene
suggestion is coming even where a kql
query has been already put in place.
Would it be possible to remove it? There's already a validation error, but it would be nice to not promote it.
Also, I was wondering if we could improve something on this side: not a blocker but the helping tooltip is not pointing to the right direction at least.
Addressed the following points:
|
@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.
LGTM, the performance impact of the extra re-renders appears to be minimal.
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
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. Tested again and found no more issues 👍
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Co-authored-by: Joe Reuter <[email protected]>
Fixes #35043
Adds time shift support to formula.
You can use time shift on all Elasticsearch functions (
average
,sum
etc.) - the parameter is calledshift
and takes the same input as the "Time shift" advanced option for quick functions. Inputs than won't parse (like3ds
instead of3d
) will cause a formula error and won't render. Time shifts which are not aligned with the date histogram interval will cause a formula warning but will be treated as value and render as usual.