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

Update kubernetes_state to use the new OpenMetricsBaseCheck #1983

Merged
merged 1 commit into from
Sep 1, 2018
Merged

Update kubernetes_state to use the new OpenMetricsBaseCheck #1983

merged 1 commit into from
Sep 1, 2018

Conversation

rishabh
Copy link

@rishabh rishabh commented Aug 2, 2018 •
edited
Loading

What does this PR do?

Updates kubernetes_state to use the new OpenMetricsBaseCheck class.

Motivation

This fixes the check for when we use the new OpenMetricsBaseCheck class.

Review checklist

  • PR has a meaningful title or PR has the no-changelog label attached
  • Feature or bugfix has tests
  • Git history is clean
  • If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo

Additional Notes

@rishabh rishabh requested a review from a team as a code owner August 2, 2018 19:51
@rishabh rishabh force-pushed the rish/prometheus_rework branch from e7b150c to 55edc20 Compare August 3, 2018 02:05
@rishabh rishabh force-pushed the rish/kubernetes_state-prometheus-rework branch from bdd07ff to 620a065 Compare August 3, 2018 03:20
mfpierre
mfpierre reviewed Aug 3, 2018
View reviewed changes
kubernetes_state/tests/test_kubernetes_state.py Outdated

# run check twice to have pod/node mapping
for _ in range(2):
check.check(instance)
print('HEREBRUH')
for i in aggregator._metrics.items():
print i
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like a debugging block 😁

Copy link
Author

Choose a reason for hiding this comment

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

Haha, I thought I caught all my prints.

@l0k0ms l0k0ms mentioned this pull request Aug 3, 2018
Merged
4 tasks
@rishabh rishabh force-pushed the rish/prometheus_rework branch from b3180b7 to 579904a Compare August 3, 2018 15:39
@rishabh rishabh force-pushed the rish/kubernetes_state-prometheus-rework branch from ac8ce72 to 00f8cca Compare August 3, 2018 19:48
@rishabh rishabh force-pushed the rish/prometheus_rework branch 2 times, most recently from be99352 to d5b1cdb Compare August 8, 2018 18:44
@rishabh rishabh force-pushed the rish/kubernetes_state-prometheus-rework branch 2 times, most recently from b19a23e to e66cd28 Compare August 9, 2018 18:51
@rishabh rishabh changed the title Update kubernetes_state to use the new GenericPrometheusCheck Update kubernetes_state to use the new OpenMetricsBaseCheck Aug 9, 2018
@rishabh rishabh force-pushed the rish/prometheus_rework branch 2 times, most recently from 222cee3 to 7bde919 Compare August 21, 2018 20:50
@rishabh rishabh changed the base branch from rish/prometheus_rework to master August 24, 2018 18:05
@rishabh rishabh force-pushed the rish/kubernetes_state-prometheus-rework branch from e66cd28 to 458fbbe Compare August 24, 2018 18:40
@mfpierre mfpierre force-pushed the rish/kubernetes_state-prometheus-rework branch from 458fbbe to 03297b6 Compare August 29, 2018 17:04
@codecov-io
Copy link

codecov-io commented Aug 29, 2018 •
edited
Loading

Codecov Report

Merging #1983 into master will decrease coverage by 7.11%.
The diff coverage is 71.05%.

Impacted file tree graph

@rishabh rishabh force-pushed the rish/kubernetes_state-prometheus-rework branch 3 times, most recently from 75ad33b to 07fc84d Compare August 29, 2018 19:36
@mfpierre mfpierre force-pushed the rish/kubernetes_state-prometheus-rework branch from 07fc84d to 27ab390 Compare August 30, 2018 08:39
xvello
xvello reviewed Aug 30, 2018
View reviewed changes
Copy link
Contributor

@xvello xvello left a comment

Choose a reason for hiding this comment

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

Reviewed common logic, will do a second pass on the magic methods

kubernetes_state/datadog_checks/kubernetes_state/kubernetes_state.py Outdated
self.service_check(sc_name, mapping[label.value], tags=tags)
else:
self.log.debug("Unable to handle %s - unknown condition %s" % (sc_name, label.value))
if 'condition' in sample[self.SAMPLE_LABELS]:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: a bit repetitive, wdyt about starting with

condition = sample[self.SAMPLE_LABELS].get('condition')
if condition:

to remove these duplicate lookups?

kubernetes_state/datadog_checks/kubernetes_state/kubernetes_state.py Outdated
label_value = self._extract_label_value('status', labels)
return label_value, switch.get(self._extract_label_value('condition', labels),
{'service_check_name': None, 'mapping': None})
return labels['status'], switch.get(labels['condition'], {'service_check_name': None, 'mapping': None})
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm concerned we might start throwing KeyErrors if the label were to miss. Shall we use get('condition') instead, to mimic _extract_label_value's return-None-if-not-found behaviour?

@xvello xvello mentioned this pull request Aug 30, 2018
Merged
4 tasks
@mfpierre mfpierre force-pushed the rish/kubernetes_state-prometheus-rework branch from 0c77454 to 7717f3b Compare August 30, 2018 15:18
@rishabh rishabh force-pushed the rish/kubernetes_state-prometheus-rework branch from 7717f3b to 347ba60 Compare August 30, 2018 18:27
xvello
xvello approved these changes Aug 31, 2018
View reviewed changes
Copy link
Contributor

@xvello xvello left a comment

Choose a reason for hiding this comment

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

LGTM 🚤

xvello
xvello previously approved these changes Aug 31, 2018
View reviewed changes
Copy link
Contributor

@xvello xvello left a comment

Choose a reason for hiding this comment

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

LGTM 🚤

xvello
xvello reviewed Aug 31, 2018
View reviewed changes
kubernetes_state/datadog_checks/kubernetes_state/kubernetes_state.py Outdated
"""
Set up the kubernetes_state instance so it can be used in OpenMetricsBaseCheck
"""
endpoint = instance.get('kube_state_url')
Copy link
Contributor

Choose a reason for hiding this comment

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

@rishabh same here 4b6555a

@mfpierre mfpierre force-pushed the rish/kubernetes_state-prometheus-rework branch from 1381cd9 to 8232c4a Compare August 31, 2018 17:26
@mfpierre
Use OpenMetricsBaseCheck and drop protobuf support
ca02ead 
PR feedback and remove unused fixture

Use PrometheusCheck defaults

Do not modify original instance

Add label_to_hostname_suffix
@mfpierre mfpierre force-pushed the rish/kubernetes_state-prometheus-rework branch from 8232c4a to ca02ead Compare August 31, 2018 22:44
@ofek ofek merged commit fd3c6c0 into master Sep 1, 2018
@ofek ofek deleted the rish/kubernetes_state-prometheus-rework branch September 1, 2018 02:32
nmuesch pushed a commit that referenced this pull request Nov 1, 2018
@nmuesch
Use OpenMetricsBaseCheck and drop protobuf support (#1983)
8b6fb43 
PR feedback and remove unused fixture

Use PrometheusCheck defaults

Do not modify original instance

Add label_to_hostname_suffix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xvello xvello xvello left review comments

@mfpierre mfpierre mfpierre left review comments

@ofek ofek ofek approved these changes

Assignees
No one assigned
Labels
containers integration/kubernetes_state
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rishabh @codecov-io @mfpierre @xvello @ofek