-
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
[ML] Add search time runtime support for index based Data Visualizer #95252
Conversation
Pinging @elastic/ml-ui (:ml) |
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 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 = { |
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.
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.
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.
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 |
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.
it would be good to use a isPopulatedObject
check here too.
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.
Updated here 1e708e1
(#95252)
Thanks for testing! Looks like the issue is fixed after #94760: |
@@ -602,7 +615,8 @@ export class DataVisualizer { | |||
timeFieldName: string, | |||
earliestMs?: number, | |||
latestMs?: number, | |||
datafeedConfig?: Datafeed | |||
datafeedConfig?: Datafeed, | |||
runtimeFields?: RuntimeMappings |
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.
nit, should be called runtimeMappings
for consistency
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.
Updated here a6ae3ce
(#95252)
x-pack/plugins/ml/server/models/data_visualizer/data_visualizer.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/server/models/data_visualizer/data_visualizer.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/server/routes/schemas/data_visualizer_schema.ts
Outdated
Show resolved
Hide resolved
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
I have added it as an additional item to #77462 - we can address it separately to this PR. |
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 { |
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.
Looking into this typeguard implementation I think it's worth writing some unit tests
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 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.
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 latest changes and Use Full Data button is working in the Data Visualizer for me when using a saved search with a runtime field.
…d transform" This reverts commit ce813f0
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.
LGTM
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @qn895 |
…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
…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 <43350163+qn895@users.noreply.github.com>
Summary
Part of #77462. This PR:
Handles saved search or search queries with runtime fields

Adds capability for the histogram preview to show runtime fields within Transform/Data Frame Analytics.
Before
After
Checklist
Delete any items that are not applicable to this PR.