Create-metricset docs review #8375

sayden · 2018-09-20T11:46:45Z

Right now, It's only lacking a more detailed description about testing. @dedemorton suggested that maybe they should be in their own section. I think it's a good idea too.

dedemorton

Submitted a handful of changes, but looks good overall. Thanks for doing this!!

dedemorton · 2018-09-21T21:35:04Z

docs/devguide/create-metricset.asciidoc

 binary in debug mode with the following command:
 +
 [source,bash]
 ----
-./{beat} -e -d "*"
+./metricset -e -d "*"


Shouldn't this say metricbeat?

dedemorton · 2018-09-21T21:35:56Z

docs/devguide/create-metricset.asciidoc

 }
 ----

 [float]
 ===== Definition

-The MetricSet type defines all fields of the metricset. As a minimum it must inherit the `mb.BaseMetricSet` fields,
+The MetricSet type defines all fields of the metricset. As a minimum it must be composed of	 the `mb.BaseMetricSet` fields,


minor: remove the extra spaces after "be composed of"

dedemorton · 2018-09-21T22:26:50Z

docs/devguide/metricset-details.asciidoc

-The `fields.yml` file is used for different purposes:
+You can find up to 3 related `fields.yml` files in the beats repository for each metricbeat module:
+
+* `fields.yml` (at the root of the repository).


There is no fields.yml at the root. You should give the full path and filename.

dedemorton · 2018-09-21T22:34:30Z

docs/devguide/metricset-details.asciidoc

@@ -87,14 +87,22 @@ functionality of the metricset and `Fetch` method from the data mapping.
 [float]
 ==== fields.yml

-The `fields.yml` file is used for different purposes:
+You can find up to 3 related `fields.yml` files in the beats repository for each metricbeat module:


I think this structure is harder for readers to skim. It's OK if you want to leave it for now (might make sense for me to take a quick editorial pass through the dev docs after you are done with your additions anyhow). If you want to make this easier to skim, you can put the description of each file under the bullet point (then you can just list the files and avoid saying "The first fields.yml is under..." (let me know if my comment is confusing...easier to do than explain). The asciidoc markup for indenting sections below bullet points looks like this:

. Some text next to a bullet point + Paragraph. + Example block + Etc . Some text next to a bullet point

Yeah, I agree that maybe it wasn't the best. I have built a new structure that I think is a bit better.

dedemorton · 2018-09-21T23:16:21Z

docs/devguide/metricset-details.asciidoc

@@ -104,7 +112,7 @@ Here an example for the `fields.yml` file from the MySQL module.
 include::../../metricbeat/module/mysql/_meta/fields.yml[]
 ----

-There is another `fields.yml` file under `module/{module}/{metricset}/_meta/fields.yml` that contains all fields retrieved
+The last `fields.yml` file under `module/{module}/{metricset}/_meta/fields.yml` that contains all fields retrieved


The sentence structure here is not parallel with the previous bullet item, but that problem will be fixed if you change the structure as I described previously.

If you decide to punt, change this bullet item to use a parallel sentence structure: "The xyz is under....and it contains..."

dedemorton · 2018-09-21T23:28:06Z

docs/devguide/create-metricset.asciidoc

 +
 [source,bash]
 ----
 make create-metricset
 ----
 +
-You'll be prompted to enter a module and metricset name. Only use characters `[a-z]`
+You need to use Python 2.7 to run this command. If you also have Python 3 in your system, you can run the command with `VIRTUALENV_PARAMS="-p python2.7" make create-metricset` to force it to run with Python 2.7.


Typically we use asciidoc attributes to resolve version numbers, or this info gets stale and we forget to update it. We should use one of the existing attributes, or add one to this file: https://github.com/elastic/beats/blob/master/libbeat/docs/version.asciidoc

So you might add something like :python-short: python2.7 to the versions file, and then use Python {python-short} to resolve the value at doc build time.

Note that asciidoc attributes that appear within code snippets need extra love to resolve correctly. Wrap the code in plus signs instead of back ticks.

+VIRTUALENV_PARAMS="-p python{python-short}" make create-metricset+

Ok, I found a python reference already which also works so I have changed it accordingly.

dedemorton · 2018-09-21T23:32:03Z

docs/devguide/metricset-details.asciidoc

@@ -148,7 +156,7 @@ We recommend that you use all three when you create a metricset. Unit tests are
 written in Go and have no dependencies. Integration tests are also written
 in Go but require the service from which the module collects metrics to also be running.
 System tests for Metricbeat also require the service to be running in most cases and are
-written in Python based on our small Python test framework.
+written in Python 2 based on our small Python test framework.


same comment...use an asciidoc attribute to resolve the version number.

ruflin · 2018-09-28T18:22:33Z

I added the review label as I assume it's ready to be reviewed.

dedemorton · 2018-09-28T18:56:16Z

docs/devguide/create-metricset.asciidoc

@@ -17,7 +17,7 @@ To create a new metricset:
 make create-metricset
 ----
 +
-You need to use Python 2.7 to run this command. If you also have Python 3 in your system, you can run the command with `VIRTUALENV_PARAMS="-p python2.7" make create-metricset` to force it to run with Python 2.7.
+You need to use Python 2.7 to run this command. If you also have Python 3 in your system, you can run the command with +VIRTUALENV_PARAMS="-p python{python}" make create-metricset+ to force it to run with Python 2.7.


There are still a couple of places where you say "Python 2.7"

Note that {python} is defined as:

:python: 2.7.9

So python{python} resolves as python2.7.9. If you need this to resolve to a shorter version (2.7), you''ll need to add another attribute to the versions.asciidoc file and use that attribute here.

I messed up here the python titles with commands. I think now with a new parameter in the versions file it should be more clear

dedemorton · 2018-09-28T18:59:13Z

docs/devguide/metricset-details.asciidoc

+[float]
+===== metricbeat/{module}/_meta/fields.yml
+
+The second `fields.yml` file is under `metricbeat/module/{module}/_meta/fields.yml` and it contains
 the general top level structure for all metricsets. Normally you only need to modify the description in this file.

 Here an example for the `fields.yml` file from the MySQL module.


Sorry, I missed this before, but there is a missing word here. Should say "Here is an example" or "Here's an example"

Sorry about that :)

dedemorton · 2018-09-28T19:00:41Z

docs/devguide/metricset-details.asciidoc

+[float]
+===== metricbeat/{module}/{metricseat}/_meta/fields.yml  
+
+The last `fields.yml` file under `metricbeat/module/{module}/{metricset}/_meta/fields.yml` that contains all fields retrieved


The phrasing of this sentence is still wrong. Should say "The last...file is under...and it contains... (otherwise it's a sentence fragment and doesn't match style/structure of other sections).

Sorry again :)

dedemorton · 2018-09-28T19:05:05Z

docs/devguide/metricset-details.asciidoc

@@ -148,7 +161,7 @@ We recommend that you use all three when you create a metricset. Unit tests are
 written in Go and have no dependencies. Integration tests are also written
 in Go but require the service from which the module collects metrics to also be running.
 System tests for Metricbeat also require the service to be running in most cases and are
-written in Python based on our small Python test framework.
+written in {python} based on our small Python test framework.


See my previous comment. I think you want this to say Python {python}

dedemorton

Found a few minor nits. The structure of the content about the yaml files is much better. thanks!

ruflin · 2018-10-18T06:25:56Z

To go green this needs a rebase on master and then make fmt.

sayden · 2018-10-22T20:35:09Z

Rebased done and make fmt passed with Python 2.7 and Go 1.10.3 on the root folder (beats)

ruflin · 2018-10-24T06:59:08Z

@sayden Seems like this breaks the doc build: https://beats-ci.elastic.co/job/elastic+beats+pull-request+multijob-docs/923/console

You can run make docs in the specific beat directory to build the docs or make docs-preview to directly open it in a browser.

sayden · 2018-10-24T08:23:37Z

Gonna fix it

sayden · 2018-10-29T20:51:55Z

It took me a while until I realized that Jenkins was complaining about the brackets {} in the title so I have replace them with square brackets [].

@dedemorton Is this ok? If not I think I'll need some creative suggestion 😆

dedemorton · 2018-10-30T01:35:11Z

@sayden You need to escape the content that appears in curly braces. Otherwise, asciidoc thinks it's an attribute and tries to resolve it. It fails (silently--a feature, I've been told), and you end up with an empty heading, which as you've discovered breaks the doc build.

To fix the problem, escape the curly braces. For example:

metricbeat/\{module\}/_meta/fields.yml

Note that you don't have to escape curly braces in the example that appears after the heading because you've told asciidoc to treat it as text (by wrapping in back ticks).

exekias · 2018-11-06T10:59:08Z

@kaiyan-sheng this content may be useful if you find anything outdated in the current guide ^

elasticmachine · 2018-12-03T14:38:41Z

Pinging @elastic/infrastructure

dedemorton · 2019-04-22T22:18:03Z

@sayden Wondering about the status of this PR. Would you like some help moving it forward?

sayden · 2020-02-07T14:30:19Z

