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

[Docs] Reformat exported fields documentation #12242

Merged
merged 9 commits into from
Jun 21, 2019

Conversation

dedemorton
Copy link
Contributor

@dedemorton dedemorton commented May 22, 2019

Closes #9519

This PR changes the structure of collapses the structure of the exported field reference docs by removing sections that add little value. You can see the layout under https://metricbeatexample.firebaseapp.com/exported-fields.html

Note that I had to add description to heartbeat/_meta/fields.common.yml because it was missing. We'll need area owners to verify that no other fields were dropped.

There's some real content that's stripped out with this change. We need to decide how to handle it.

@ruflin Please check my logic. Also let me know if you have any thoughts on how we can simplify the structure without throwing away useful content.

Update: I've reverted back to the version that includes all the sections because important info was being stripped out. I've (hopefully) made the existing content easier to scan by fixing the heading levels and removing duplicated info.

@dedemorton dedemorton added docs in progress Pull request is currently in progress. review needs_backport PR is waiting to be backported to other branches. labels May 22, 2019
@dedemorton dedemorton requested a review from ruflin May 22, 2019 19:47
@dedemorton dedemorton requested review from a team as code owners May 22, 2019 19:47
@ruflin
Copy link
Member

ruflin commented May 24, 2019

Good to see this kicked off. Now we also see what it means of content we miss. It's quite a bit more then I expected and in some case the content seems to be useful. I think we need a different strategy :-(

I keep thinking the table view like we have in ECS will make things better. And every time we have a description we put it on top of the following table?

@dedemorton
Copy link
Contributor Author

@ruflin I was afraid you would say that. I monkeyed around with the script quite a bit and couldn't get my logic for building tables to work quite right...there were always a couple of modules with yml structures that resulted in end table tags in the wrong place. If you can help me think through the logic to get this working, I would be obliged.

@dedemorton
Copy link
Contributor Author

I think I just figured out why my original attempt at creating tables was not working. Fixed a bug in the script for generating field reference: #12432

I'll take another crack at this after I get all the books building in asciidoctor.

@dedemorton
Copy link
Contributor Author

dedemorton commented Jun 6, 2019

