-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Random sampling feature #143221
[Lens] Random sampling feature #143221
Conversation
@@ -16,6 +16,15 @@ import type { DataView } from '@kbn/data-views-plugin/common'; | |||
import { stubIndexPattern } from '../../stubs'; | |||
import { IEsSearchResponse } from '..'; | |||
|
|||
// Mute moment.tz warnings about not finding a mock timezone | |||
jest.mock('../utils', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debugging test failures was really hard with all warning from moment.tz
. This fixed the issue assigning a fixed (supported) timezone for moment
.
@@ -427,11 +427,11 @@ export function insertTimeShiftSplit( | |||
const timeRange = aggConfigs.timeRange; | |||
const filters: Record<string, unknown> = {}; | |||
const timeField = aggConfigs.timeFields[0]; | |||
const timeFilter = getTime(aggConfigs.indexPattern, timeRange, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This computation could be done once for all the timeShifts
Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors) |
There was a problem hiding this 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, LGTM. Still think we should add some in-product documentation, but we can do that as a follow-up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, @dej611! I've left you a few small comments for your review. As none of them are super critical, assuming you're able to address them, I'll go ahead and approve this now so I don't hold you up further.
Just for the sake of visibility, the plan is to follow this with another PR to address the following items that @dej611, @KOTungseth, and I have discussed:
- Switch the slider's
EuiFormRow
fromcolumnCompressed
torowCompressed
to allow the slider to expand to the full available width of the flyout. - Show "Speed" and "Accuracy" labels at each of the slider's respective extremities to emphasize what is happening.
- Change the slider's tick numbers to percentages for easier reading.
- Update help text below the slider to briefly explaining what this feature does (supplied by @KOTungseth) and include a link to the full documentation.
x-pack/plugins/lens/public/datasources/form_based/layer_settings.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/lens/public/datasources/form_based/layer_settings.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/lens/public/datasources/form_based/layer_settings.tsx
Outdated
Show resolved
Hide resolved
.../plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.scss
Outdated
Show resolved
Hide resolved
x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/flyout_container.scss
Show resolved
Hide resolved
x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/flyout_container.tsx
Outdated
Show resolved
Hide resolved
…gs.tsx Co-authored-by: Michael Marcialis <[email protected]>
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
History
To update your PR or re-run it, just comment with: |
isSamplingEnabled() { | ||
return ( | ||
isSamplingEnabled(this.opts.probability) && | ||
this.getRequestAggs().filter((agg) => !agg.type.hasNoDsl).length > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to follow...
Could you please explain this condition?
this.getRequestAggs().filter((agg) => !agg.type.hasNoDsl).length > 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That condition is set to detect a specific case when no sub-agg is available for the sampling, therefore it makes no sense to have it. That's the case of Count
of Records in Lens, where it should just return the number of documents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
App services code lgtm
## Summary Adds the 8.8 documentation for the following: - Enable report sharing: #153429 Docs preview: https://kibana_157215.docs-preview.app.elstc.co/guide/en/kibana/master/reporting-getting-started.html - Random sampling feature: #143221 Docs preview: https://kibana_157215.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#improve-visualization-loading-time - Improve Ignore global filters UI: #154441 and #155280 Docs preview: https://kibana_157215.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#add-annotations --------- Co-authored-by: Tim Sullivan <[email protected]>
## Summary Adds the 8.8 documentation for the following: - Enable report sharing: elastic#153429 Docs preview: https://kibana_157215.docs-preview.app.elstc.co/guide/en/kibana/master/reporting-getting-started.html - Random sampling feature: elastic#143221 Docs preview: https://kibana_157215.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#improve-visualization-loading-time - Improve Ignore global filters UI: elastic#154441 and elastic#155280 Docs preview: https://kibana_157215.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#add-annotations --------- Co-authored-by: Tim Sullivan <[email protected]> (cherry picked from commit 06a800f)
# Backport This will backport the following commits from `main` to `8.8`: - [[DOCS] Adds 8.8 Viz docs (#157215)](#157215) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Kaarina Tungseth","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-05-23T15:41:42Z","message":"[DOCS] Adds 8.8 Viz docs (#157215)\n\n## Summary\r\n\r\nAdds the 8.8 documentation for the following:\r\n\r\n- Enable report sharing: https://github.com/elastic/kibana/pull/153429\r\nDocs preview:\r\nhttps://kibana_157215.docs-preview.app.elstc.co/guide/en/kibana/master/reporting-getting-started.html\r\n\r\n- Random sampling feature: https://github.com/elastic/kibana/pull/143221\r\nDocs preview:\r\nhttps://kibana_157215.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#improve-visualization-loading-time\r\n\r\n- Improve Ignore global filters UI:\r\nhttps://github.com//pull/154441 and\r\nhttps://github.com//pull/155280\r\nDocs preview:\r\nhttps://kibana_157215.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#add-annotations\r\n\r\n---------\r\n\r\nCo-authored-by: Tim Sullivan <[email protected]>","sha":"06a800fbad1f2a8fd9146bfe0c439e2107f771fc","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Docs","release_note:skip","Team:SharedUX","v8.8.0","v8.9.0"],"number":157215,"url":"https://github.com/elastic/kibana/pull/157215","mergeCommit":{"message":"[DOCS] Adds 8.8 Viz docs (#157215)\n\n## Summary\r\n\r\nAdds the 8.8 documentation for the following:\r\n\r\n- Enable report sharing: https://github.com/elastic/kibana/pull/153429\r\nDocs preview:\r\nhttps://kibana_157215.docs-preview.app.elstc.co/guide/en/kibana/master/reporting-getting-started.html\r\n\r\n- Random sampling feature: https://github.com/elastic/kibana/pull/143221\r\nDocs preview:\r\nhttps://kibana_157215.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#improve-visualization-loading-time\r\n\r\n- Improve Ignore global filters UI:\r\nhttps://github.com//pull/154441 and\r\nhttps://github.com//pull/155280\r\nDocs preview:\r\nhttps://kibana_157215.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#add-annotations\r\n\r\n---------\r\n\r\nCo-authored-by: Tim Sullivan <[email protected]>","sha":"06a800fbad1f2a8fd9146bfe0c439e2107f771fc"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/157215","number":157215,"mergeCommit":{"message":"[DOCS] Adds 8.8 Viz docs (#157215)\n\n## Summary\r\n\r\nAdds the 8.8 documentation for the following:\r\n\r\n- Enable report sharing: https://github.com/elastic/kibana/pull/153429\r\nDocs preview:\r\nhttps://kibana_157215.docs-preview.app.elstc.co/guide/en/kibana/master/reporting-getting-started.html\r\n\r\n- Random sampling feature: https://github.com/elastic/kibana/pull/143221\r\nDocs preview:\r\nhttps://kibana_157215.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#improve-visualization-loading-time\r\n\r\n- Improve Ignore global filters UI:\r\nhttps://github.com//pull/154441 and\r\nhttps://github.com//pull/155280\r\nDocs preview:\r\nhttps://kibana_157215.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#add-annotations\r\n\r\n---------\r\n\r\nCo-authored-by: Tim Sullivan <[email protected]>","sha":"06a800fbad1f2a8fd9146bfe0c439e2107f771fc"}}]}] BACKPORT--> Co-authored-by: Kaarina Tungseth <[email protected]>
## Summary Adds the 8.8 documentation for the following: - Enable report sharing: elastic#153429 Docs preview: https://kibana_157215.docs-preview.app.elstc.co/guide/en/kibana/master/reporting-getting-started.html - Random sampling feature: elastic#143221 Docs preview: https://kibana_157215.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#improve-visualization-loading-time - Improve Ignore global filters UI: elastic#154441 and elastic#155280 Docs preview: https://kibana_157215.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#add-annotations --------- Co-authored-by: Tim Sullivan <[email protected]>
Summary
Closes #142958
This PR introduces the random sampling feature in Lens, as a layer setting.
Main features in the PR:
Layer Settings
action/panelDimensionContainer
to be reusable across LensTodo list:
Refactor
DimensionContainer
to be a reusable flyout componentDimensionEditor
use the new flyout componentLayer settings
tech preview
tag to it (used lab icon + tooltip)Make it work for Annotations?No.Remove
random_sampler
agg (originally ported from [Lens] Random sampling #142980)Inject
probability
intoaggConfigs
and use it when less than1
Use SearchSessionId as sampler seed
Other
terms aggCheck time shift when sampling
doc_count
Add tests
Add functional tests
🐛 There's a weird bug I could not reproduce reliably when switching from a
date histogram
to aTop values
as sampling is enabled and some expression fails.Some testing behaviour
Terms
withOther
option enabled check the Inspector that both requests are using the same seedChecklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers