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

lens esql generation #196049

Open
wants to merge 116 commits into
base: main
Choose a base branch
from
Open

lens esql generation #196049

wants to merge 116 commits into from

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Oct 14, 2024

Summary

updates lens so it uses ESQL when requesting data from elasticsearch rather than DSL when that is possible.

nothing should change for the user, everything should work exactly as before

but when going to inspector you will be able to see that in some cases we will send out ESQL request.

the feature is only enabled when the lens.enable_esql feature flag is enabled.

at the moment this covers very few cases, but rather builds the necessary infrastructure so we can keep improving this over time:

  • user has to be in UTC timezone
  • user needs to use set of lens operations that are supported

how to test:

  1. put this in your kibana.dev.yaml:
feature_flags.overrides:
  lens.enable_esql: true
  1. go to advanced settings and set timezone to UTC
  2. go to lens, configure a chart (for example a metric chart showing average of products.price
  3. go to inspector -> requests and you can see that we sent out ESQL query
  4. visually nothing should change

todo:

  • make sure CI is green when the feature flag is enabled

@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 labels Nov 12, 2024
@stratoula
Copy link
Contributor

stratoula commented Jan 22, 2025

I see an error in the console when I drop Records "field"

image

yes something goes wrong with the xAxis label
image

@stratoula
Copy link
Contributor

ok console error gone and x-axis working as expected. This is the only thing not working correctly #196049 (comment)

const appliedTimeRange = input?.timeRange
? {
from: DateMath.parse(input.timeRange.from),
to: DateMath.parse(input.timeRange.to),
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to round up here . . . right now the quick range of "Today" ends up being the same exact from/to.

Suggested change
to: DateMath.parse(input.timeRange.to),
to: DateMath.parse(input.timeRange.to, { roundUp: true }),
Screen.Recording.2025-01-22.at.1.42.08.PM.mov

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, not sure why but this issue is still happening after the change I suggested.

Copy link
Member Author

Choose a reason for hiding this comment

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

resolved in ec2506e

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 looks great Peter, LGTM!

@ppisljar ppisljar added backport:8.17 and removed backport:skip This commit does not require backporting labels Jan 29, 2025
@afharo
Copy link
Member

afharo commented Jan 29, 2025

Is this the first use of the Feature Flags service? Nice! 🙌

afharo
afharo previously requested changes Jan 29, 2025
Comment on lines 185 to 186
feature_flags.overrides:
lens.enable_esql: true
Copy link
Member

Choose a reason for hiding this comment

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

Please, remember to remove this before merging

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 was just added in the commit named revert before merging to check once again that CI is passing when this flag is on

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for confirming! I'm sure that Core won't be needed when this is removed :)

@afharo afharo dismissed their stale review January 29, 2025 12:11

Core review won't be needed when the changes to kibana.yml are removed

@elasticmachine
Copy link
Contributor

⏳ Build in-progress, with failures

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #102 / lens app - group 6 lens rollup tests should allow seamless transition to and from table view
  • [job] [logs] FTR Configs #65 / Search session sharing Search session sharing with lens should share search session with by value lens and don't share with by reference
  • [job] [logs] FTR Configs #65 / Search session sharing Search session sharing with lens should share search session with by value lens and don't share with by reference

History

@ppisljar
Copy link
Member Author

ppisljar commented Jan 29, 2025

as expected rollup and search sessions tests fail when turning the flag on, everything else passes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:8.17 Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants