-
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 "Analyze in ML" buttons #48268
Conversation
…ntry-rate-data-vis
…ntry-rate-data-vis
…y350/kibana into 47201-adapt-log-entry-rate-data-vis
…ntry-rate-data-vis
Pinging @elastic/infra-logs-ui (Team:infra-logs-ui) |
💔 Build Failed |
💚 Build Succeeded |
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.
Both links successfully link to the Single Metric Viewer in the ML UI. I think this is appropriate for the partition-specific inspection link. How about linking to the "Anomaly Explorer" for the overall link instead?
I wonder how useful passing the whole link format function down is. 🤔 In the places where the link is rendered, the dynamic arguments (start, end) are already known. So maybe it would be clearer to move the URL formatting logic to the ML link component and just pass down the jobId? So the component would be something like
<AnalyzeInMlButton jobId={jobId} partition={partition} timeRange={timeRange} />
The button itself would then use the base path from chrome
to assemble the link. The partition
prop could be optional to switch between the filled overall link and the empty detail link.
Would that make sense?
x-pack/legacy/plugins/infra/public/pages/logs/analysis/page_results_content.tsx
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/infra/public/pages/logs/analysis/sections/helpers/ml_links.tsx
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/infra/public/pages/logs/analysis/page_results_content.tsx
Outdated
Show resolved
Hide resolved
@weltenwort Thanks for the review 👍 I've made all of the changes suggested, this is ready for another look. |
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.
Looks great, thanks for keeping it so clean under time pressure ❤️
The comment below is just something I remembered taking me a lot of time to debug a while ago.
}, | ||
}); | ||
|
||
const hash = `/timeseriesexplorer?_g=${_g}&_a=${encodeURIComponent(_a)}`; |
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.
From experience there are situations where the all-mighty angular router chooses to disagree with the standard url encoding. It might be safer to use the QueryString.encode
function from ui/utils/query_string
to avoid that. Not sure if that is likely to happen here, but encodeURIComponent
rings an alarm bell with me. 😉
So it could look something like
const hash = `/timeseriesexplorer?${QueryString.encode({_g, _a})}`;
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.
Thank you for the hint, changed 👍 There seems to be quite a lot of discrepancy across all of the different Kibana plugins when it comes to links (understandable to a degree with React vs Angular, but even then even within the same routing environment).
💚 Build Succeeded |
💚 Build Succeeded |
Summary
This closes #46445 by implementing "Analyze in ML" buttons to the log rate results page. There's a button next to the overall chart, and buttons for the individual partitions.
Implementation notes
I've used
chrome.getBasePath()
imported fromui/chrome
which isn't ideal given the new platform changes. I tried to use ouruseKibanaInjectedVar
hook to access this, but I couldn't seem to get that to work (definitely possible PEBCAK). It works...but not ideal 🙈Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.