Skip to content

Conversation

@xvello
Copy link
Contributor

@xvello xvello commented Apr 2, 2019

What does this PR do?

Like summaries and histogram types have their _sum / _bucket / _count timeseries aggregated in a single metric using the trimmed name, versions 0.4.0+ of the python prometheus/openmetrics client trim the _total suffix out of counter metric names.

This is a breaking change for us, as integration and custom checks should change their configuration to refer to counter metrics with the new shorter metric name.

This PR introduces a compatibility layer to make this transition transparent. When looking up a metric name in one of the scraper_config's map/list, the following happens:

  • the lookup is attempted with the current (possibly trimmed) name
  • if the lookup fails and the metric is a counter, a second lookup is attempted, adding the _total suffix to the metric name.

If we go this route, the prometheus mixin should also be updated, as it uses the same client lib.

Alternative route

We could patch create_scraper_configuration to scan config['label_joins'], config['metrics_mapper'], config['ignore_metrics'] and metrics_mapper to duplicate all entries containing a _total suffix to a new entry without it.

This would avoid the double-lookup cost ; but as we do not know if a given metric is a counter at this time, we might create false-positives by matching non-counters with a _total suffix (example: the kubelet's container_network_tcp_usage_total is a gauge)

Other changes:

  • kubelet/tests/test_kubelet.py : update a test that is accessing internals of the Sample object

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

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
Copy link

codecov bot commented Apr 30, 2019

Codecov Report

Merging #3443 into master will decrease coverage by 0.19%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #3443     +/-   ##
========================================
- Coverage    86.3%   86.1%   -0.2%     
========================================
  Files         717     689     -28     
  Lines       36637   36281    -356     
  Branches     4301    4442    +141     
========================================
- Hits        31618   31239    -379     
+ Misses       3830    3781     -49     
- Partials     1189    1261     +72

@mfpierre
Copy link
Contributor

mfpierre commented May 3, 2019

superseded by #3700

@mfpierre mfpierre closed this May 3, 2019
@ofek ofek deleted the xvello/openmetrics-060 branch May 3, 2019 13:42
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