-
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
[Logs UI] Add "View in machine learning" links in the anomaly explorer #74555
Conversation
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
@elasticmachine merge upstream |
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.
👀 Reviewing on behalf of @elastic/logs-metrics-ui
...
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.
The machine learning team just improved the job management page such that it is possible to link to a filtered state in #74533. What do you think of having these buttons link to the job management page instead, since the flyout is all about managing the jobs and not about the results?
@@ -11,10 +11,12 @@ import { encode } from 'rison-node'; | |||
import { TimeRange } from '../../../../common/http_api/shared/time_range'; | |||
import { useLinkProps, LinkDescriptor } from '../../../hooks/use_link_props'; | |||
|
|||
type DatemathRange = TimeRange | { startTime: string; endTime: string }; |
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.
Would it be worth limiting ourselves to one date representation for simplicity's sake?
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'm removing this, since the time range is not needed for the job management pages
...plugins/infra/public/components/logging/log_analysis_setup/setup_flyout/module_list_card.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Felix Stürmer <weltenwort@users.noreply.github.com>
I think it might make sense... but if I understood #72402 correctly we have to restore the link to the anomaly explorer, right? Do we want to get rid of it? |
Hm, the main use of this link is to allow for the user to manage (stop, edit, delete) the job. So the intended use is different from the link for individual anomalies, which is about inspecting the anomalies. I probably should have proposed a different URL in the issue. 🤔 |
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, I just left two question below.
const moduleIcon = | ||
moduleStatus.type === 'required' ? ( | ||
<EuiIcon size="xxl" type="machineLearningApp" /> | ||
) : ( | ||
<EuiIcon color="secondary" size="xxl" type="check" /> | ||
); | ||
|
||
const viewInMlLinkProps = useLinkProps({ | ||
app: 'ml', | ||
hash: '/jobs', |
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.
Should we use pathname
instead of hash
because this seems to be rewritten to /apps/ml/jobs
immediately?
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.
Let me try 👍
<EuiButtonEmpty {...viewInMlLinkProps}> | ||
<FormattedMessage | ||
id="xpack.infra.logs.analysis.viewInMlButtonLabel" | ||
defaultMessage="View in Machine Learning" | ||
/> | ||
</EuiButtonEmpty> | ||
</> |
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.
The vertical rhythm is a bit weird. Do we want to insert a <EuiSpacer size="xs" />
above it like shown in the EUI card docs?
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 can add it. I could not find the documentation that you mentioned though :/
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 was referring to the double-row example code on the <EuiCard>
documentation page:
<EuiCard
icon={<EuiIcon size="xxl" type="savedObjectsApp" />}
title="Save Objects"
description="Example of a short card description."
footer={
<div>
<EuiButton aria-label="Go to Save Objects">Go for it</EuiButton>
<EuiSpacer size="xs" />
<EuiText size="s">
<p>
Or try <EuiLink href="http://google.com">this</EuiLink>
</p>
</EuiText>
</div>
}
/>
💚 Build SucceededBuild metricsasync chunks size
History
To update your PR or re-run it, just comment with: |
* master: (30 commits) [code coverage] always download node before team assignment (elastic#75424) [Form lib] Allow new "defaultValue" to be provided when resetting the… (elastic#75302) [Logs UI] Add "View in machine learning" links in the anomaly explorer (elastic#74555) skip flaky suite (elastic#75440) skip flaky suite (elastic#75386) [Saved objects] Add support for version on create & bulkCreate when overwriting a document (elastic#75172) [Functional]Table Vis increase sleep time in order filter to be applied (elastic#75138) MOAR RAM (elastic#75423) [Visualize] Horizontal Bar Percentiles Overlapping (elastic#75315) [ML] DF Analytics / Transforms: Fix job row actions menu invalid DOM nesting warning (elastic#74499) [ML] Inference models management (elastic#74978) [Monitoring] Migrate karma tests (elastic#75301) [Index template] Add filters to simulate preview (elastic#74497) Bump and consolidate dependencies (elastic#75360) [Ingest Manager] Fix agent config rollout rate limit to use constants (elastic#75364) Update Node.js to version 10.22.0 (elastic#75254) [ML] Anomaly Explorer / Single Metric Viewer: Fix error reporting for annotations. (elastic#74953) [Discover] Fix histogram cloud tests (elastic#75268) Uiactions to navigate to visualize or maps (elastic#74121) Use prefix search invis editor field/agg combo box (elastic#75290) ...
Summary
Add links to view the Ml jobs for the anomaly explorer in the Machine learning app.
Closes #72402
Follow-up:
The current link is hardcoded. The ML team is working on a URL generator that will make easier for consumers to use URLs to their app.