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] Update documentation copied from beats #2131

Merged
merged 2 commits into from
Apr 23, 2019

Conversation

bmorelli25
Copy link
Member

@bmorelli25 bmorelli25 commented Apr 22, 2019

Updating some docs copied from beats. Everything is pretty straight forward, though I have two comments:

  • My main concern is with the changes to monitoring-beats.asciidoc. Looks like Support shipping directly to monitoring cluster beats#9260 introduced a new class of settings named monitoring.*, which I can't get to work in APM Server. Can a dev confirm these doc changes should be left out for now? edit: Just realized I was trying this in 7.0, not master. This change works in master. Should we also update apm-server.yml to use the new settings name?
  • A separate build issue was causing my make update-beats-docs command to fail. To try and fix it, I updated the build_docs script to use the dockerized build command. This seems to work fine. Might make sense to update now that the .pl version has reached EOL.

Related elastic/beats#11823
Closes #1957
Closes #2083

@bmorelli25 bmorelli25 requested review from graphaelli and jalvz April 22, 2019 19:39
@bmorelli25 bmorelli25 self-assigned this Apr 22, 2019
Copy link
Member

@graphaelli graphaelli left a comment

Choose a reason for hiding this comment

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

This looks great. Would you have a go at updating _meta/beat.yml (+ make update to update apm-server.yml and apm-server.docker.yml)? This can close out #2083 all in one go.

@bmorelli25
Copy link
Member Author

bmorelli25 commented Apr 22, 2019

Side note - I wonder if I'm running into the same thing @jalvz did here: #2119 (comment).
make update seems to only update _meta/beat.yml -> apm-server.yml
I have to run make update again to update apm-server.docker.yml from apm-server.yml. Is this expected behavior?
Aaaaand I read the whole issue, y'all are already on it 😅

@bmorelli25 bmorelli25 merged commit 8594984 into elastic:master Apr 23, 2019
@bmorelli25 bmorelli25 deleted the update-beats-docs branch April 23, 2019 14:52
# have the Elasticsearch output configured, you can simply uncomment the
# following line.
#xpack.monitoring.elasticsearch:
#monitoring.enabled: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the late response here. With the latest changes from libbeat, both options - xpack.monitoring and monitoring are supported, but xpack.monitoring will be deprecated from 7.1.0 on. Afaik we usually still document deprecated settings, so I suggest to document them both, but clearly mark the deprecated setting.
Also, when describing the new setting, could you please ensure to fully remove xpack also from the header and the description.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do typically document deprecated settings, but I chose to follow Beats lead by removing it from yml files and docs: elastic/beats/pull/11677+ elastic/beats/pull/11678. We can document the deprecated settings, but it will mean adding more complexities to the docs as this is beats documentation - which isn't necessarily a bad thing - just want you to be aware.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm asking to see if there's any rhyme or reason as to why xpack was left in the yml files, or if that was an oversight.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for checking @bmorelli25.

Up to you if you want to leave the yml+docs as they are, just wanted to ensure it wasn't removed by mistake and is streamlined with documenting other/future deprecated features and settings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool beans - I appreciate you bringing this up! I'm very curious to hear the thought process for why beats did this the way they did. Based on their answers, I'll figure out the path forward and determine what I need to or don't need to do.

bmorelli25 added a commit to bmorelli25/apm-server that referenced this pull request May 23, 2019
bmorelli25 added a commit to bmorelli25/apm-server that referenced this pull request May 23, 2019
bmorelli25 added a commit to bmorelli25/apm-server that referenced this pull request May 23, 2019
bmorelli25 added a commit to bmorelli25/apm-server that referenced this pull request May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support shipping directly to monitoring cluster [docs] Remove Kibana endpoint from Beats docs
4 participants