TSVB should use "histogram:maxBars" and "histogram:barTarget" settings for auto instead of a default 100 buckets #83628

alexwizp · 2020-11-18T12:12:41Z

Summary

This PR adds an additional Level of detail option for TSVB on the Time Series tab

The mechanism for obtaining the maximum value of buckets has also been changed. Instead of a hard-coded value of 100 buckets, now we take this value from the uiSettings by reading:

histogram:maxBars - max value for Max Bars input control
histogram:barTarget - default values for Max Bars input control

src/plugins/vis_type_timeseries/server/lib/vis_data/helpers/calculate_auto.js was removed. We reused the same code from data plugin

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)

For maintainers

This was checked for breaking API changes and was labeled appropriately

alexwizp · 2020-11-18T14:44:44Z

@elasticmachine merge upstream

alexwizp · 2020-11-19T07:42:30Z

@elasticmachine merge upstream

wylieconlon

In this first round of comments I'm mostly commenting on the behavior. Here's what I noticed:

I think it would be save to increase the histogram limit well past 100 by default. Why not set it as high as 65k, which is the limit in ES for now?
Similar to the issues we saw with auto histograms for numbers, it's confusing that the slider doesn't actually change anything automatically. What we decided for Lens was that an unlabeled slider with 5 "steps" is a pretty good way of doing this
I'd like to tweak the form design even more, maybe we can use the idea of the "granularity" slider that Lens has?

wylieconlon · 2020-11-19T23:03:42Z

src/plugins/vis_type_timeseries/server/lib/vis_data/helpers/get_bucket_size.js


-  return calculateAuto.near(100, duration).asSeconds();
+  return calcAutoIntervalLessThan(maxBars, timerange).asSeconds();


Okay so if I understand correctly this is the core part of the change, previously auto was hardcoded to 100, and now it's up to 100?

Yes that true. Now we read histogram:maxBars configuration property as a limit value. The user can change it if necessary.

…default 120 buckets Closes: elastic#54012

alexwizp · 2020-11-20T12:33:53Z

@timroes @wylieconlon Today I tried various options for making this functionality obvious to users. What do you think about next option:

regardless of the histogram:maxBars value in the configuration, we will work with a percentage representation. In this case, a step of 10% of the histogram:maxBars value looks like a good solution from the UI perspective.

timroes · 2020-11-23T09:57:31Z

@alexwizp I like linking this up to the existing advanced settings. I also support Wylie's ask here for control of the level of detail. On the specific UI: I guess I'd not put a percentage on this slider, since it might cause confusion again: "What does 100% mean?" "So 50% will always be half of the 100%?" (which due to rounding it might not afaik).

I guess I would just label the two sides of the slider "Coarse" and "Finest" and don't give the actual values within a label in the UI.

alexwizp · 2020-11-23T12:20:04Z

Thank you @timroes and @wylieconlon. PR was updated

elasticmachine · 2020-11-23T13:08:05Z

Pinging @elastic/kibana-app (Team:KibanaApp)

alexwizp · 2020-11-23T13:26:27Z

@elasticmachine merge upstream

src/plugins/data/server/index.ts

wylieconlon

I'm really liking the direction of this improvement! I've left a few more comments about the behavior and labeling, but I think I don't need to do another code review to approve the direction.

Before merging, please make sure the title and description are appropriate to be put directly into the release notes: https://www.elastic.co/guide/en/kibana/7.10/release-notes-7.10.0.html

This includes the checklist!

src/plugins/vis_type_timeseries/public/application/components/index_pattern.js

wylieconlon · 2020-11-23T23:01:16Z

src/plugins/vis_type_timeseries/public/application/components/index_pattern.js

+                      isEntireTimeRangeActive(model, isTimeSeries) ||
+                      !(model[intervalName] === AUTO_INTERVAL || !model[intervalName])
+                    }
+                    min={0}


This value is definitely wrong because it will never generate data. I think a value of 1 is probably too low as well. What do you think about 10?

@wylieconlon I needed it earlier to correctly mark the scale from 0 to 100 with a step of 10. When entering a number other than 0, the scale would be shifted. I handle that case here:

Let's create a constant to make it a little obvious

Unfortunately even without a scale I cannot set1as a min value cause the max - min value must be a multiple of the step property, otherwise I see an error. Looks like a problem with EUI component.

Also I'm not sure that 10 is a good default value for this property. I think for some cases 1 value can be useful and allows to emulate "Entire timerange mode" which we have on other tabs in TSVB. Please keep in mind that to see data for that case you should set

…index_pattern.js Co-authored-by: Wylie Conlon <wylieconlon@gmail.com>

# Conflicts: # src/plugins/data/server/server.api.md

lukeelmers

Updates to aggs service LGTM, thanks @alexwizp! Code review only

kibanamachine · 2020-11-24T15:51:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 53a2235

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
`visTypeTimeseries`	1.8MB	1.8MB	+3.6KB

