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

Support prometheus endpoint #7098

Merged
merged 20 commits into from
Aug 19, 2020
Merged

Support prometheus endpoint #7098

merged 20 commits into from
Aug 19, 2020

Conversation

coignetp
Copy link
Contributor

@coignetp coignetp commented Jul 9, 2020

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)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

@codecov
Copy link

codecov bot commented Jul 9, 2020

Codecov Report

Merging #7098 into master will increase coverage by 5.03%.
The diff coverage is 82.08%.

Impacted Files Coverage Δ
consul/tests/test_unit.py 100.00% <ø> (ø)
consul/datadog_checks/consul/consul.py 87.87% <73.33%> (-2.19%) ⬇️
consul/datadog_checks/consul/metrics.py 100.00% <100.00%> (ø)
consul/tests/common.py 100.00% <100.00%> (ø)
consul/tests/conftest.py 100.00% <100.00%> (ø)
consul/tests/test_integration.py 98.24% <100.00%> (+0.62%) ⬆️
process/datadog_checks/process/__about__.py
...b_runner/datadog_checks/gitlab_runner/__about__.py
elastic/tests/test_config.py
lighttpd/datadog_checks/lighttpd/__init__.py
... and 788 more

# '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',
Copy link
Contributor Author

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.

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 merge this PR to support the prometheus endpoint, even if some metrics are missing. We can add them later.

@coignetp coignetp marked this pull request as ready for review July 10, 2020 14:40
consul/README.md Outdated Show resolved Hide resolved
consul/README.md Outdated Show resolved Hide resolved
consul/README.md Outdated Show resolved Hide resolved
consul/datadog_checks/consul/consul.py Outdated Show resolved Hide resolved
consul/datadog_checks/consul/consul.py Outdated Show resolved Hide resolved
consul/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
consul/datadog_checks/consul/consul.py Outdated Show resolved Hide resolved
Copy link
Contributor

@sarina-dd sarina-dd left a comment

Choose a reason for hiding this comment

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

editorial review completed

consul/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
consul/datadog_checks/consul/consul.py Outdated Show resolved Hide resolved
consul/datadog_checks/consul/data/conf.yaml.example Outdated Show resolved Hide resolved
consul/datadog_checks/consul/data/conf.yaml.example Outdated Show resolved Hide resolved
consul/README.md Outdated Show resolved Hide resolved
consul/README.md Outdated Show resolved Hide resolved
consul/README.md Outdated Show resolved Hide resolved
consul/datadog_checks/consul/consul.py Outdated Show resolved Hide resolved
ChristineTChen
ChristineTChen previously approved these changes Jul 24, 2020
Copy link
Contributor

@ChristineTChen ChristineTChen left a 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.

coignetp and others added 10 commits August 18, 2020 16:49
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>
Copy link
Contributor

@ChristineTChen ChristineTChen left a 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

Comment on lines +55 to +57
raise ConfigurationError(
'The DogStatsD method and the Prometheus method are both enabled. Please choose only one.'
)
Copy link
Contributor

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?

Copy link
Contributor Author

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

@coignetp coignetp merged commit d872d43 into master Aug 19, 2020
@coignetp coignetp deleted the paul/consul-prometheus branch August 19, 2020 12:36
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