@dedemorton I have removed the mentions about Python because they are changing all the Pyhton related things now that we have had to move to Python 3. I have also reviewed some of the comments you did and I have tried to restructure some of the things in the way you explained (I'm not sure if I did it correctly)

Feel free to add more comments, of course.

sayden · 2020-02-07T14:31:47Z

Forgot to mention, I have ran a zillion of make commands from update, docs to collect-docs in few folders but I'm not sure if I'm missing something yet. I'll take a look at the CI once it has finished.

dedemorton

Minor change to add a space, but otherwise LGTM. I'll approve because there's no need for you to ask for a second review after adding the space.

dedemorton · 2020-02-07T18:15:05Z

docs/devguide/metricset-details.asciidoc


+* `metricbeat/fields.yml`: Contains the definitions to create the Elasticsearch template, the Kibana index pattern configuration and the exported fields documentation for metricsets.To make sure the Elasticsearch template is correct, it's important to keep this file up-to-date with all the changes. Generally, you shouldn't touch this file manually because it's generated by some commands in the build environment.


Suggested change

* `metricbeat/fields.yml`: Contains the definitions to create the Elasticsearch template, the Kibana index pattern configuration and the exported fields documentation for metricsets.To make sure the Elasticsearch template is correct, it's important to keep this file up-to-date with all the changes. Generally, you shouldn't touch this file manually because it's generated by some commands in the build environment.

* `metricbeat/fields.yml`: Contains the definitions to create the Elasticsearch template, the Kibana index pattern configuration and the exported fields documentation for metricsets. To make sure the Elasticsearch template is correct, it's important to keep this file up-to-date with all the changes. Generally, you shouldn't touch this file manually because it's generated by some commands in the build environment.

dedemorton · 2020-02-13T20:25:11Z

I think this is ready merge!

sayden added docs Metricbeat Metricbeat labels Sep 20, 2018

sayden requested a review from dedemorton September 20, 2018 11:46

dedemorton reviewed Sep 21, 2018

View reviewed changes

ruflin added the review label Sep 28, 2018

dedemorton reviewed Sep 28, 2018

View reviewed changes

dedemorton requested changes Sep 28, 2018

View reviewed changes

sayden force-pushed the feature/creating-a-metricbeat-docs-improvement branch from f61f628 to fd5e3be Compare October 22, 2018 20:34

ruflin removed Team:Integrations Label for the Integrations team labels Nov 27, 2018

ruflin assigned sayden Dec 3, 2018

ruflin added the Team:Integrations Label for the Integrations team label Dec 3, 2018

kaiyan-sheng assigned kaiyan-sheng and unassigned kaiyan-sheng Feb 18, 2019

sayden added in progress Pull request is currently in progress. and removed review labels Mar 13, 2019

andresrc added the [zube]: In Progress label Jun 19, 2019

andresrc added :infrastructure [zube]: Inbox [zube]: In Progress and removed Team:Integrations Label for the Integrations team [zube]: In Progress [zube]: Inbox labels Jan 29, 2020

sayden force-pushed the feature/creating-a-metricbeat-docs-improvement branch from f34e08e to 363ac47 Compare February 7, 2020 12:36

Re-commit

e4becd4

sayden force-pushed the feature/creating-a-metricbeat-docs-improvement branch from 363ac47 to e4becd4 Compare February 7, 2020 12:37

sayden added 2 commits February 7, 2020 13:49

Create-metricset

942fd4f

Fix some comments from Dede

aad7948

dedemorton approved these changes Feb 7, 2020

View reviewed changes

Add missing space

ac5bef6

sayden merged commit 2f58eb9 into elastic:master Feb 14, 2020

zube bot added [zube]: Done and removed [zube]: In Progress labels Feb 14, 2020

andresrc removed the [zube]: Done label Feb 19, 2020

kvch pushed a commit to kvch/beats that referenced this pull request Feb 20, 2020

Create-metricset docs slight update (elastic#8375)

49dff76


		* `metricbeat/fields.yml`: Contains the definitions to create the Elasticsearch template, the Kibana index pattern configuration and the exported fields documentation for metricsets.To make sure the Elasticsearch template is correct, it's important to keep this file up-to-date with all the changes. Generally, you shouldn't touch this file manually because it's generated by some commands in the build environment.

Create-metricset docs review #8375

Create-metricset docs review #8375

Conversation

sayden commented Sep 20, 2018

dedemorton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sayden Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dedemorton left a comment

Choose a reason for hiding this comment

ruflin commented Oct 18, 2018

sayden commented Oct 22, 2018

ruflin commented Oct 24, 2018

sayden commented Oct 24, 2018

sayden commented Oct 29, 2018

dedemorton commented Oct 30, 2018 • edited Loading

exekias commented Nov 6, 2018

elasticmachine commented Dec 3, 2018

dedemorton commented Apr 22, 2019

sayden commented Feb 7, 2020

sayden commented Feb 7, 2020 • edited Loading

dedemorton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dedemorton commented Feb 13, 2020

sayden Sep 28, 2018 •

edited

Loading

dedemorton commented Oct 30, 2018 •

edited

Loading

sayden commented Feb 7, 2020 •

edited

Loading