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] Time scaling without date histogram #140107

Merged
merged 16 commits into from Sep 15, 2022
Merged

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Sep 6, 2022

Closes: #79656

Summary

In #77811 the option to apply a time unit to the value of a time series is discussed (e.g. per hour, per day independent of the underlying Elasticsearch interval).

The same function could also be provided for non-timebased charts as long as a time range filter can be applied because the used index pattern has a configured default timefield.

In this case the time scaling would behave as if the complete time range is a single large bucket and scaled accordingly.

A visualization which can be realized using this is to show the rate of ingested documents per hour for the complete current time range by using a metric visualization on count with time scale applied to it.

Which operations suppport Normalize by unit ?

  • count
  • sum

Screens

Screen.Recording.2022-09-07.at.15.39.12.mov

@alexwizp alexwizp changed the title [WIP][Lens] Time scaling without date histogram [Lens] Time scaling without date histogram Sep 7, 2022
@alexwizp alexwizp self-assigned this Sep 7, 2022
@alexwizp alexwizp added v8.5.0 release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens release_note:feature Makes this part of the condensed release notes backport:skip This commit does not require backporting and removed release_note:enhancement labels Sep 7, 2022
@alexwizp alexwizp added this to In progress in Lens via automation Sep 7, 2022
@alexwizp alexwizp marked this pull request as ready for review September 7, 2022 14:15
@alexwizp alexwizp requested review from a team as code owners September 7, 2022 14:15
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

@alexwizp
Copy link
Contributor Author

alexwizp commented Sep 7, 2022

@elasticmachine merge upstream

@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Sep 7, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@alexwizp
Copy link
Contributor Author

alexwizp commented Sep 8, 2022

@elasticmachine merge upstream

@@ -128,6 +128,7 @@ export const countOperation: OperationDefinition<CountIndexPatternColumn, 'field
isBucketed: false,
scale: 'ratio',
sourceField: field.name,
timeScalingMode: 'optional',
Copy link
Contributor

Choose a reason for hiding this comment

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

The time scaling mode is defined on the operation, no need to define it on each individual column. We also don't do this in other places (and you are not reading the time scaling mode from the column anywhere, only from the operation definition)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point!

@alexwizp alexwizp requested review from flash1293 and removed request for a team September 9, 2022 12:21
@flash1293 flash1293 added release_note:enhancement and removed release_note:feature Makes this part of the condensed release notes labels Sep 9, 2022
@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@flash1293 flash1293 removed the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Sep 12, 2022
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

This code is removing the time scale on every change if there is no date histogram:

Kapture.2022-09-12.at.11.43.10.mp4

@alexwizp
Copy link
Contributor Author

Thank you, @flash1293. I didn't find any regression on removing that timeScale: undefined,

Plus it fixes that case

Screen.Recording.2022-09-12.at.13.27.40.mov

@flash1293
Copy link
Contributor

This mostly works fine, but on testing the details I noticed that we are not taking the reduced time range per dimension into account:
Screenshot 2022-09-13 at 13 12 27

In such a configuration it's fetching the data for the reduced time range, then doing the normalization on the full time range. We could fix this by passing the reduced time range into the lens_time_scale call as a new argument:

In the implementation, the reduced time range is the maximum length (if the search context time range is smaller than the reduced time range, just use it, otherwise cap it by the length of the reduced time range)

Copy link
Contributor

@VladLasitsa VladLasitsa left a comment

Choose a reason for hiding this comment

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

LGTM

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 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
lens 1.2MB 1.2MB -34.0B

Page load bundle

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

id before after diff
lens 34.7KB 34.8KB +32.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @alexwizp

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested and everything works as expected now, LGTM

@alexwizp alexwizp merged commit a60b730 into elastic:main Sep 15, 2022
Lens automation moved this from In progress to Done Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.5.0
Projects
No open projects
Lens
  
Done
Development

Successfully merging this pull request may close these issues.

[Lens] Time scaling without date histogram
6 participants