Distributable file count

id	before	after	diff
`default`	43051	43050	-1
`oss`	22583	22582	-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`	976.0KB	977.4KB	+1.4KB
`visTypeTimeseries`	137.1KB	137.8KB	+698.0B
total			+2.0KB

History

💚 Build #89871 succeeded 6bd8321
💚 Build #89863 succeeded 9eb3d51
💚 Build #89742 succeeded 6ffe8f6
💔 Build #89718 failed 5223c4a
💚 Build #89596 succeeded d0c9625

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

…s for auto instead of a default 100 buckets (elastic#83628) * TSVB needs a "tsvb:max_buckets" target setting for auto instead of a default 120 buckets Closes: elastic#54012 * remove calculate_auto * max bars -> Level of detail * rename allowLevelofDetail * fix PR comment * Update constants.ts * Update src/plugins/vis_type_timeseries/public/application/components/index_pattern.js Co-authored-by: Wylie Conlon <wylieconlon@gmail.com> * create LEVEL_OF_DETAIL_MIN_BUCKETS constant * calcAutoIntervalLessThan -> search.aggs.calcAutoIntervalLessThan Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Wylie Conlon <wylieconlon@gmail.com> # Conflicts: # src/plugins/data/server/server.api.md

stratoula

LGTM, didnt test it locally

* master: (41 commits) [Maps] fix code-owners (elastic#84265) [@kbn/utils] Clean target before build (elastic#84253) [code coverage] collect for oss integration tests (elastic#83907) [APM] Use `asTransactionRate` consistently everywhere (elastic#84213) Attempt to fix incremental build error (elastic#84152) Unskip "Copy dashboards to space" (elastic#84115) Remove expressions.legacy from README (elastic#79681) Expression: Add render mode and use it for canvas interactivity (elastic#83559) [deb/rpm] Move systemd service to /usr/lib/systemd/system (elastic#83571) [Security Solution][Resolver] Allow a configurable entity_id field (elastic#81679) [ML] Space permision checks for job deletion (elastic#83871) [build] Provide ARM build of RE2 (elastic#84163) TSVB should use "histogram:maxBars" and "histogram:barTarget" settings for auto instead of a default 100 buckets (elastic#83628) [Workplace Search] Initial rendering of Org Sources (elastic#84164) update geckodriver to 0.28 (elastic#84085) Fix timelion vis escapes single quotes (elastic#84196) [Security Solution] Fix incorrect time for dns histogram (elastic#83532) [DX] Bump TS version to v4.1 (elastic#83397) [Security Solution] Add endpoint policy revision number (elastic#83982) [Fleet] Integration Policies List view (elastic#83634) ...

…s for auto instead of a default 100 buckets (#83628) (#84250) * TSVB needs a "tsvb:max_buckets" target setting for auto instead of a default 120 buckets Closes: #54012 * remove calculate_auto * max bars -> Level of detail * rename allowLevelofDetail * fix PR comment * Update constants.ts * Update src/plugins/vis_type_timeseries/public/application/components/index_pattern.js Co-authored-by: Wylie Conlon <wylieconlon@gmail.com> * create LEVEL_OF_DETAIL_MIN_BUCKETS constant * calcAutoIntervalLessThan -> search.aggs.calcAutoIntervalLessThan Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Wylie Conlon <wylieconlon@gmail.com> # Conflicts: # src/plugins/data/server/server.api.md

alexwizp requested review from wylieconlon and stratoula November 19, 2020 13:42

wylieconlon reviewed Nov 19, 2020

View reviewed changes

alexwizp added 2 commits November 20, 2020 12:54

TSVB needs a "tsvb:max_buckets" target setting for auto instead of a …

898a6ae

…default 120 buckets Closes: elastic#54012

remove calculate_auto

3049392

alexwizp force-pushed the 54012 branch from d019ebe to 3049392 Compare November 20, 2020 09:55

max bars -> Level of detail

72683ee

rename allowLevelofDetail

6eadda1

alexwizp force-pushed the 54012 branch from bc6713a to 6eadda1 Compare November 20, 2020 12:50

alexwizp requested a review from sulemanof November 20, 2020 12:50

alexwizp added 2 commits November 23, 2020 14:02

fix PR comment

064f832

Merge remote-tracking branch 'upstream/master' into 54012

21a91aa

alexwizp changed the title ~~TSVB needs a "tsvb:max_buckets" target setting for auto instead of a default 120 buckets~~ TSVB should use "histogram:maxBars" and "histogram:barTarget" settings for auto instead of a default 100 buckets Nov 23, 2020

alexwizp requested a review from wylieconlon November 23, 2020 12:57

alexwizp marked this pull request as ready for review November 23, 2020 13:07

alexwizp requested a review from a team November 23, 2020 13:07

alexwizp requested a review from a team as a code owner November 23, 2020 13:07

alexwizp self-assigned this Nov 23, 2020

alexwizp added release_note:enhancement Feature:TSVB TSVB (Time Series Visual Builder) Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Nov 23, 2020

alexwizp added v7.11.0 v8.0.0 labels Nov 23, 2020

kibanamachine and others added 3 commits November 23, 2020 08:26

Merge branch 'master' into 54012

d0c9625

Merge branch 'master' into 54012

5223c4a

Update constants.ts

6ffe8f6

lukeelmers reviewed Nov 23, 2020

View reviewed changes

src/plugins/data/server/index.ts Outdated Show resolved Hide resolved

wylieconlon approved these changes Nov 23, 2020

View reviewed changes

Update src/plugins/vis_type_timeseries/public/application/components/…

9eb3d51

…index_pattern.js Co-authored-by: Wylie Conlon <wylieconlon@gmail.com>

alexwizp force-pushed the 54012 branch from 7fde2b3 to 9eb3d51 Compare November 24, 2020 06:51

alexwizp added 2 commits November 24, 2020 10:31

create LEVEL_OF_DETAIL_MIN_BUCKETS constant

ac08751

calcAutoIntervalLessThan -> search.aggs.calcAutoIntervalLessThan

6bd8321

alexwizp requested a review from lukeelmers November 24, 2020 11:21

Merge remote-tracking branch 'upstream/master' into 54012

53a2235

# Conflicts: # src/plugins/data/server/server.api.md

lukeelmers approved these changes Nov 24, 2020

View reviewed changes

alexwizp merged commit 6ef6c0f into elastic:master Nov 24, 2020

alexwizp mentioned this pull request Nov 24, 2020

[7.x] TSVB should use "histogram:maxBars" and "histogram:barTarget" settings for auto instead of a default 100 buckets (#83628) #84250

Merged

stratoula reviewed Nov 24, 2020

View reviewed changes

alexwizp mentioned this pull request Dec 10, 2020

[TSVB] Fields lists do not populate all the times #85530

Merged

flash1293 mentioned this pull request Dec 18, 2020

[Lens] Granularity slider for date histogram interval #86437

Open

wylieconlon mentioned this pull request Mar 9, 2021

[TSVB] Remove metrics:max_buckets setting because it is redundant to histogram:maxBars #94212

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TSVB should use "histogram:maxBars" and "histogram:barTarget" settings for auto instead of a default 100 buckets #83628

TSVB should use "histogram:maxBars" and "histogram:barTarget" settings for auto instead of a default 100 buckets #83628

alexwizp commented Nov 18, 2020 •

edited

Loading

alexwizp commented Nov 18, 2020

alexwizp commented Nov 19, 2020

wylieconlon left a comment

wylieconlon Nov 19, 2020

alexwizp Nov 20, 2020

alexwizp commented Nov 20, 2020 •

edited

Loading

timroes commented Nov 23, 2020

alexwizp commented Nov 23, 2020

elasticmachine commented Nov 23, 2020

alexwizp commented Nov 23, 2020

wylieconlon left a comment •

edited

Loading

wylieconlon Nov 23, 2020

alexwizp Nov 24, 2020

lukeelmers left a comment

kibanamachine commented Nov 24, 2020

stratoula left a comment


		return calculateAuto.near(100, duration).asSeconds();
		return calcAutoIntervalLessThan(maxBars, timerange).asSeconds();

TSVB should use "histogram:maxBars" and "histogram:barTarget" settings for auto instead of a default 100 buckets #83628

TSVB should use "histogram:maxBars" and "histogram:barTarget" settings for auto instead of a default 100 buckets #83628

Conversation

alexwizp commented Nov 18, 2020 • edited Loading

Summary

Checklist

For maintainers

alexwizp commented Nov 18, 2020

alexwizp commented Nov 19, 2020

wylieconlon left a comment

Choose a reason for hiding this comment

wylieconlon Nov 19, 2020

Choose a reason for hiding this comment

alexwizp Nov 20, 2020

Choose a reason for hiding this comment

alexwizp commented Nov 20, 2020 • edited Loading

timroes commented Nov 23, 2020

alexwizp commented Nov 23, 2020

elasticmachine commented Nov 23, 2020

alexwizp commented Nov 23, 2020

wylieconlon left a comment • edited Loading

Choose a reason for hiding this comment

wylieconlon Nov 23, 2020

Choose a reason for hiding this comment

alexwizp Nov 24, 2020

Choose a reason for hiding this comment

lukeelmers left a comment

Choose a reason for hiding this comment

kibanamachine commented Nov 24, 2020

💚 Build Succeeded

Metrics [docs]

Async chunks

Distributable file count

Page load bundle

History

stratoula left a comment

Choose a reason for hiding this comment

alexwizp commented Nov 18, 2020 •

edited

Loading

alexwizp commented Nov 20, 2020 •

edited

Loading

wylieconlon left a comment •

edited

Loading