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 improvements #548

Merged
merged 31 commits into from
Feb 1, 2018
Merged

Docs improvements #548

merged 31 commits into from
Feb 1, 2018

Conversation

roncohen
Copy link
Contributor

@roncohen roncohen commented Jan 25, 2018

added:

  • configuration through a file
  • command reference
  • logging configuration
  • path config
  • directory layout
  • dashboard set and dashboard config
  • elasticsearch output config
  • enabling SSL for the Elasticsearch output
  • env var configuration
  • configuring the Kibana endpoint
  • configuring auto template loading
  • manual template loading

Moved some sections around:

screenshot 2018-01-25 21 31 21

  • Main headline is "APM Server Reference (Beta)" now
  • Reordered the intake API under a section

relies on elastic/beats#6184
related: #440

@roncohen
Copy link
Contributor Author

There's a bunch of this we need to test before we merge

Use `sudo` to run the following commands if:

* the config file is owned by `root`, or
* {beatname_uc} is configured to capture data that requires `root` access
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a bit weird for APM Server, but i guess this could be taken to mean if you set it up to listen on port < 1025, you'll need to run it as root.

@roncohen
Copy link
Contributor Author

When this gets merged, we should update our copy: elastic/beats#6186


include::./transaction-api.asciidoc[]

include::./error-api.asciidoc[]
Copy link
Contributor

Choose a reason for hiding this comment

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

@simitt
Copy link
Contributor

simitt commented Jan 26, 2018

@roncohen could you add a note regarding this Issue #519 in the dashboards doc section?


["source","sh",subs="attributes,callouts"]
----------------------------------------------------------------------
PS > {beatname_lc} setup --dashboards
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not had a chance to test this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@roncohen
Copy link
Contributor Author

@simitt is this what you had in mind?: elastic/beats#6197

@roncohen roncohen changed the title WIP on docs improvements Docs improvements Jan 26, 2018
Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

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

I am not through yet, but here are some first review comments.

include::./installing-on-windows.asciidoc[]
Copy link
Contributor

Choose a reason for hiding this comment

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

I liked having the Docker guide also referenced in the installation section, as it is an alternative to installing APM Server nativly.

The distinction into downloading or installing from repos/on windows, seems a bit constructed, as for windows you also need to download first.
How about having 4 sections that are subpages describing the options for the specific OS:

  • Installation on Windows
  • Installation on Linux
  • Installation on Mac OS
  • Installation with Docker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a good idea. I'll merge this now so we at least have something and then we can iterate on it

you'll find the configuration file at +/etc/{beatname_lc}/{beatname_lc}.yml+. Under
Docker, it's located at +/usr/share/{beatname_lc}/{beatname_lc}.yml+. For mac and win,
look in the archive that you just extracted. There’s also a full example
configuration file called +{beatname_lc}.reference.yml+ that shows all non-deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

how about actively supported instead of non-deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that non-deprecated make it explicit that we're not including deprecated stuff. "actively supported" could mean that we support it in the discuss forum or on subscriptions.


See the
{libbeat}/config-file-format.html[Config File Format] section of the
_Beats Platform Reference_ for more about the structure of the config file.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not rendered as a link for me:

screen shot 2018-01-29 at 11 09 32

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same, i think it should be fixed in the live version though

==== `logging.level`

Minimum log level. One of `debug`, `info`, `warning`, or `error`. The default
log level is `info`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested this and found the following:

  • log level warning actually needs to be warn, otherwise an error is thrown and the server doesn't start.
  • log level error still outputs info and warn logs.
  • log level warn still outputs info logs.

We'll need to address this in beats.

Copy link
Contributor

Choose a reason for hiding this comment

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

@roncohen after this being fixed in beats I think you need to change warn back to warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simitt already done! :)

When true, writes all logging output to the Windows Event Log.

[float]
==== `logging.to_files`
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you test this? I tried to use log files, by copying over the example, but the files have not been created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved on chat, works

