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] Add reduced time range option #136706

Merged
merged 21 commits into from
Aug 3, 2022
Merged

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Jul 20, 2022

Fixes #132112

This PR adds a "reduced time range" option to all metrics operations - if applied, only values which are within the specified window (anchored at the end of the selected time range) are aggregated for the final metric. The window is specified in the same way as the time shift interval, but without any restrictions.

The label is adjusted automatically when the window changes

It can be applied together with time shift - in this case the window is shifted in the same way as the base time range.

This option can't be applied if a date histogram is used (Once TSDB sliding window lands, it can be translated into using sliding window but at the moment Elasticsearch can't handle this functionality) or if the current data view doesn't have a default time field (because there isn't a reference time frame to anchor the interval in this case).

Screenshot 2022-07-27 at 11 18 43

Screenshot 2022-07-27 at 11 18 49

Screenshot 2022-07-27 at 11 19 05

Screenshot 2022-07-27 at 11 19 18

Screenshot 2022-07-27 at 11 17 19

This does not work for counter rate, as the Lens counter rate can only be used with a date histogram which is incompatible with the window filter without TSDB support. However, it can be done using a formula even today with these features:
Growth rate per second of my_counter_field in the last 5 mins as a metric

pick_max(0, max(my_counter_field, timeRange="5m") - max(my_counter_field, timeRange="5m", shift="5m")) / 5 * 60

Once TSDB aggs land, it will be a simple "Sum of my_counter_field with a window of 5m"

Implementation

On Lens side this is implemented similar to the time shift option.

On AggConfig side this is part of the existing filter agg config - it takes a window and translates it into a filter based on the current time range and the default time field of the index pattern.

@flash1293 flash1293 added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens backport:skip This commit does not require backporting v8.4.0 labels Jul 20, 2022
@KOTungseth KOTungseth added the ui-copy Review of UI copy with docs team is recommended label Jul 21, 2022
@KOTungseth
Copy link
Contributor

179935458-d40f7f9b-4429-4609-a70c-7a00817e3196

Instead of Window, how about Time interval?
We use the following for other Lens time configurations:
Screen Shot 2022-07-21 at 9 13 48 AM

For the tooltip, we could use Select the time interval for the metric.

For the Time shift tooltip, I would use Select an option or create a custom value. Examples: 30s, 20m, 24h, 2d, 1w, 1M.

179935460-f6c66ec3-45ac-45ba-b7f5-a8bf8eebdd6d

Instead of Window value is not valid., I would use Time interval is invalid. Select a valid option.

179935464-e974257b-d6d1-4c8f-ac31-ddaed1aafbe5

Do we have a default value for this setting? If yes, I would replace Type custom values (e.g. 8w) with the default value.

179948037-afa8aa34-a68c-4389-8a6a-794e043b118b

Instead of Window can only be used without a date histogram and with a specified default time field on the data view, I would use Date histogram is unsupported by time interval. You must use a data view with a default time field.

@flash1293
Copy link
Contributor Author

flash1293 commented Jul 21, 2022

Thanks for taking a look, @KOTungseth

Instead of Window, how about Time interval?
We use the following for other Lens time configurations:

Unfortunately this is conceptually something very different than the interval of the date histogram - they are the same in the sense that they both describe a time duration, but the window option is about defining the window of time that should be considered at all for this aggregation while the interval is about slicing the whole time range into even buckets. I'm concerned users would try to configure the actual date histogram interval within the metric configuration because it uses the same name. Open to find a better word than "window", but it should be clear what this is about.

There is no default value for the window - by default, no window is applied and data from the whole time range (the one configured in the top right) is taken into account

@ghudgins
Copy link
Contributor

ghudgins commented Jul 26, 2022

Agree with @flash1293 that Time Interval is problematic and I agree with @KOTungseth that Window is problematic. Really this is the same as "Filter by" but for a time field (thus needing a different affordance)

My bad ideas:

  • Additional time window
  • Additional recent window
  • Additional time filter
  • Filter by time
  • Filter by recent data

maybe they provoke something?

@flash1293
Copy link
Contributor Author

I agree that it’s very “filtery” but it’s also closely related to the time range. What about “Reduced time range” with a description below the input “Additional time range filter aligned with the end of the global time range”?

@ghudgins
Copy link
Contributor

Reduced time range

nice - that sounds pretty good to me.

@flash1293 flash1293 changed the title [Lens] Add window option [Lens] Add reduced time range option Jul 27, 2022
@KOTungseth
Copy link
Contributor

I agree that it’s very “filtery” but it’s also closely related to the time range. What about “Reduced time range” with a description below the input “Additional time range filter aligned with the end of the global time range”?

How about:

Reduced time range
Reduces the time range specified in the global time filter.

@flash1293
Copy link
Contributor Author

Noting down the things discussed: We were also pushing this around a bit in our sync a bit, but we didn't come to a good conclusion. The biggest problem with "Reduce time range" is that it doesn't make it obvious this reduced time range is aligned with the end of the global time range. I think it's important to keep it in there somehow. Do you have an idea for that, @KOTungseth ?

@flash1293 flash1293 marked this pull request as ready for review July 27, 2022 15:54
@flash1293 flash1293 requested review from a team as code owners July 27, 2022 15:54
@elasticmachine
Copy link
Contributor

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

@stratoula stratoula removed the v8.4.0 label Aug 2, 2022
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.

Great addition! This works great, LGTM! I find the term Reduced time range a very good choice and with a small help from the description is easy to understand what this is for. 👍

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Only read the code, overall changes LGTM great work @flash1293

@ghudgins
Copy link
Contributor

ghudgins commented Aug 2, 2022

Reduces the time range specified in the global time filter. --> Reduces the time range specified in the global time filter from the end of the global time filter.

I know it's getting late but this I think amends @KOTungseth's text to account for that extra piece of info

@flash1293
Copy link
Contributor Author

Adjusted:
Screenshot 2022-08-03 at 12 24 19

@flash1293 flash1293 enabled auto-merge (squash) August 3, 2022 10:38
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 898 900 +2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
data 2420 2423 +3

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 +6.5KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
data 21 22 +1

Page load bundle

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

id before after diff
data 428.4KB 429.0KB +664.0B
Unknown metric groups

API count

id before after diff
data 3097 3100 +3

History

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

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 ui-copy Review of UI copy with docs team is recommended v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Window config for last value, counter rate and average/percentile
8 participants