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

[WIP] Adds 'upload data from file' to Kibana home page #24226

Merged
merged 11 commits into from
Oct 24, 2018

Conversation

ryankeairns
Copy link
Contributor

@ryankeairns ryankeairns commented Oct 18, 2018

Fixes #23777

WIP: The 'Upload data from file' link now points to /app/ml#/filedatavisualizer (check that this view/work has been merged (#24423) to master before merging this PR). merged and rebased this PR.

Summary

Adds 'upload file' and 'functionbeat' to home page per #23777 .

screenshot 2018-10-22 10 58 42

Checklist

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

For maintainers

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ryankeairns ryankeairns changed the title [WIP] Adds upload file and functionbeat to Kibana home page [WIP] Adds 'upload data from file' to Kibana home page Oct 19, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ryankeairns
Copy link
Contributor Author

retest

1 similar comment
@ryankeairns
Copy link
Contributor Author

retest

@alexfrancoeur
Copy link

@ryankeairns from the screenshot it looks like we're defaulting to highlight only the index pattern if there is no data. In general, I think we can remove this conditional logic now that we have the splash screen. This was meant to highlight sample data a bit more. @nreese can provide some details around the initial implementation.

@grabowskit do we have a URL that we can plugin here for the upload a file option?

@alexfrancoeur
Copy link

Woops, looks like the description was updated with the appropriate URL

@ryankeairns
Copy link
Contributor Author

@alexfrancoeur the screenshot picked up my mouse hovering that element. I'll update the screenshot.

@ryankeairns
Copy link
Contributor Author

retest

