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

Fix alias field generation in docs #9269

Merged
merged 3 commits into from
Dec 3, 2018

Conversation

ruflin
Copy link
Member

@ruflin ruflin commented Nov 28, 2018

Alias fields documented in the fields list were not correct. The reason is that path was used in our generation of the docs for a different puprose. This one was renamed and reporting of path was added for field alias.

@ruflin
Copy link
Member Author

ruflin commented Nov 28, 2018

@dedemorton This is a fix for the aliases but I think we need a bigger overhaul on how we represent / show fields. @karenzone Perhaps if we figure it out for ECS, we can also use it here.

@webmat webmat added the ecs label Nov 28, 2018
@ruflin ruflin mentioned this pull request Nov 28, 2018
+
--
type: alias

path: agent.type
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this literal output of path: a bit dry. I understand that this is the terminology used inside ElasticSearch.

But I think we could output something a bit more human friendly here:

*`beat.name`*::
+
--
type: alias

alias to: agent.type

Copy link
Member Author

Choose a reason for hiding this comment

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

changed, PR will be updated in a minute.

@dedemorton
Copy link
Contributor

@ruflin I agree. The headings in particular are not very helpful and the repetition of headings looks odd. The structure looks especially bad when the descriptions in the source yml are cryptic. It works a bit better when the descriptions are more complete. I think we should come up with a good design for the reference content and work back from there. @karenzone Let's work on this together.

@webmat
Copy link
Contributor

webmat commented Nov 28, 2018

Yeah the context is that all of these field being aliased are now replaced by more canonical fields. Most of them are ECS fields, but not necessarily.

So there's a few things we can do, in addition to my simple suggestion above, if we think it's worth it:

  • We could have a functional asciidoc link that lets the user navigate to the new canonical definition
  • We could pull the definition of the new field (in example above, get agent.type definition), and add it to this old field (in example above, output definition to deprecated field beat.name), in addition to mentioning that it's now an alias.
  • Instead of copying the field's doc to the old field, we could output a deprecation warning and say that "This field is going away in 8.0. In the meantime, this alias ensures visualizations and aggregations based on this field will continue working, but please update them to the new field."
    • Note that most module fields are moving in this fashion, and that some of the generic Beats fields are moving as well. So perhaps we can have a full explanation in one place, and a much shorter text in each of these aliases, which may well end up being 100+ aliases.

One final note, the presence of an alias does not absolutely mean that this is a migration for 7.0, or caused by ECS. So perhaps all of these suggestions above need to be triggered by an additional field in the yml files. Such field migrations can keep happening in 8.0 and later, and we may want to be able to differentiate them.

@dedemorton
Copy link
Contributor

I don't like the idea of an asciidoc link because I don't want to force users to go to a different page to find a field description. I think the 3rd option is best because it helps users understand why we've created the aliases. At any rate, we should pull in the description of ECS fields from a single source of truth and use those descriptions wherever we describe the fields. I hope that we can make the descriptions generic enough so that they make sense wherever the description might show up.

@ruflin
Copy link
Member Author

ruflin commented Nov 29, 2018

This PR is here to fix the current bug. As mentioned in my initial comment, we need to have a broader discussion but this should not hold back this PR.

@dedemorton What is the best place we can continue this discussion?

@ruflin ruflin added the needs_backport PR is waiting to be backported to other branches. label Nov 29, 2018
@webmat webmat mentioned this pull request Nov 29, 2018
14 tasks
@webmat
Copy link
Contributor

webmat commented Nov 29, 2018

I'm good with splitting off the perfect representation of aliases to later. This PR addresses some foundational stuff.

Perhaps open a GH issue specifically about the visual representation of aliases, and port the discussion there?

Alias fields documented in the fields list were not correct. The reason is that `path` was used in our generation of the docs for a different puprose. This one was renamed and reporting of path was added for field alias.
@ruflin
Copy link
Member Author

ruflin commented Nov 29, 2018

Issue opened here: #9288

@webmat Could you approve the PR so I can merge it? It will conflict with many other (ecs) PR's :-)

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

Yes, LGTM

@ruflin ruflin merged commit 753566b into elastic:master Dec 3, 2018
@ruflin ruflin deleted the fix-alias-generation branch December 3, 2018 10:18
@webmat webmat mentioned this pull request Dec 3, 2018
5 tasks
@ruflin ruflin added v6.6.0 and removed needs_backport PR is waiting to be backported to other branches. labels Dec 11, 2018
ruflin added a commit to ruflin/beats that referenced this pull request Dec 11, 2018
Alias fields documented in the fields list were not correct. The reason is that `path` was used in our generation of the docs for a different puprose. This one was renamed and reporting of path was added for field alias.

(cherry picked from commit 753566b)
ruflin added a commit that referenced this pull request Dec 11, 2018
Alias fields documented in the fields list were not correct. The reason is that `path` was used in our generation of the docs for a different puprose. This one was renamed and reporting of path was added for field alias.

(cherry picked from commit 753566b)
DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
Alias fields documented in the fields list were not correct. The reason is that `path` was used in our generation of the docs for a different puprose. This one was renamed and reporting of path was added for field alias.
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.

3 participants