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

Update README for Prometheus Client Output #2452

Merged
merged 2 commits into from
Jun 19, 2017

Conversation

trastle
Copy link
Contributor

@trastle trastle commented Feb 21, 2017

Update the README of the Prometheus Output with the example configuration.

@@ -1,6 +1,13 @@
# Prometheus Client Service Output Plugin

This plugin starts a Prometheus Client, listening on a port defined in the
configuration file.
This plugin starts a [Prometheus](https://prometheus.io/) Client, it exposes all metrics on `/metrics` to be polled by a Prometheus server.
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you reword this?

Copy link
Contributor Author

@trastle trastle Feb 21, 2017

Choose a reason for hiding this comment

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

The original wording made more sense to me when the port was the only configuration option. When the following block shows the example config it seems odd to specifically call out the port here.

Additionally the config was referred to as port here and address within the example config.

It also seemed important to note that all metrics collected by Telegraf are exposed by the Prometheus client.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, fair enough 👍

## Configuration

# Publish all metrics to /metrics for Prometheus to scrape
[[outputs.prometheus_client]]
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be in an unindented code block, ie, with "```" on either side. See other plugin readmes for an 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 do.

@trastle
Copy link
Contributor Author

trastle commented Feb 21, 2017

@sparrc The other thing I was considering was listing the required and optional parameters. Similar to: https://github.com/influxdata/telegraf/tree/master/plugins/outputs/kafka but this felt like overkill. Thoughts?

@sparrc
Copy link
Contributor

sparrc commented Feb 22, 2017

yes, probably overkill

@danielnelson
Copy link
Contributor

@trastle are you still planning to update or would you like me to finish this up?

@danielnelson danielnelson added this to the 1.4.0 milestone Jun 13, 2017
@trastle
Copy link
Contributor Author

trastle commented Jun 19, 2017

@danielnelson sorry completely forgot about this. @sparrc I have made the requested formatting changes. Sorry for the delay.

@danielnelson danielnelson merged commit 00b37a7 into influxdata:master Jun 19, 2017
jeichorn pushed a commit to jeichorn/telegraf that referenced this pull request Jul 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants