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

Fix http handler #5434

Merged
merged 4 commits into from
Jan 10, 2020
Merged

Fix http handler #5434

merged 4 commits into from
Jan 10, 2020

Conversation

AlexandreYang
Copy link
Member

@AlexandreYang AlexandreYang commented Jan 10, 2020

What does this PR do?

Before, if the prometheus_url is changed after the scrapper is created, we won't be able to find the handler. Hence leading to a KeyError.

Initial feature PR: #5414

Example:

Average Execution Time : 223ms
      Error: 'https://10.128.246.166:10250/metrics/cadvisor'
      Traceback (most recent call last):
        File "/opt/datadog-agent/embedded/lib/python3.7/site-packages/datadog_checks/base/checks/base.py", line 673, in run
          self.check(instance)
        File "/opt/datadog-agent/embedded/lib/python3.7/site-packages/datadog_checks/kubelet/kubelet.py", line 285, in check
          self.process(self.cadvisor_scraper_config, metric_transformers=self.transformers)
        File "/opt/datadog-agent/embedded/lib/python3.7/site-packages/datadog_checks/base/checks/openmetrics/mixins.py", line 401, in process
          for metric in self.scrape_metrics(scraper_config):
        File "/opt/datadog-agent/embedded/lib/python3.7/site-packages/datadog_checks/base/checks/openmetrics/mixins.py", line 359, in scrape_metrics
          response = self.poll(scraper_config)
        File "/opt/datadog-agent/embedded/lib/python3.7/site-packages/datadog_checks/base/checks/openmetrics/mixins.py", line 559, in poll
          response = self.send_request(endpoint, scraper_config, headers)
        File "/opt/datadog-agent/embedded/lib/python3.7/site-packages/datadog_checks/base/checks/openmetrics/mixins.py", line 583, in send_request
          return self.http_handlers[scraper_config['prometheus_url']].get(endpoint, stream=True, **kwargs)
      KeyError: 'https://10.128.246.166:10250/metrics/cadvisor'

Motivation

Additional Notes

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

@AlexandreYang AlexandreYang changed the title Fix http handler [WIP] Fix http handler Jan 10, 2020
@codecov
Copy link

codecov bot commented Jan 10, 2020

@AlexandreYang AlexandreYang changed the title [WIP] Fix http handler Fix http handler Jan 10, 2020
@AlexandreYang AlexandreYang marked this pull request as ready for review January 10, 2020 13:21
@AlexandreYang AlexandreYang requested review from a team as code owners January 10, 2020 13:21
@@ -57,7 +57,7 @@ def __init__(self, *args, **kwargs):

super(OpenMetricsBaseCheck, self).__init__(*args, **kwargs)
self.config_map = {}
self.http_handlers = {}
self._http_handlers = {}
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: Let's not expose the http handlers and use get_http_handler

mfpierre
mfpierre previously approved these changes Jan 10, 2020
Copy link
Contributor

@mfpierre mfpierre left a comment

Choose a reason for hiding this comment

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

LGTM

mfpierre
mfpierre previously approved these changes Jan 10, 2020
@AlexandreYang AlexandreYang merged commit ecd9fa7 into master Jan 10, 2020
@AlexandreYang AlexandreYang deleted the alex/fix_http_handler branch January 10, 2020 15:15
AlexandreYang added a commit that referenced this pull request Jan 30, 2020
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.

3 participants