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

Do not fail on octet stream content type for OpenMetrics #5843

Merged

Conversation

AlexandreYang
Copy link
Member

@AlexandreYang AlexandreYang commented Feb 24, 2020

What does this PR do?

  1. Set encoding to utf-8 when there is no encoding to avoid failure down the road.

Content might be in bytes when the service exposing the openmetrics endpoint doesn't set correctly the Content-Type and Encoding.

The openmetrics/prometheus exposition format suggest using utf-8: https://github.com/prometheus/docs/blob/master/content/docs/instrumenting/exposition_formats.md

  1. Explicitly set the content type we accept to text/plain

Motivation

Currently, if bytes content is received. In Python 3 (works for Python 2), the openmetrics check will fail with the following issue:

openmetrics (1.3.0)
-------------------
  Instance ID: openmetrics:idm-mhcf74:efee10b119b1d5b0 [ERROR]
  Configuration Source: file:/etc/datadog-agent/conf.d/openmetrics.yaml
  Total Runs: 13
  Metric Samples: Last Run: 0, Total: 0
  Events: Last Run: 0, Total: 0
  Service Checks: Last Run: 1, Total: 13
  Average Execution Time : 11ms
  Error: startswith first arg must be bytes or a tuple of bytes, not str
  Traceback (most recent call last):
    File "/opt/datadog-agent/embedded/lib/python3.7/site-packages/datadog_checks/base/checks/base.py", line 678, in run
      self.check(instance)
    File "/opt/datadog-agent/embedded/lib/python3.7/site-packages/datadog_checks/base/checks/openmetrics/base_check.py", line 82, in check
      self.process(scraper_config)
    File "/opt/datadog-agent/embedded/lib/python3.7/site-packages/datadog_checks/base/checks/openmetrics/mixins.py", line 378, 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 352, in scrape_metrics
      for metric in self.parse_metric_family(response, scraper_config):
    File "/opt/datadog-agent/embedded/lib/python3.7/site-packages/datadog_checks/base/checks/openmetrics/mixins.py", line 301, in parse_metric_family
      for metric in text_fd_to_metric_families(input_gen):
    File "/opt/datadog-agent/embedded/lib/python3.7/site-packages/prometheus_client/parser.py", line 138, in text_fd_to_metric_families
      if line.startswith('#'):
  TypeError: startswith first arg must be bytes or a tuple of bytes, not str

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

@codecov
Copy link

codecov bot commented Feb 24, 2020

Codecov Report

Merging #5843 into master will increase coverage by 0.09%.
The diff coverage is n/a.

Impacted Files Coverage Δ
disk/tests/common.py 63.63% <0.00%> (-36.37%) ⬇️
disk/tests/conftest.py 85.71% <0.00%> (-11.43%) ⬇️
disk/tests/test_filter.py 96.45% <0.00%> (-3.55%) ⬇️
disk/datadog_checks/disk/disk.py 83.88% <0.00%> (-2.07%) ⬇️
airflow/tests/conftest.py 100.00% <0.00%> (ø) ⬆️
ibm_mq/datadog_checks/ibm_mq/ibm_mq.py
nagios/datadog_checks/nagios/nagios.py
kube_apiserver_metrics/tests/conftest.py
exchange_server/tests/common.py
activemq_xml/tests/conftest.py
... and 364 more

@AlexandreYang
Copy link
Member Author

Closing in favour of #5847

@ofek ofek deleted the alex/fix_openmetrics_octet_stream_content_type branch February 24, 2020 22:18
@AlexandreYang AlexandreYang restored the alex/fix_openmetrics_octet_stream_content_type branch February 28, 2020 10:30
@AlexandreYang AlexandreYang reopened this Feb 28, 2020
@AlexandreYang AlexandreYang changed the title Do not fail on octet stream content type Do not fail on octet stream content type1 Feb 28, 2020
@AlexandreYang AlexandreYang changed the title Do not fail on octet stream content type1 [HOLD] Do not fail on octet stream content type1 Feb 28, 2020
@AlexandreYang AlexandreYang force-pushed the alex/fix_openmetrics_octet_stream_content_type branch from 3603417 to eb2699a Compare February 28, 2020 10:32
@AlexandreYang AlexandreYang changed the title [HOLD] Do not fail on octet stream content type1 [HOLD] Do not fail on octet stream content type Feb 28, 2020
@AlexandreYang AlexandreYang changed the title [HOLD] Do not fail on octet stream content type Do not fail on octet stream content type Feb 28, 2020
Copy link
Contributor

@xornivore xornivore left a comment

Choose a reason for hiding this comment

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

👍

@AlexandreYang AlexandreYang merged commit 9ede682 into master Mar 2, 2020
@AlexandreYang AlexandreYang deleted the alex/fix_openmetrics_octet_stream_content_type branch March 2, 2020 17:25
@ofek ofek changed the title Do not fail on octet stream content type Do not fail on octet stream content type for OpenMetrics Mar 2, 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.

4 participants