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] Display link to create data view from error cases in data frame analytics results pages #143596

Merged
merged 5 commits into from
Oct 20, 2022

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Oct 18, 2022

Summary

Fixes #141919

Classification/Regression:

image

Outlier detection:

image

Cloning:

image

Checklist

Delete any items that are not applicable to this PR.

@alvarezmelissa87 alvarezmelissa87 requested a review from a team as a code owner October 18, 2022 21:06
@alvarezmelissa87 alvarezmelissa87 self-assigned this Oct 18, 2022
@alvarezmelissa87 alvarezmelissa87 added :ml Feature:Data Frame Analytics ML data frame analytics features labels Oct 18, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@alvarezmelissa87 alvarezmelissa87 added v8.6.0 release_note:skip Skip the PR/issue when compiling release notes labels Oct 18, 2022
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

<FormattedMessage
id="xpack.ml.dataframe.analytics.dataViewPromptMessage"
defaultMessage="No data view exists for index {destIndex}. "
defaultMessage="No data view exists for index{destIndex}. "
Copy link
Member

Choose a reason for hiding this comment

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

Is there supposed to be a space in between for index{destIndex}

Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming it's like this for when destIndex is undefined. What about when destIndex is defined? It would read "No data view exists for indexmy-index-name. "

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Updated in 7e8031b

<FormattedMessage
id="xpack.ml.dataframe.analyticsList.noSourceDataViewForClone"
defaultMessage="Unable to clone the analytics job. No data view exists for index {dataView}."
values={{ dataView: sourceIndex }}
Copy link
Member

Choose a reason for hiding this comment

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

Super duper nit: The variable name dataView in this message is a bit confusing. Suggestion to rename variable to something like No data view exists for index {sourceIndex}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 7e8031b

<FormattedMessage
id="xpack.ml.dataframe.analytics.cloneAction.dataViewPromptLinkText"
defaultMessage="Create a data view for {dataView}"
values={{ dataView: sourceIndex }}
Copy link
Member

Choose a reason for hiding this comment

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

Super duper nit: Similarly, the variable name dataView in this message is a bit confusing as the data view has not been created yet. Suggestion to rename variable to something like Create a data view for {sourceIndex}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 7e8031b

@@ -90,6 +91,8 @@ export const OutlierExploration: FC<ExplorationProps> = React.memo(({ jobId }) =
jobConfig?.analyzed_fields?.excludes,
resultsField
);
const destIndex =
Copy link
Member

Choose a reason for hiding this comment

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

This logic is used in several places in DFA like useResultsViewConfig or deleteAnalyticsAndDestIndex... Might be out of scope of this PR but it's better to have a utility function (e.g. getDestinationIndex) so future updates to the logic can be more consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added util function in c849b80

@alvarezmelissa87
Copy link
Contributor Author

Updated and ready for a final look when you get a chance 🙏 cc @qn895, @peteharverson

@@ -34,7 +34,7 @@ export const IndexPatternPrompt: FC<Props> = ({ destIndex, color }) => {
<EuiText size="xs" color={color ?? 'warning'}>
<FormattedMessage
id="xpack.ml.dataframe.analytics.dataViewPromptMessage"
defaultMessage="No data view exists for index{destIndex}. "
defaultMessage="No data view exists for index {destIndex}. "
Copy link
Member

Choose a reason for hiding this comment

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

This will leave an extra space before the period if the destIndex is undefined. "No data view exists for index ."

@alvarezmelissa87
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ml 1628 1629 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 3.4MB 3.4MB +1.1KB

History

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

cc @alvarezmelissa87


import { DataFrameAnalyticsConfig } from '../../../../common/types/data_frame_analytics';

export const getDestinationIndex = (jobConfig: DataFrameAnalyticsConfig | undefined) =>
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion to add if (!jobConfig) return ''; in the beginning so we don't have to check for it three times.

@qn895
Copy link
Member

qn895 commented Oct 20, 2022

LGTM assuming this is fixed (since there seems to be a problem with Github not showing all the changes).

@alvarezmelissa87 alvarezmelissa87 merged commit 2eea2c2 into elastic:main Oct 20, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Oct 20, 2022
@alvarezmelissa87 alvarezmelissa87 deleted the ml-dfa-data-view-link branch October 20, 2022 19:18
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 24, 2022
* main: (57 commits)
  [Files] Filepicker (elastic#143111)
  [Infrastructure UI] Replace Lens table with EUI table and own api (elastic#142871)
  [api-docs] Daily api_docs build (elastic#143829)
  [api-docs] Daily api_docs build (elastic#143825)
  [api-docs] Daily api_docs build (elastic#143823)
  [Security Solution] Restructuring folders of Detection Engine + refactoring Rule Management (elastic#142950)
  [Dev tools] Fix performance issue with autocomplete suggestions (elastic#143428)
  [Security Solution] Disable ML rule's edit button link under basic license (elastic#143260)
  [Lens]  Use the language-documentation package for formula (elastic#143649)
  [api-docs] Daily api_docs build (elastic#143811)
  [Security Solution] Fix missing title on inspect pop-up (elastic#143601)
  fix incorrect filters being passed to events table causing duplicate entries in our inpsect tool request tab (elastic#143239)
  [Security Solution][Endpoint] `get-file` response action kibana download file API (elastic#143708)
  Rely on refresh context to update stats independently of overview cards. (elastic#143308)
  [RAM] Rule event log - Fix incorrect results when filtering by message and outcome simultaneously (elastic#143119)
  [ML] Display link to create data view from error cases in data frame analytics results pages (elastic#143596)
  Update links in README :) (elastic#143675)
  Add more tests for ml_inference_logic (elastic#143764)
  skip failing test suite (elastic#143717)
  [DOCS] Add assignees to case APIs (elastic#143610)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Data Frame Analytics ML data frame analytics features :ml release_note:skip Skip the PR/issue when compiling release notes v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ML] Display link to create data view from error cases in data frame analytics results pages
6 participants