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

[Logs UI] Add "Analyze in ML" buttons #48268

Merged
merged 104 commits into from
Oct 16, 2019
Merged

Conversation

Kerry350
Copy link
Contributor

@Kerry350 Kerry350 commented Oct 15, 2019

⚠️ ⚠️ ⚠️ This PR contains all commits from #47642 and #47558 and should not be reviewed / merged until they're merged ⚠️ ⚠️ ⚠️

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.

analyze_button

Implementation notes

I've used chrome.getBasePath() imported from ui/chrome which isn't ideal given the new platform changes. I tried to use our useKibanaInjectedVar hook to access this, but I couldn't seem to get that to work (definitely possible PEBCAK). It works...but not ideal 🙈

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

Kerry350 and others added 30 commits October 3, 2019 15:15
…y350/kibana into 47201-adapt-log-entry-rate-data-vis
@Kerry350 Kerry350 added Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.5.0 :Logs UI labels Oct 15, 2019
@Kerry350 Kerry350 requested a review from a team October 15, 2019 18:29
@Kerry350 Kerry350 self-assigned this Oct 15, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-logs-ui (Team:infra-logs-ui)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@weltenwort weltenwort self-requested a review October 16, 2019 00:17
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@weltenwort weltenwort left a 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?

grafik

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?

@Kerry350
Copy link
Contributor Author

@weltenwort Thanks for the review 👍 I've made all of the changes suggested, this is ready for another look.

Copy link
Member

@weltenwort weltenwort left a 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)}`;
Copy link
Member

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})}`;

Copy link
Contributor Author

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).

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Kerry350 Kerry350 merged commit 3162194 into elastic:master Oct 16, 2019
Kerry350 added a commit to Kerry350/kibana that referenced this pull request Oct 16, 2019
Kerry350 added a commit to Kerry350/kibana that referenced this pull request Oct 16, 2019
Kerry350 added a commit to Kerry350/kibana that referenced this pull request Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Logs UI Logs UI feature release_note:enhancement Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Logs UI] Analysis log entry rate anomaly chart should provide a link to the ML UI
4 participants