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

[ML] Add search time runtime support for index based Data Visualizer #95252

Merged
merged 13 commits into from
Mar 29, 2021

Conversation

qn895
Copy link
Member

@qn895 qn895 commented Mar 23, 2021

Summary

Part of #77462. This PR:

  • Adds search time runtime fields support index-based Data Visualizer

Screen Shot 2021-03-23 at 17 55 44

  • Handles saved search or search queries with runtime fields
    2021-03-25 at 14 54

  • Adds capability for the histogram preview to show runtime fields within Transform/Data Frame Analytics.

Before

Screen Shot 2021-03-23 at 18 01 14

After

Screen Shot 2021-03-23 at 18 01 55

  • Adds better error message when a query fails (e.g. due to bad scripts or runtime field definitions)

Screen Shot 2021-03-25 at 14 40 29

Checklist

Delete any items that are not applicable to this PR.

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@qn895 qn895 mentioned this pull request Mar 23, 2021
12 tasks
Copy link
Contributor

@peteharverson peteharverson 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 LGTM.

The only issue I found was when using a saved search that uses a runtime field defined in the index pattern, where the 'Use full data' button gives an error, but that should be fixed by #94760. Can you please double check that after rebasing.

@@ -629,7 +645,10 @@ export class DataVisualizer {
cardinalityField = {
cardinality: { field },
};
runtimeMappings.runtime_mappings = datafeedConfig.runtime_mappings;
runtimeMappings.runtime_mappings = {
Copy link
Member

@jgowdyelastic jgowdyelastic Mar 24, 2021

Choose a reason for hiding this comment

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

In my recent PR #94760 i've removed this else if altogether and added the population of the runtime_mappings to the else below.

but looking at this loop again, i think the change you've made on line 629 is correct. We should always add runtime_mappings outside of this loop, if they have been supplied.

In short, this if else isn't needed as we've populated them on line 629.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved this to outside of the loop here 1e708e1 (#95252)

@@ -612,7 +626,9 @@ export class DataVisualizer {
// Value count aggregation faster way of checking if field exists than using
// filter aggregation with exists query.
const aggs: Aggs = datafeedAggregations !== undefined ? { ...datafeedAggregations } : {};
const runtimeMappings: { runtime_mappings?: RuntimeMappings } = {};
const runtimeMappings: { runtime_mappings?: RuntimeMappings } = runtimeFields
Copy link
Member

Choose a reason for hiding this comment

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

it would be good to use a isPopulatedObject check here too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated here 1e708e1 (#95252)

@qn895
Copy link
Member Author

qn895 commented Mar 24, 2021

Tested and LGTM.

The only issue I found was when using a saved search that uses a runtime field defined in the index pattern, where the 'Use full data' button gives an error, but that should be fixed by #94760. Can you please double check that after rebasing.

Thanks for testing! Looks like the issue is fixed after #94760:

Screen Shot 2021-03-24 at 10 49 47

@@ -602,7 +615,8 @@ export class DataVisualizer {
timeFieldName: string,
earliestMs?: number,
latestMs?: number,
datafeedConfig?: Datafeed
datafeedConfig?: Datafeed,
runtimeFields?: RuntimeMappings
Copy link
Member

Choose a reason for hiding this comment

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

nit, should be called runtimeMappings for consistency

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated here a6ae3ce (#95252)

@peteharverson
Copy link
Contributor

I am still seeing an error when selecting a saved search which uses a runtime field from the index pattern, and then hitting the 'Use full data' button. I am also seeing it in the job wizards, even after #94760. The time_field_range endpoint is returning

{"success":true,"start":{"epoch":null},"end":{"epoch":null}}

I have added it as an additional item to #77462 - we can address it separately to this PR.

@qn895
Copy link
Member Author

qn895 commented Mar 25, 2021

I am still seeing an error when selecting a saved search which uses a runtime field from the index pattern, and then hitting the 'Use full data' button. I am also seeing it in the job wizards, even after #94760.

I think this should be fixed now with the latest changes. Let me know if still have the issue. Since it's also happening with the job wizards, I will add a fix for that in a separate follow up PR.

} from '../../../../../src/plugins/data/common/index_patterns';
import type { RuntimeField, RuntimeMappings } from '../types/fields';

export function isRuntimeField(arg: unknown): arg is RuntimeField {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking into this typeguard implementation I think it's worth writing some unit tests

Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually the same implementation in Transform. Considering we have a few other PRs to switch to estypes, I'll leave this here for now so we can consolidate later.

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested latest changes and Use Full Data button is working in the Data Visualizer for me when using a saved search with a runtime field.

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

@kibanamachine
Copy link
Contributor

💚 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
ml 6.1MB 6.1MB +695.0B

History

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

cc @qn895

@qn895 qn895 added the auto-backport Deprecated - use backport:version if exact versions are needed label Mar 29, 2021
@qn895 qn895 merged commit 587f83a into elastic:master Mar 29, 2021
@qn895 qn895 deleted the data-viz-runtime-support branch March 29, 2021 17:10
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 29, 2021
…lastic#95252)

* [ML] Add runtime support from index pattern for data viz

* [ML] move runtime mappings outside of aggregatableFields loop

* [ML] Change arg name to runtimeMappings

* [ML] Fix dv full time range broken

* [ML] Fix dv broken with time range

* [ML] Add better error handling/transparency

* [ML] Update to using estypes.RuntimeField

* [ML] Update to use some shared common functions between ml and transform

* Revert "[ML] Update to use some shared common functions between ml and transform"

This reverts commit ce813f0

* [ML] Disable context menu if no charts
@kibanamachine
Copy link
Contributor

💚 Backport successful

7.x / #95684

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Mar 29, 2021
…95252) (#95684)

* [ML] Add runtime support from index pattern for data viz

* [ML] move runtime mappings outside of aggregatableFields loop

* [ML] Change arg name to runtimeMappings

* [ML] Fix dv full time range broken

* [ML] Fix dv broken with time range

* [ML] Add better error handling/transparency

* [ML] Update to using estypes.RuntimeField

* [ML] Update to use some shared common functions between ml and transform

* Revert "[ML] Update to use some shared common functions between ml and transform"

This reverts commit ce813f0

* [ML] Disable context menu if no charts

Co-authored-by: Quynh Nguyen <[email protected]>
@lcawl lcawl added release_note:feature Makes this part of the condensed release notes and removed release_note:enhancement labels May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:File and Index Data Viz ML file and index data visualizer :ml release_note:feature Makes this part of the condensed release notes v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants