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

Add docs for autodiscover #5868

Merged
merged 4 commits into from
Dec 14, 2017
Merged

Add docs for autodiscover #5868

merged 4 commits into from
Dec 14, 2017

Conversation

exekias
Copy link
Contributor

@exekias exekias commented Dec 13, 2017

This PR adds documentation for #5245

@exekias exekias added docs Filebeat Filebeat in progress Pull request is currently in progress. Metricbeat Metricbeat needs_backport PR is waiting to be backported to other branches. review v6.1.0 labels Dec 13, 2017
@exekias exekias removed the in progress Pull request is currently in progress. label Dec 13, 2017
@exekias exekias requested a review from dedemorton December 13, 2017 12:06
exclude_lines: ["^\\s+[\\-`('.|_]"] # drop asciiart lines
-------------------------------------------------------------------------------------

This will launch `redis` prospector for all containers running a image with `redis` in its name.
Copy link
Contributor

Choose a reason for hiding this comment

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

By "this" you mean the above? Seems to start the docker prospector for Redis logs, not the redis prospector. Or the below which seems to start the redis module :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right 😇 , it launches the docker prospector for the redis container, will fix that, thanks!

Copy link
Contributor

@dedemorton dedemorton left a comment

Choose a reason for hiding this comment

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

Nice first cut! I'd like to do a more in-depth edit of the content later on, but it's a good first cut for the initial release.

Autodiscover allows you to watch for system changes and dynamically adapt settings to them, as they happen.
This is specially useful when running your infrastructure on containers.

When you run an application on containers it becomes a moving target to the monitoring system, Autodiscover
Copy link
Contributor

Choose a reason for hiding this comment

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

comma splice. Change the comma to a period here:

...monitoring system. Autodiscover...

This is specially useful when running your infrastructure on containers.

When you run an application on containers it becomes a moving target to the monitoring system, Autodiscover
allows you to automatically detect what's running and update settings to monitor it.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to add a specific example here. Might be tricky since this is shared content, but worth exploring because I think we are being too vague here. (We can solve this later)

When you run an application on containers it becomes a moving target to the monitoring system, Autodiscover
allows you to automatically detect what's running and update settings to monitor it.

You can define configuration templates for different containers, Autodiscover subsystem will use them
Copy link
Contributor

Choose a reason for hiding this comment

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

Will users know what we mean by configuration templates here?

Also, change the comma to a period:

"...different containers. The autodiscover subsystem...

[float]
=== Configuration options

Autodiscover settings are defined under {beatname_lc}.autodiscover field, to enable it you must configure
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd change this around a bit to be consistent with similar sections in the docs. Would also make a few small edits:

You define autodiscover settings in the  +{beatname_lc}.autodiscover+ section of the +{beatname_lc}.yml+
config file. To enable autodiscover, you specify a list of providers.

[float]
=== Providers

Autodiscover providers work by watching for events on the system and translating that into internal autodiscover
Copy link
Contributor

Choose a reason for hiding this comment

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

Change:

...and translating that into...

To:

...and translating those events into....

Conditions match events from the provider, the use the <<conditions,same format from processors conditions>>.

Confiuration templates can contain variables from the autodiscover event, they can be accessed under `data` namespace.
For example, with the example event, "`${data.port}`" will result into `6379`.
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 say:

"${data.port}" resolves to 6379.

or

"${data.port}" results in 6379.

hosts: "${data.host}:6379"
-------------------------------------------------------------------------------------

This will launch `redis` module for all containers running a image with `redis` in its name.
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to:

This configuration launches a redis module for all containers running an image with redis in the name.

exclude_lines: ["^\\s+[\\-`('.|_]"] # drop asciiart lines
-------------------------------------------------------------------------------------

This will launch `docker` logs prospector for all containers running a image with `redis` in its name.
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to:

This configuration launches a docker logs prospector for all containers running an image with redis in the name.


This will launch `docker` logs prospector for all containers running a image with `redis` in its name.

When defining modules, you can override default prospector to use docker:
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to:

If you are using modules, you can override the default prospector and use the docker prospector instead.

Autodiscover settings are defined under {beatname_lc}.autodiscover field, to enable it you must configure
a list of providers to be used.

["source","yaml",subs="attributes"]
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH, I'm not sure this config example is useful without the details of the provider config. I'd suggest removing the entire "Configuration options" section because you don't describe the options. Then you can just tack on the above paragraph to the end of the intro:


...to monitor services as they start running.

You define autodiscover settings in the  +{beatname_lc}.autodiscover+ section of the +{beatname_lc}.yml+
config file. To enable autodiscover, you specify a list of providers.

+[float]
+=== Providers

I'd like to make other changes to the intro, but that's probably good enough for now.

@exekias
Copy link
Contributor Author

exekias commented Dec 13, 2017

Thanks for the review and pointers @dedemorton! It should be ready for a second pass :)

Copy link
Contributor

@dedemorton dedemorton left a comment

Choose a reason for hiding this comment

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

Just one small change. otherwise lgtm

allows you to automatically detect what's running and update settings to monitor it.

You can define configuration templates for different containers, Autodiscover subsystem will use them
You can define configuration templates for different containers. Autodiscover subsystem will use them
to monitor services as they start running.

[float]
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove lines 13 and 14. This content doesn't warrant a separate container.

Copy link
Contributor

@adriansr adriansr left a comment

Choose a reason for hiding this comment

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

LGTM

@tsg tsg merged commit 223c074 into elastic:master Dec 14, 2017
exekias added a commit to exekias/beats that referenced this pull request Dec 14, 2017
* Add docs for autodiscover

* Fix comment on filebeat autodiscover prospector settings

* Apply review comments

* Remove configuration options section

(cherry picked from commit 223c074)
@exekias exekias removed the needs_backport PR is waiting to be backported to other branches. label Dec 14, 2017
andrewkroh pushed a commit that referenced this pull request Dec 15, 2017
* Add docs for autodiscover

* Fix comment on filebeat autodiscover prospector settings

* Apply review comments

* Remove configuration options section

(cherry picked from commit 223c074)
@dedemorton dedemorton mentioned this pull request Jan 22, 2018
7 tasks
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
* Add docs for autodiscover

* Fix comment on filebeat autodiscover prospector settings

* Apply review comments

* Remove configuration options section

(cherry picked from commit 3dcb70e)
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.

4 participants