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 metrics option type for legacy OpenMetrics config spec #9318

Merged
merged 3 commits into from
May 12, 2021

Conversation

jejikenwogu
Copy link
Contributor

What does this PR do?

Fix incorrect validation spec for istio integration.

Motivation

The istio validation spec expects metrics to be a list of strings (Sequence[str]). It is in fact supposed to be a list of maps (Sequence[Mapping[str, str]]).

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 May 12, 2021

Codecov Report

Merging #9318 (32cfb39) into master (4727cd3) will increase coverage by 0.00%.
The diff coverage is n/a.

Flag Coverage Δ
amazon_msk 89.53% <ø> (ø)
datadog_checks_dev 80.55% <ø> (ø)
istio 93.18% <ø> (+0.56%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...atadog_checks/ibm_mq/collectors/stats_collector.py 88.70% <0.00%> (-3.23%) ⬇️
couch/datadog_checks/couch/couch.py 88.25% <0.00%> (-0.76%) ⬇️
mysql/datadog_checks/mysql/innodb_metrics.py 65.66% <0.00%> (+1.28%) ⬆️
istio/tests/conftest.py 84.54% <0.00%> (+1.81%) ⬆️
zk/tests/conftest.py 94.44% <0.00%> (+2.22%) ⬆️

@ofek ofek changed the title Fix for #9317: Fix incorrect validation spec for istio integration Fix metrics option type for legacy OpenMetrics config spec May 12, 2021
Copy link
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

@jejikenwogu Thank you very much!

@ofek ofek merged commit 23b5f36 into DataDog:master May 12, 2021
@jejikenwogu jejikenwogu deleted the fixIstioConfigValidtor branch May 12, 2021 05:00
@boeboe
Copy link

boeboe commented May 28, 2021

This "fix" brakes the Istio integration

2021-05-28 22:49:54 UTC | CORE | ERROR | (pkg/collector/runner/runner.go:301 in work) | Error running check istio: [{"message": "Detected 1 error while loading configuration model `InstanceConfig`:\nmetrics -> 1\n  str type expected", "traceback": "Traceback (most recent call last):\n  File \"/opt/datadog-agent/embedded/lib/python3.8/site-packages/datadog_checks/base/checks/base.py\", line 971, in run\n initialization()\n  File \"/opt/datadog-agent/embedded/lib/python3.8/site-packages/datadog_checks/base/checks/base.py\", line 396, in load_configuration_models\n    instance_config = self.load_configuration_model(package_path, 'InstanceConfig', raw_instance_config)\n  File \"/opt/datadog-agent/embedded/lib/python3.8/site-packages/datadog_checks/base/checks/base.py\", line 436, in load_configuration_model\n    raise_from(ConfigurationError('\\n'.join(message_lines)), None)\n  File \"<string>\", line 3, in raise_from\ndatadog_checks.base.errors.ConfigurationError: Detected 1 error while loading configuration model `InstanceConfig`:\nmetrics -> 1\n  str type expected\n"}]

Helm values:

datadog:
  confd:
    istio.yaml: |-
      ad_identifiers:
        - am-istio
      init_config:

      instances:
        - istiod_endpoint: http://istiod.istio-system:15014/metrics
          send_histograms_buckets: true
        - citadel_endpoint: http://istio-citadel.istio-system:15014/metrics
          send_histograms_buckets: true
        - istio_mesh_endpoint: http://%%host%%:15090/stats/prometheus
          send_histograms_buckets: true

@ofek
Copy link
Contributor

ofek commented Jun 2, 2021

@boeboe That's the error that this resolves. Look out for Agent 7.29.0 in ~3 weeks

@jbfavre
Copy link

jbfavre commented Jun 8, 2021

@ofek when has this error been introduced ?
For me, running 7.26.0 is fine, running 7.28.2 isn't.

@boeboe
Copy link

boeboe commented Jun 8, 2021

7.27.0 was still working for me. It broke in 7.28.0.

@jbfavre
Copy link

jbfavre commented Jun 9, 2021

@boeboe thansk for the answer.
Will use 7.27.0 until 7.29.0 release.

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