I've been playing around with this a bit more, and I'm not loving the way the long names look in the table format. Our page layout is not optimized for displaying tables, plus we can't currently control column widths because docbook does not preserve table attributes. ECS doesn't have this problem because we control the names. (Note that this is just an example. I will put the description before the type...just haven't bother yet)

image

@ruflin
Copy link
Member

ruflin commented Jun 6, 2019

What if we do some tricks with tables and having 2 rows per entry. First entry with the field name and then below 2-3 columns with all the other things? I wonder if with this we could make it nicer?

@dedemorton
Copy link
Contributor Author

I wonder if with this we could make it nicer

I'll come up with a few variations and put them up on firebase. I think you might find that complex tables are less readable than lists when rendered using our current site template. :-/ We also need to consider how these tables look when viewed on mobile devices.

@dedemorton
Copy link
Contributor Author

Here's an example that illustrates why tables with long, non-breaking content do not display well with our current doc tool chain:

image

@dedemorton
Copy link
Contributor Author

dedemorton commented Jun 7, 2019

@ruflin I've done several iterations of table formats, and none of them works well with the long names that we use in some modules (see example above). I recommend that we avoid formatting this content in tables. Several reasons:

  • The logic to generate the tables is more complex and will break more easily
  • Special characters (like [ and |) break the table format and need to be fixed in the source yaml files. This fails silently, which means that the table format can break without us realizing it.
  • The benefits of using a table (compressed content that's easy to scan) are mostly lost when we create lopsided tables (big wide left col for very long fully-qualified names).

I'm going to propose a different approach. I've cleaned up the script a bit to add nested section levels and remove repeated content. That way, we don't lose content by removing sections (as we did with my first commit), but the content is a bit easier to scan because it doesn't have so much repeated info and the heading levels are properly nested.

I recommend that we also add content to descriptions that are missing content.

Here's what my suggested format looks like:

(There will be missing descriptions in some sections until someone adds them, but overall I think this looks less janky.)

image

@ruflin
Copy link
Member

ruflin commented Jun 11, 2019

@dedemorton Thanks for the interations. I wonder if we should skip group fields if they don't have a description?

An other option could be that we go for a more compact bullet list like we have here: https://www.elastic.co/guide/en/beats/metricbeat/current/metricbeat-metricset-mssql-performance.html And the type we just mark in brackets in the end (type: keyword).

@dedemorton dedemorton changed the title [Docs] Remove extraneous section levels from field docs [Docs] Reformat exported fields documentation Jun 11, 2019
@dedemorton
Copy link
Contributor Author

@ruflin and I met to discuss our plans going forward. We decided to merge this PR because it's better than what we have now and requires no additional changes to the yaml files.

I've opened #12523 to track the next phase of work that's required. Our end goal is to someday format this content in tables for easy scanning and retrievability, but we need to wait for updates to our doc site design to enable this.

@ruflin
Copy link
Member

ruflin commented Jun 19, 2019

Change LGTM. So basically it fixes the heading, removes fields from the sub titles and moves type to the bottom.

@dedemorton Could you rebase this on master and run make update to get the build green?

@mikemadden42
Copy link
Contributor

jenkins retest this please

@ycombinator
Copy link
Contributor

Sorry @dedemorton, I just merged #12621, so this one will need the rebase.

@dedemorton
Copy link
Contributor Author

I cannot get the CI on this PR to pass. Should I try creating a new branch that has only the new commits? I have no idea what to do to fix this.

@dedemorton
Copy link
Contributor Author

merging since the docs build is now GREEN! thanks @ycombinator!

@dedemorton dedemorton merged commit cadbd9d into elastic:master Jun 21, 2019
@dedemorton dedemorton deleted the change_exported_fields branch June 21, 2019 01:55
dedemorton added a commit to dedemorton/beats that referenced this pull request Jun 21, 2019
* [Docs] Remove extraneous section levels from field docs

* Clean up headings to make content easier to scan

* Move description and run make update

* Rebase and run make update

* Removed old commented out section from script

* Rebase and run make update

* Rebase and run make update

* Fix code formatting and run make update

* Fixing python script formatting to make pep8 linter happy
dedemorton added a commit that referenced this pull request Jun 21, 2019
* [Docs] Reformat exported fields documentation (#12242)

* [Docs] Remove extraneous section levels from field docs

* Clean up headings to make content easier to scan

* Move description and run make update

* Rebase and run make update

* Removed old commented out section from script

* Rebase and run make update

* Rebase and run make update

* Fix code formatting and run make update

* Fixing python script formatting to make pep8 linter happy

* [Docs] Run make update
@dedemorton dedemorton removed the in progress Pull request is currently in progress. label Jun 25, 2019
@dedemorton
Copy link
Contributor Author

Let's consider this to be a 7.2 enhancement to the docs. Not planning to backport to 7.1 and earlier unless there's a big desire.

@dedemorton dedemorton removed the needs_backport PR is waiting to be backported to other branches. label Jun 25, 2019
DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
* [Docs] Remove extraneous section levels from field docs

* Clean up headings to make content easier to scan

* Move description and run make update

* Rebase and run make update

* Removed old commented out section from script

* Rebase and run make update

* Rebase and run make update

* Fix code formatting and run make update

* Fixing python script formatting to make pep8 linter happy
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
* [Docs] Reformat exported fields documentation (elastic#12242)

* [Docs] Remove extraneous section levels from field docs

* Clean up headings to make content easier to scan

* Move description and run make update

* Rebase and run make update

* Removed old commented out section from script

* Rebase and run make update

* Rebase and run make update

* Fix code formatting and run make update

* Fixing python script formatting to make pep8 linter happy

* [Docs] Run make update
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.

Proposal to improve the layout of the field reference documentation for Beats
4 participants