Properly utilize the provided metrics_mapper

[casidiablo]

What does this PR do?

The current implementation of the Prometheus check ignores completely the values of metrics_mapper, because the code is pretty much building it using the original metric names.

Instead, we should use the mappings provided by the user in the metrics_mapper dictionary.

Motivation

Given an example config like this:

    metrics:
      - cp_kafka_server_replicamanager_underreplicatedpartitions
    type_overrides:
      cp_kafka_server_replicamanager_underreplicatedpartitions: gauge
    metrics_mapper:
      cp_kafka_server_replicamanager_underreplicatedpartitions: server.replica_manager.under_replicated_partitions

We should emit metrics named server.replica_manager.under_replicated_partitions instead of cp_kafka_server_replicamanager_underreplicatedpartitions which is what the agent does currently.

Additional Notes

N/A

Review checklist (to be filled by reviewers)

• 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
• Feature or bugfix must have tests
• Git history must be clean
• If PR adds a configuration option, it must be added to the configuration file.

[codecov]

Codecov Report

Merging #3446 into master will decrease coverage by 2.56%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #3446      +/-   ##
=========================================
- Coverage   86.46%   83.9%   -2.57%     
=========================================
  Files         735      62     -673     
  Lines       37955    4702   -33253     
  Branches     4461     574    -3887     
=========================================
- Hits        32819    3945   -28874     
+ Misses       3936     636    -3300     
+ Partials     1200     121    -1079

[ofek]

@casidiablo Good catch, thanks!

[casidiablo]

When do you expect this would be released as a docker image? Which version will this be in?

[ofek]

@casidiablo 6.12, unless you feel comfortable using datadog/agent-dev:master

https://hub.docker.com/r/datadog/agent-dev/

[ahmed-mez]

@casidiablo thank you for contributing, but metrics_mapper is not a parameter and is not meant to be set by users.
The proper way to change a metric name is by setting up the optional <NEW_METRIC_NAME> field in the metrics param.

In your case it should be something like:

    metrics:
      - cp_kafka_server_replicamanager_underreplicatedpartitions: server.replica_manager.under_replicated_partitions

You can still find all the params that you can use to configure the check here in this file https://github.com/DataDog/integrations-core/blob/master/prometheus/datadog_checks/prometheus/data/conf.yaml.example

[casidiablo]

How does that look in the kubernetes integration?

[ahmed-mez]

Hi @casidiablo
could you provide us with more details regarding the changes you want to make in the kubernetes check?
Thanks!

[casidiablo]

It's explained in the motivation block above. Basically, I have this configuration in my pods:

    metrics:
      - cp_kafka_server_replicamanager_underreplicatedpartitions
    type_overrides:
      cp_kafka_server_replicamanager_underreplicatedpartitions: gauge
    metrics_mapper:
      cp_kafka_server_replicamanager_underreplicatedpartitions: server.replica_manager.under_replicated_partitions

What I expect from this is to get a metric named server.replica_manager.under_replicated_partitions show up in datadog. Instead, I get cp_kafka_server_replicamanager_underreplicatedpartitions.

In other words, metrics_mapper is not respected so I end up with metrics that look horrible and are sometimes incomprehensible.

[ahmed-mez]

As I told you metrics_mapper is an internal attribute and is not meant to be set by users. If you want to change the metric name you can do the following:

metrics:
      - cp_kafka_server_replicamanager_underreplicatedpartitions: server.replica_manager.under_replicated_partitions

Tell me if this worked for you. Thanks!

[casidiablo]

If I do that then the prometheus check just fails to start:

        could not invoke python check constructor: Traceback (most recent call last):
  File "/opt/datadog-agent/embedded/lib/python2.7/site-packages/datadog_checks/base/checks/prometheus/base_check.py", line 95, in __init__
    self.get_scraper(instance)
  File "/opt/datadog-agent/embedded/lib/python2.7/site-packages/datadog_checks/base/checks/prometheus/base_check.py", line 143, in get_scraper
    metrics = default_instance.get("metrics", []) + instance.get("metrics", [])
TypeError: can only concatenate list (not "dict") to list

I think it's because it does not expect metrics to be a dictionary but a list.