need to configure the `setup.template.name` and `setup.template.pattern` options
(see <<configuration-template>>). If you are using the pre-built Kibana
dashboards, you also need to set the `setup.dashboards.index` option (see
<<configuration-dashboards>>).
Copy link
Contributor

Choose a reason for hiding this comment

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

You could also mention that the index for Sourcemaps needs to be changed here. (Except if we don't want to cross reference to experimental features).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's wait for now


*`mapping`*: Dictionary mapping index names to new names

*`default`*: Default string value if `mapping` does not find a match.
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 the mapping and default option rather confusing, as they don't show up in the example, and I think the specified index pattern is simply used as default if nothing else found.
If the two options can be specifically set then they should go into the example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will make a note to suggest to beats


The number of times to retry publishing an event after a publishing failure.
After the specified number of retries, the events are typically dropped.
Some Beats, such as Filebeat, ignore the `max_retries` setting and retry until all
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this line as APM Server has nothing to do with Filebeat

==== Load the template manually

To load the template manually, run the <<setup-command,`setup`>> command. A
connection to Elasticsearch is required. If Logstash output is enabled, you need
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't support logstash output.

//// Use the following include to pull this content into a doc file:
//// include::../../libbeat/docs/command-reference.asciidoc[]
//////////////////////////////////////////////////////////////////////////

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that we should mention machine learning here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed here: elastic/beats@91b8c7b

Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

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

Thanks for adding all this documentation @roncohen. Had a couple of comments, but overall looks quite good.

This requires a Kibana endpoint configuration. You should have configured the
endpoint earlier when you
<<{beatname_lc}-configuration,configured {beatname_uc}>>. If you didn't,
configure it now.
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually the config part comes after. this, so saying you should have configured the endpoint earlier is a bit confusing.
I suggest to completely remove this NOTE section. Instead only link to the configure dashboard loading, which itself should link to the Kibana API setup section.


Make sure Kibana is running before you perform this step. If you are accessing a
secured Kibana instance, make sure you've configured credentials as described in
<<{beatname_lc}-configuration>>.
Copy link
Contributor

Choose a reason for hiding this comment

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

this renders to Configuring APM Server with a capital C in the middle of the sentence.

<<{beatname_lc}-configuration>>.

To set up the Kibana dashboards for {beatname_uc}, use the appropriate command
for your system.
Copy link
Contributor

Choose a reason for hiding this comment

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

The next section is an overview over APM Server cmds, as the instruction for the Power Shell is valid for all instructions, how about adding the Powershell instruction here instead:

screen shot 2018-01-29 at 14 34 15

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, will do it in beats in a separate PR when this is done.


From the PowerShell prompt, change to the directory where you installed {beatname_uc},
and run:

Copy link
Contributor

Choose a reason for hiding this comment

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

Something is very strange regarding including setup cmd here, I see Power shell instructions on a non-windows machine:

screen shot 2018-01-29 at 14 42 14

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think thats fixed then it goes live


ifndef::only-elasticsearch[]
NOTE: A connection to Elasticsearch is required to load the index template. If
the output is Logstash, you must
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove all descriptions for Logstash output as we don't describe it anywhere as possible output option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see the ifndef::only-elasticsearch[] line above. only-elasticsearch is defined for APM Server so these lines should not show up.

Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

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

Apart from the one comment I left LGTM.

Could you please ensure to create Issues for comments where you mentioned to fix within the next iterations.

@roncohen roncohen merged commit 7b69482 into elastic:master Feb 1, 2018
roncohen added a commit to roncohen/apm-server that referenced this pull request Feb 1, 2018
Major restructure. Adds many documents from beats that are applicable to APM Server.
roncohen added a commit that referenced this pull request Feb 1, 2018
Major restructure. Adds many documents from beats that are applicable to APM Server.
roncohen added a commit to roncohen/apm-server that referenced this pull request Feb 1, 2018
Major restructure. Adds many documents from beats that are applicable to APM Server.
roncohen added a commit that referenced this pull request Feb 1, 2018
Major restructure. Adds many documents from beats that are applicable to APM Server.
@roncohen roncohen mentioned this pull request Feb 5, 2018
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.

2 participants