@@ -209,16 +214,35 @@ const AddDataUi = ({ apmUiEnabled, isNewKibanaInstance, intl }) => {
</EuiLink>
</EuiText>
</EuiFlexItem>
<EuiFlexItem className={footerItemClasses}>
Copy link
Contributor

Choose a reason for hiding this comment

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

This link needs to be added conditionally since ML plugin will not always be installed or enabled.

Also, why is the csv import in the ML plugin? Maybe the CSV import needs to be put into its own plugin.

Choose a reason for hiding this comment

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

@nreese in the future, we'd like to implement a full workflow. The ML team introduced this as it has some intelligence behind it. This will evolve into an "autogrok" capability for your pipeline. You can imagine this in the future where you upload a file to test it out, end with a pipeline and then add that pipeline config through a UI. But yes, I agree, there are plenty of details to hash out here - this is the first iteration of the UI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nreese Would this entail doing something like:

const mlEnabled = chrome.getInjected('mlEnabled');
...
if (mlEnabled === true) {<EuiFlexItem ... />}

My intent here was to get the ball rolling on this change, but I'm getting a little out of my depth :) Thanks for the feedback!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'm getting close with something like

{mlEnabled !== false ? <EuiFlexItem>...</EuiFlexItem> : null}

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but you will have to add mlEnabled to the injected vars. mlEnabled is added by the APM application but that may not be enabled/installed either. I think the best bet is to add mlEnabled to https://github.com/elastic/kibana/blob/master/x-pack/plugins/ml/index.js and then check that value for displaying the FlexItem

type="button"
>
<FormattedMessage
defaultMessage="Import a JSON, CSV or text file"
Copy link
Contributor

Choose a reason for hiding this comment

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

Import a JSON, CSV or text file -> Import a JSON, CSV, or text file (add comma after CSV)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ryankeairns
Copy link
Contributor Author

retest

@jgowdyelastic
Copy link
Member

jgowdyelastic commented Oct 22, 2018

The text "Import a JSON, CSV, or text file" suggests to me that you want to link directly to the File Data Visualizer page, the address of which is /app/ml#/filedatavisualizer.

The current link being used here (/app/ml#/datavisualizer) will go to the Data Visualizer landing page which looks like this:
image

This is currently on a feature branch and will be merged tomorrow in time for FF.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ryankeairns
Copy link
Contributor Author

@alexfrancoeur cc:/ @nreese I think we need to tag in an engineer to finish this last conditional piece around showing the 'Upload data from file' link. My intent was to get this rolling from the design side and I think we've eclipsed my Kibana-foo for finishing the last recommendation from Nathan: #24226 (comment)

Is there somebody who could jump in and put this over the finish line?
I've updated the link per @jgowdyelastic 's comment.

Thanks!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese
Copy link
Contributor

nreese commented Oct 23, 2018

@ryankeairns I will create a PR against yours that finishes the conditional stuff

@ryankeairns
Copy link
Contributor Author

@nreese @alexfrancoeur rebased this PR, checked that the link to the new ML page worked.

@ryankeairns ryankeairns removed the WIP Work in progress label Oct 23, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ryankeairns
Copy link
Contributor Author

@elastic/apm @jgowdyelastic @alexfrancoeur This should be ready to go, please take a look! In particular, this PR moves mlEnabled from APM to ML... will that cause any issues?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@formgeist
Copy link
Contributor

@sqren Does this affect us and our ML integration in particular?

@sorenlouv
Copy link
Member

@formgeist Not sure. We will need to test it to make sure it doesn't.

Copy link

@alexfrancoeur alexfrancoeur left a comment

Choose a reason for hiding this comment

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

@ryankeairns LGTM though you may need to merge with master to get #24438, I'm not seeing the latest in your pull

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@gchaps
Copy link
Contributor

gchaps commented Oct 24, 2018

@ryankeairns, @alexfrancoeur, @jgowdyelastic If its not too late, here are some suggested edits that make the text easier to read.

Import data

Import and visualize data from a log file. You can upload files up to 100 MB.

Upload file

(Added import in the first sentence to tie it to the title. Added space before MB. Does Upload file work?)

Select an index pattern

Visualize the data in an existing Elasticsearch index.

Select index

("Select an index pattern" reads better than "pick index pattern)

Start trial

To experience the full Machine Learning features that a Platinum subscription has to offer, start a 30-day trial.

(Removed "from the license management page" because I'm assuming that clicking the button takes the user to that page.)

@alexfrancoeur
Copy link

@gchaps thanks for the feedback! I think this is more related to the ML portion of the UI and something that @jgowdyelastic may be able to adjust either before or after FF. This PR is specific to the link on the home page which I believe your have already provided suggestions for.

@ryankeairns @nreese will we be good to merge by EOD?

@ryankeairns
Copy link
Contributor Author

@alexfrancoeur I think we just need some reassurance or approvals from the APM and ML teams based on the work that @nreese did in this PR. Functionally, it works. We're just not certain that moving mlEnabled will have any unintended consequences. I'm basically passing along that concern from Nathan, hopefully I've got that straight.

@peteharverson
Copy link
Contributor

@gchaps I will make the edits to the ML Data Visualizer landing page that you suggested in #24226 (comment). As @alexfrancoeur said, that page is only accessed from within the ML plugin. The edits to the page accessed from the Home page were completed in #24438 and #24485.

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.

A couple of suggested edits to the link to the ML File Data Visualizer, plus a typo

@ryankeairns
Copy link
Contributor Author

Made the edits suggested by @peteharverson

injectDefaultVars(server) {
const config = server.config();
return {
mlEnabled: config.get('xpack.ml.enabled'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just looking for the xpack.ml.enabled setting in kibana.yml? If so, using this as the check as to whether to add the 'Upload data from log file' link to the Home page LGTM.

I believe APM have been using this mlEnabled flag to check whether to add a link to creating ML jobs from APM dashboards, but that is being edited in #24486 as they now need to check the type of license, and not just whether the ML feature is enabled.

@alexfrancoeur
Copy link

Given that FF is EoD, would it be possible to merge today and address any issues that APM may have after? As @peteharverson mentioned, it sounds like they already have a bug #24486 they need to address. I don't believe we need to check for license. I feel like we're already using this flag on the homepage in Cloud to detect if whether or not we should be showing ML on the homepage (this may not be the case though).

@formgeist
Copy link
Contributor

@alexfrancoeur You don't need to wait for us to merge, our issues are not directly related to the moving of the mlEnabled flag, but rather the changes to the ML licensing as @peteharverson mentioned. We're working on a fix.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

lgtm
code review, tested changes in chrome

@alexfrancoeur
Copy link

I think we're good here then, @ryankeairns want to merge & backport?

@ryankeairns ryankeairns merged commit 03d7294 into elastic:master Oct 24, 2018
ryankeairns pushed a commit to ryankeairns/kibana that referenced this pull request Oct 24, 2018
* adds upload file and functionbeat to home

* removes functionbeat, adds ml file upload href

* remove functionbeat callout, add ml file url

* fix tests for basepath

* less caveman sounding

* update code editor snapshot

* make link conditional, copy changes

* update ml link

* move mlEnabled to ml plugin

* copy edits
ryankeairns pushed a commit that referenced this pull request Oct 24, 2018
* adds upload file and functionbeat to home

* removes functionbeat, adds ml file upload href

* remove functionbeat callout, add ml file url

* fix tests for basepath

* less caveman sounding

* update code editor snapshot

* make link conditional, copy changes

* update ml link

* move mlEnabled to ml plugin

* copy edits
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants