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.

Sign up for GitHub

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

Jump to bottom

Support for gathering metrics from prometheus endpoint for the kubelet itself. #1581

Merged
merged 6 commits into from
May 25, 2018
Merged

Support for gathering metrics from prometheus endpoint for the kubelet itself. #1581

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cac5d98
Add support for scraping metrics from prometheus endpoint for kubelet…
May 18, 2018
a3c1443
Test cleanup.
May 18, 2018
3dd315e
Code review changes.
May 22, 2018
ee90155
Fix failing test.
May 22, 2018
3a55e38
Log warning for deprecated config option.
May 23, 2018
6a20339
Remove etcd metrics from kubelet prometheus endpoint.
May 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion datadog_checks_base/datadog_checks/checks/prometheus/base_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,13 @@
from .. import AgentCheck
from ...errors import CheckException


class Scraper(PrometheusScraper):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name of this class is admittedly bad and I'd take any suggestions for renaming. Maybe rename the mixin to PrometheusScraperMixin and also have PrometheusScraper as the standalone version?

Copy link
Contributor

@mfpierre mfpierre May 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Your suggestion sounds good to me, you'll need to update a few things (check base,exports, Istio check) for renaming.

I'd say it'd be nice to have but as it's in the prometheus module it's not shocking to be named just Scraper.

"""
This class scrapes a prometheus endpoint and submits the metrics on behalf of a check. This class
is used by checks that scrape more than one prometheus endpoint.
"""

def __init__(self, check):
super(Scraper, self).__init__()
self.check = check
Expand Down Expand Up @@ -65,7 +71,7 @@ def _submit_service_check(self, *args, **kwargs):

class GenericPrometheusCheck(AgentCheck):
"""
GenericPrometheusCheck is a class that helps instanciating PrometheusCheck only
GenericPrometheusCheck is a class that helps instantiating PrometheusCheck only
with YAML configurations. As each check has it own states it maintains a map
of all checks so that the one corresponding to the instance is executed

Expand Down
2 changes: 1 addition & 1 deletion datadog_checks_base/datadog_checks/checks/prometheus/prometheus_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It must be noted that if the check implementing this class is not officially
# supported
# its metrics will count as cutom metrics and WILL impact billing.
# its metrics will count as custom metrics and WILL impact billing.
#
# Minimal config for checks based on this class include:
# - implementing the check method
Expand Down
6 changes: 4 additions & 2 deletions kubelet/conf.yaml.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@ instances:
### This is the default setting. See next section for legacy clusters.
###
#
# url of the kubelet metrics endpoint
# url of the cadvisor metrics endpoint
# Pass an empty string, or set the cadvisor_port option to disable
# prometheus metrics collection
# prometheus cadvisor metrics collection
# metrics_endpoint: http://10.8.0.1:10255/metrics/cadvisor
#
Copy link
Contributor Author

@devonboyer devonboyer May 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Does it make more sense to have a single config option for kubelet_api_endpoint:http://10.8.0.1:10255 and a collect_cadvisor_metrics flag or have separate options for kubelet_metrics_endpoint:http://10.8.0.1:10255/metrics/cadvisor and cadvisor_metrics_endpoint:http://10.8.0.1:10255/metrics?

Copy link
Member

Choose a reason for hiding this comment

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

i like the first option better, but it won't be easy to keep retro-compatibility. The second option sounds simpler. We can update the yaml with metrics_endpoint --> cadvisor_endpoint and add kubelet_endpoint, and fallback to metrics_endpoint in the check if no cadvisor_endpoint is found in the config object.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO the best woud be to have only kubelet_api_endpoint and then we can guess /metrics and /metrics/cadvisor because there's no way to change the path in the kubelet.
So I guess we can keep the current metrics_endpoint parse it and add paths if needed

# kubelet_metrics_endpoint: http://10.8.0.1:10255/metrics
#
Copy link
Contributor Author

@devonboyer devonboyer May 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This ended up being much more straightforward to be able to turn off kublet metrics and/or cadvisor metrics separately by setting the option to "" than trying to share kubelet_api_endpoint between them.

I added a deprecated note to metrics_endpoint in favour of the more clearly named cadvisor_metrics_endpoint. Does it make sense to log a warning/info if someone is using it?

Copy link
Member

Choose a reason for hiding this comment

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

yes, a deprecation warning would be nice.

# The histogram buckets can be noisy and generate a lot of tags.
# send_histograms_buckets controls whether or not you want to pull them.
#
Expand Down
Loading