-
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
Tutorial copy #16135
Tutorial copy #16135
Conversation
textPre: 'Download Logstash from [here](https://artifacts.elastic.co/downloads/logstash/logstash-{config.kibana.version}.zip) and unzip it.' | ||
textPre: 'First time using Logstash? See the ' + | ||
'[Getting Started Guide]({config.docs.base_url}guide/en/logstash/current/getting-started-with-logstash.html).\n' + | ||
' 1. Download the Logstash Windows zip file from the [Downloads page]({config.docs.base_url}downloads/logstash).\n' + |
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.
Why are we sending the user to the downloads page instead of directly to the zip file link? This means the user has to make another click to download the zip file.
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.
Chatted with @gchaps on Slack about this. Summarizing our conversation here:
Another issue with sending the users to the download page (as opposed to the direct versioned link to the artifact) is that the download page always shows the latest version. So, for example, if Kibana 6.3 and 7.1 are released and the user is viewing this tutorial in 6.3, they might accidentally end up downloading Logstash 7.1, which might not work with their 6.3 setup. There is a link to past releases on the download page, but that's one more click + the user would have to realize that they are on the wrong version to even think of looking at past releases.
OTOH, the downside of linking directly to the versioned artifact is that we might decide to change the artifact link and forget to update the tutorial content to match.
I don't know if there's a perfect solution here but, between the two options, I think the direct versioned link to the artifact is better.
@@ -66,25 +62,23 @@ export const COMMON_NETFLOW_INSTRUCTIONS = { | |||
OSX: [ | |||
{ | |||
title: 'Run the Netflow module', | |||
textPre: 'In the Logstash installation directory, run the following command to set up the Netflow module.', | |||
textPre: 'From the installation directory, run:', |
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.
We removed references to the "installation directory" earlier in this file. Should this one be removed too, for consistency?
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.
Yes, for consistency.
'dashboards to get you exploring your data immediately. Logstash modules support Netflow Version 5 and 9. [Learn more]' + | ||
'({config.docs.logstash}/netflow-module.html) about the Netflow module', | ||
shortDescription: 'Collect Netflow records sent by a Netflow exporter.', | ||
longDescription: 'The Logstash Netflow module parses network flow data, ' + |
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 imagine users who end up on this page are familiar with Netflow concepts and terminlogy. In Netflow parlance a "collector" is a well-defined concept so I'd like that to be reflected here. So could we make this "collects and parses" instead of just "parses"?
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.
It's good to use industry-standard terminology.
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.
For some reason this PR was merged without addressing this comment. I've pushed 629f7d8 directly to master
with this change now.
shortDescription: 'Collect Netflow records sent by a Netflow exporter.', | ||
longDescription: 'The Logstash Netflow module parses network flow data, ' + | ||
' indexes the events into Elasticsearch, and installs a suite of Kibana dashboards.' + | ||
' Logstash modules support Netflow Version 5 and 9.' + |
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 think there was a typo here even before this PR. I think this should say "This module supports" instead of "Logstash modules support". Mind fixing this as part of this PR?
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.
good catch
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.
Technically, it looks good to me, I'll defer to the others on the exact copy though.
@@ -11,7 +11,7 @@ export function netflowSpecProvider() { | |||
shortDescription: 'Collect Netflow records sent by a Netflow exporter.', | |||
longDescription: 'The Logstash Netflow module parses network flow data, ' + | |||
' indexes the events into Elasticsearch, and installs a suite of Kibana dashboards.' + | |||
' Logstash modules support Netflow Version 5 and 9.' + | |||
' This modules support Netflow Version 5 and 9.' + |
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.
Minor typo: should be module (as opposed to modules).
@alexfrancoeur Looks good - thanks @gchaps! |
Btw, if not too late, it'd be great to get this screenshot added to the APM header as part of this PR? |
Closes #16003 |
* update short descriptions * apache log updates * apache metrics and and APM updates * docker, mysql, and kubernetes updates * netflow updates * all of the rest * remove From the installation directory, fix type Logstash modules => This modules * add APM dashboard screenshot * fix typo and download URL
* Tutorial copy (#16135) * update short descriptions * apache log updates * apache metrics and and APM updates * docker, mysql, and kubernetes updates * netflow updates * all of the rest * remove From the installation directory, fix type Logstash modules => This modules * add APM dashboard screenshot * fix typo and download URL * Adding "collects" to align with standard Netflow terminology of a collector
Incorporate @gchaps changes into tutorial text.
Fix broken link on home page.
cc @alexfrancoeur @formgeist @ycombinator @tsg