Skip to content

[Metrics][Discover] Discover to prefer line chars for time series data #244595

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

[Metrics][Discover] Discover to prefer line chars for time series data #244595

wants to merge 8 commits into from

Conversation

@crespocarlos
Copy link
Contributor

@crespocarlos crespocarlos commented Nov 28, 2025 •
edited
Loading

closes #236261

Summary

Changes the call to Suggestion API in Discover to set the preferred char type as line if the query contains a timeseries bucket aggregation

Should work

image

Important

The scenario above depends on this PR #244753

image

Should not work
image

This was referenced Nov 28, 2025
Closed
Open
@crespocarlos crespocarlos force-pushed the 236261-discover-prefer-line-chart-for-bucket-agg-poc branch 2 times, most recently from 3444eab to f37dab2 Compare November 28, 2025 15:14
@crespocarlos crespocarlos force-pushed the 236261-discover-prefer-line-chart-for-bucket-agg-poc branch 3 times, most recently from 735f551 to ca5109f Compare December 1, 2025 11:18
@crespocarlos crespocarlos force-pushed the 236261-discover-prefer-line-chart-for-bucket-agg-poc branch from ca5109f to 4c88e52 Compare December 1, 2025 11:20
@crespocarlos crespocarlos changed the title [Metrics][Discover] Discover to prefer line chars for time series data POC [Metrics][Discover] Discover to prefer line chars for time series data Dec 1, 2025
@crespocarlos crespocarlos added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting Team:DataDiscovery Discover, search (data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. t// Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team Team:ESQL ES|QL related features in Kibana t// v9.3.0 labels Dec 1, 2025
@crespocarlos crespocarlos marked this pull request as ready for review December 1, 2025 14:31
@crespocarlos crespocarlos requested review from a team as code owners December 1, 2025 14:31
@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 1, 2025

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 1, 2025

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 1, 2025

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

@crespocarlos
Copy link
Contributor Author

crespocarlos commented Dec 1, 2025

@elasticmachine merge upstream

stratoula
stratoula reviewed Dec 2, 2025
View reviewed changes
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.

Thanx Carlos, it works ok! I have added some ideas on how to make this function a bit smarter. I dont think you need the bucketColumns logic. Check my last comment.

You also don't get into account the TBucket function which I am assuming is going to be the most popular one with timeseries.

Last but not least before we merge it, let's wait for this to get merged first #244753 (to test them together) and I am also seen some weird behavior in ES|QL in some cases, for example

TS kibana_sample_data_logstsdb | EVAL meow = DATE_TRUNC(1 hour, @timestamp) | STATS AVG(LAST_OVER_TIME(bytes)) BY meow

The above is another way to create time buckets and is not working correctly in ES|QL TS mode. I asked the team, so let's wait for them to respond first to see if we should also include it on our checks or not!

davismcphee
davismcphee approved these changes Dec 2, 2025
View reviewed changes
Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Tested locally, Data Discovery changes LGTM 👍

davismcphee
davismcphee reviewed Dec 2, 2025
View reviewed changes
src/platform/packages/shared/kbn-unified-histogram/services/lens_vis_service.ts
Comment on lines +665 to +668
const preferredChartType =
!mappedPreferredChartType && hasTimeseriesBucketAggregation(query.esql, columns)
? ChartType.Line
: mappedPreferredChartType;
Copy link
Contributor

@davismcphee davismcphee Dec 2, 2025

Choose a reason for hiding this comment

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

Ideally we'd add a test case for this to lens_vis_service.suggestions.test.ts.

Copy link
Contributor Author

@crespocarlos crespocarlos Dec 3, 2025

Choose a reason for hiding this comment

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

thanks. I'll add a test case there

Copy link
Contributor Author

@crespocarlos crespocarlos Dec 3, 2025

Choose a reason for hiding this comment

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

6b9498e

@crespocarlos
Copy link
Contributor Author

crespocarlos commented Dec 3, 2025

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 3, 2025

💚 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
dataVisualizer 597.5KB 597.1KB -420.0B
discover 1.3MB 1.3MB +473.0B
total +53.0B

Page load bundle

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

id before after diff
dataVisualizer 29.3KB 29.3KB +9.0B
Unknown metric groups

API count

id before after diff
@kbn/esql-utils 144 147 +3

async chunk count

id before after diff
dataVisualizer 19 20 +1

History

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davismcphee davismcphee davismcphee approved these changes

@stratoula stratoula Awaiting requested review from stratoula

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

backport:skip This PR does not require backporting Feature:Metrics in Discover release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. t// Team:ESQL ES|QL related features in Kibana t// Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team v9.3.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Metrics][Discover] Default to optimal chart type (Line vs Area) for time-series queries

5 participants

@crespocarlos @elasticmachine @stratoula @davismcphee @simianhacker