[Lens][TSVB][Agg based] TSDB warning handling support for Lens, Agg based and TSVB #136833

flash1293 · 2022-07-21T13:21:32Z

Fixes #132045

Bonus (because it's very easy to do with the new warning handling): Do not show warnings anymore for suggestions. In almost all cases they are just repeating the warnings from the main chart, rendering the config panel unusable and not adding any meaningful information.

How to test

Check out https:/elastic/elasticsearch/tree/kibana-downsampling-branch next to your kibana working copy
Start ES with yarn es source instead of snapshot (this will build the given branch)
Clone https:/csoulios/downsampling-demo
Run the commands in downsample-index.es (indexing the actual data is too much for dev console, so you have to do this via curl curl -XPOST http://elastic:changeme@localhost:9200/sample-01/_bulk\?pretty -H 'Content-Type: application/json' --data-binary @sample-01-raw.json)
After running all commands, there should be sample-01-rollup which has the rolled up data
Create a data view sample-01 for testing TSDB without rollups and a data view sample-01,sample-01-rollup for testing TSDB with rollups

Behavior

In the Lens editor, rollup errors are shown as warnings in the existing warning menu. Lens warnings get a nice message explaining what's going on.

On dashboard, a warning indicator is rendered in the bottom right of the vis

For TSVB and agg based, the warning indicator is rendered both in the editor and on the dashboard. The error message coming from Elasticsearch is not rewritten in the context of the chart.

If there are other shard failures than rollup related ones, the existing warning toast is still shown (in this case the shard failures modal is also including the failures originating from rollup if both are happening at the same time)

To cause the other shard failures, extend the data view to include another index name besides sample-01 and sample-01-rollup, then index a document which auto-sets a used number field to text, e.g. like this:

POST other-trickery-1/_doc
{
  "kubernetes.container.memory.available.bytes": "abc",
  "@timestamp": "2022-06-18"
}

This will cause a generic illegal argument exception which should not be handled locally.

Implementation details

Previously the warning toasts would be shown by the esaggs function implementation automatically. This PR changes this in the following way:

Disable the automatic handling by passing disableShardWarnings to the search context
In the embeddable (visualize and Lens), call showWarnings with the adapters and pass a callback which filters out rollup-related warnings

Due to this, the way shard failure warnings can be suppressed for TSVB needs to be changed:

Do not call handleWarnings from within the TSVB request handler anymore - the visualize embeddable does that already
Add a suppressWarnings function to the vis type definition which is checked in the embeddable whether the warning should be shown or not

…pported functions

elasticmachine · 2022-09-01T11:17:18Z

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

stratoula · 2022-09-05T12:43:54Z

@flash1293 thanx a ton for the guidelines!
This works great in Lens and TSVB. I just noticed that the warning is wrong in agg based
Check here

I have selected median but the warning displays percentiles

Update: If I select percentile_ranks though the warning is correct 🤔

flash1293 · 2022-09-05T12:50:43Z

@stratoula that's expected - if you check the request then you'll see that Elasticsearch doesn't have a special "median" agg - it was a percentiles agg with a single percentage 50 the whole time:

kibana/src/plugins/data/common/search/aggs/metrics/median.ts

Line 28 in 3730dd0

dslName: 'percentiles',

stratoula · 2022-09-05T12:52:16Z

Hahaha got it! Thanx Joe! Finishing some more tests and approving (I love the image btw :D )

stratoula

Code LGTM! This works great! I have tested all the editors following the steps. Love the bonus too ;)

MichaelMarcialis

@flash1293: I've left you some comments below for your review.

Additionally, I was curious about that shard failure modal you showed. Is this modal something that we created/control? If so, why the decision to put this content in an EuiModal versus an EuiFlyout? I imagine that a flyout is far more scalable for this sort of content content, which has the potential to get fairly lengthy (since flyouts natively support scrolling).

Also, I found the contents of that modal to be a bit confusing. It almost looks as though "Reason" is being listed multiple times for each entry (table column, bold text in open accordion, row in open accordion), with the last being a different message than the first two. Are they supposed to all be the same? If so, why are we showing it three times?

src/plugins/charts/public/static/components/warnings.tsx

.../plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/warnings_popover.scss

flash1293 · 2022-09-07T08:00:50Z

Thanks for the review @MichaelMarcialis .

Shard failure modal

This component existed before and wasn't changed on this PR. I see your point about turning it into a flyout, but I would like to split this out. The component is owned by the app services team.

Inline warning

The notification badge was just some random idea - I like your suggestion. This is how it looks:

Could you review again?

…-ref HEAD~1..HEAD --fix'

MichaelMarcialis

Thanks for making those changes, @flash1293. Left one additional question/comment for you, but nothing earth-shattering. Approving now so I don't hold you up further.

MichaelMarcialis · 2022-09-07T14:49:44Z

src/plugins/charts/public/static/components/warnings.tsx

+ <div style={{ maxWidth: 512 }}>
+ {warnings.map((w, i) => (
+ <React.Fragment key={i}>
+ {w}
+ <br />
+ </React.Fragment>
+ ))}
+ </div>


Are these warnings just being nested in a div in the warning popover, and using a br to space them out? If so, would it be better to house each warning item in something like an EuiText and use some CSS padding and borders to separate each warning in the list? That way it looks similar to how warnings were intended to look in the Lens warning menu.

Old warning popover mockup:

…ens-tsdb-rollup

flash1293 · 2022-09-08T14:56:21Z

Styled it a bit, but happy to improve in a follow-up:

kibana-ci · 2022-09-08T15:59:39Z

💚 Build Succeeded

Buildkite Build
Commit: 2d769aa

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`charts`	90	91	+1

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
`charts`	246	249	+3
`visualizations`	389	390	+1
total			+4

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	+2.3KB
`visualizations`	197.5KB	198.4KB	+881.0B
total			+3.1KB

Page load bundle

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

id	before	after	diff
`charts`	44.4KB	45.2KB	+812.0B
`visTypeTimeseries`	18.9KB	18.8KB	-34.0B
`visualizations`	47.3KB	47.5KB	+213.0B
total			+991.0B

Unknown metric groups

API count

id	before	after	diff
`charts`	261	264	+3
`visualizations`	417	419	+2
total			+5

History

💚 Build #69939 succeeded d99299a
💚 Build #69794 succeeded ed274f4
💚 Build #69302 succeeded 34bcac7
💛 Build #69010 was flaky d792e40
💛 Build #68961 was flaky b8ffebb
💔 Build #68944 failed ddd79ad

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

…ased and TSVB (elastic#136833) * start integrating TSDB into Lens * current state * make new type filter work * fix translations * handle warnings * clean up and make warnings better * split function list for referenced fields too and extend list of unsupported functions * improve warnings * fix a bug * add todo for utc timezone * adding TSDB support to DataViews * adding test * date_histogram: force fixed interval for tsdb rollups * fixing types * fixing types * fixing types * fixing types * handling edge cases * removing undefined entries from FieldSpec * renaming .meta property * enforce UTC timezone * isRolledUpField helper * updating snapshot * adjust fixed interval and utc timezone for timelion and tsvb * fixed interval mapping * fixing test * add showWarning to search service * add comments * add unit tests * test foo * cleanup * add s to property name in test * comments for api items * fixing test * use the warnings when calling showWarnings * change showWarning to just show a single warning * put handleWarnings on the request adapter * comment * simplify 1 * fix lens unit test * fix PR * remove duplicate identifier * remove underscoring for unused variables * revert inspector changes, extract the response warnings in the search service * fix bug * remove console.log * re-apply typescript fixes to app test code * declutter * add test, improve comments * fix some unexported public api items * include rawResponse in the warning structure * fix lint * tweak clean up example app * SearchResponseWarnings and SearchResponseWarningNotification * fix export bug * not include shardStats if there are no warnings * Update src/plugins/data/common/search/types.ts * fix duplicated entries * remove empty line * simplify SearchResponseWarnings interface * remove array copying * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * comments for api_docs * simplify per feedback * Pass callback to handleResponse in showWarnings * export more public types * update example to make possible to show shard failure * pr cleanup * eslint fix * allow example app to not show default warnings * move extractWarning and related types to inspector plugin * wip functional test of example app * fix test references * finish functional test * relocate extractWarnings back to search/fetch * fix test * remove need for isTimeout, isShardFailure * ts fix * improve test * Change showWarnings to accept the RequestAdapter * use showWarnings in vis_types/timeseries * more tests * use handle_warning name * fix ts * add reason field to SearchResponseWarning * fix component snapshot * update comments * test cleanup * fix test * ensure notification appears only once * fix and cleanup * fix ts * fix response.json bug * use top-level type, and lower-level reason.type * cleanup * change warning structure * change warning infrastructure * fix * allows to disable automatic handling of shard warnings on expressions * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * fix tests * fix tsconfig json * allows to disable automatic handling of shard warnings on expressions * fix tests * allows to disable automatic handling of shard warnings on expressions * disable warnings the right way * fix stuff and clean up * random cleanup * fix test * remove unused function * review comments * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * style warnings and switch to real error message Co-authored-by: ppisljar <[email protected]> Co-authored-by: Timothy Sullivan <[email protected]> Co-authored-by: Tim Sullivan <[email protected]> Co-authored-by: kibanamachine <[email protected]>

start integrating TSDB into Lens

8cd700e

flash1293 added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens labels Jul 21, 2022

flash1293 and others added 27 commits July 27, 2022 16:59

Merge remote-tracking branch 'upstream/main' into lens-tsdb-rollup

7ba02d5

current state

ac12e1b

make new type filter work

c8f3004

fix translations

8042e78

Merge remote-tracking branch 'upstream/main' into lens-tsdb-rollup

2751e37

handle warnings

6aa932e

clean up and make warnings better

a4ec322

split function list for referenced fields too and extend list of unsu…

b414817

…pported functions

improve warnings

ed718c4

fix a bug

cca9c14

add todo for utc timezone

312e87c

Merge remote-tracking branch 'upstream/main' into lens-tsdb-rollup

ad396a1

adding TSDB support to DataViews

e25ff1f

adding test

cbfd29c

date_histogram: force fixed interval for tsdb rollups

c592552

fixing types

56305ef

fixing types

dcbf1a4

fixing types

6fe93b7

fixing types

26829dc

handling edge cases

922af99

removing undefined entries from FieldSpec

15c11a6

renaming .meta property

d51c240

enforce UTC timezone

c251280

isRolledUpField helper

9b70f52

updating snapshot

7611070

Merge remote-tracking branch 'upstream/main' into lens-tsdb-rollup

cb5c749

adjust fixed interval and utc timezone for timelion and tsvb

c708a9f

flash1293 added the release_note:enhancement label Sep 1, 2022

flash1293 changed the title ~~[Lens] TSDB support for Lens~~ [Lens][TSVB][Agg based] TSDB warning handling support for Lens, Agg based and TSVB Sep 1, 2022

remove unused function

d792e40

flash1293 marked this pull request as ready for review September 1, 2022 11:17

flash1293 requested review from a team as code owners September 1, 2022 11:17

Merge branch 'main' into lens-tsdb-rollup

34bcac7

stratoula approved these changes Sep 5, 2022

View reviewed changes

Merge branch 'main' into lens-tsdb-rollup

ed274f4

MichaelMarcialis reviewed Sep 6, 2022

View reviewed changes

src/plugins/charts/public/static/components/warnings.tsx Outdated Show resolved Hide resolved

.../plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/warnings_popover.scss Outdated Show resolved Hide resolved

flash1293 added 2 commits September 7, 2022 09:17

Merge remote-tracking branch 'upstream/main' into lens-tsdb-rollup

539d425

review comments

8906ca1

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

d99299a

…-ref HEAD~1..HEAD --fix'

MichaelMarcialis approved these changes Sep 7, 2022

View reviewed changes

flash1293 added 3 commits September 8, 2022 16:36

Merge remote-tracking branch 'upstream/main' into lens-tsdb-rollup

76ca7cc

style warnings and switch to real error message

ac9ecd1

Merge branch 'lens-tsdb-rollup' of github.com:flash1293/kibana into l…

2d769aa

…ens-tsdb-rollup

flash1293 enabled auto-merge (squash) September 8, 2022 14:56

flash1293 merged commit e7a8c87 into elastic:main Sep 8, 2022

kibanamachine added v8.5.0 backport:skip This commit does not require backporting labels Sep 8, 2022

drewdaemon mentioned this pull request Oct 26, 2022

[Lens] embeddable warnings placement looks pretty bad with a metric. #144027

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens][TSVB][Agg based] TSDB warning handling support for Lens, Agg based and TSVB #136833

[Lens][TSVB][Agg based] TSDB warning handling support for Lens, Agg based and TSVB #136833

flash1293 commented Jul 21, 2022 •

edited

Loading

elasticmachine commented Sep 1, 2022

stratoula commented Sep 5, 2022 •

edited

Loading

flash1293 commented Sep 5, 2022 •

edited

Loading

stratoula commented Sep 5, 2022

stratoula left a comment

MichaelMarcialis left a comment

flash1293 commented Sep 7, 2022

MichaelMarcialis left a comment

MichaelMarcialis Sep 7, 2022

flash1293 commented Sep 8, 2022

kibana-ci commented Sep 8, 2022

API count

[Lens][TSVB][Agg based] TSDB warning handling support for Lens, Agg based and TSVB #136833

[Lens][TSVB][Agg based] TSDB warning handling support for Lens, Agg based and TSVB #136833

Conversation

flash1293 commented Jul 21, 2022 • edited Loading

How to test

Behavior

Implementation details

elasticmachine commented Sep 1, 2022

stratoula commented Sep 5, 2022 • edited Loading

flash1293 commented Sep 5, 2022 • edited Loading

stratoula commented Sep 5, 2022

stratoula left a comment

Choose a reason for hiding this comment

MichaelMarcialis left a comment

Choose a reason for hiding this comment

flash1293 commented Sep 7, 2022

Shard failure modal

Inline warning

MichaelMarcialis left a comment

Choose a reason for hiding this comment

MichaelMarcialis Sep 7, 2022

Choose a reason for hiding this comment

flash1293 commented Sep 8, 2022

kibana-ci commented Sep 8, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

flash1293 commented Jul 21, 2022 •

edited

Loading

stratoula commented Sep 5, 2022 •

edited

Loading

flash1293 commented Sep 5, 2022 •

edited

Loading