-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Support prometheus endpoint #7098
Conversation
Codecov Report
|
# 'consul_raft_replication_installSnapshot': 'raft.replication.installSnapshot', | ||
# 'consul_raft_replication_heartbeat': 'raft.replication.heartbeat', | ||
# 'consul_raft_replication_appendEntries_rpc': 'raft.replication.appendEntries.rpc', | ||
# 'consul_raft_replication_appendEntries_logs': 'raft.replication.appendEntries.logs', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting for the consul PRs to be merged and having a new release. Note that we can still merge this PR and add the http
and raft.replication
metrics prometheus support later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this PR to support the prometheus endpoint, even if some metrics are missing. We can add them later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
editorial review completed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! We should also document the consul.prometheus.health
service check in the service_checks.json
and README.
Co-authored-by: Christine Chen <ChristineTChen@users.noreply.github.com> Co-authored-by: sarina-dd <57639676+sarina-dd@users.noreply.github.com>
Co-authored-by: Julia <611228+hithwen@users.noreply.github.com>
Co-authored-by: Christine Chen <ChristineTChen@users.noreply.github.com>
3016db7
to
5ad7637
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just have a question about dogstatsd vs prom metrics
raise ConfigurationError( | ||
'The DogStatsD method and the Prometheus method are both enabled. Please choose only one.' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the supported dogstatsd metrics all available from the prometheus endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes they should be all available https://www.consul.io/docs/agent/telemetry, even if we don't retrieve them all due to the tag issue
What does this PR do?
Support prometheus endpoint for
consul
integration.Motivation
A prometheus endpoint is available since Consul 1.1.0 (https://github.com/hashicorp/consul/blob/master/CHANGELOG.md#110-may-11-2018) and exposes metrics that are currently directly handled by dogstatsd (see the README https://github.com/DataDog/integrations-core/tree/master/consul#dogstatsd).
Additional Notes
Some of these metrics have information in the name that could be useful as labels.
I opened some PRs to change it if possible.
See:
Review checklist (to be filled by reviewers)
changelog/
andintegration/
labels attached