Skip to content

Fix Prometheus histograms when keylabels and values sit at the same path#11759

Merged
odacremolbap merged 12 commits intoelastic:masterfrom
odacremolbap:task/prometheus-keylabeled
Apr 12, 2019
Merged

Fix Prometheus histograms when keylabels and values sit at the same path#11759
odacremolbap merged 12 commits intoelastic:masterfrom
odacremolbap:task/prometheus-keylabeled

Conversation

@odacremolbap
Copy link
Copy Markdown
Contributor

@odacremolbap odacremolbap commented Apr 11, 2019 •
edited
Loading

When a Prometheus histogram is configured to set KeyLabel at the same path at the event where the values will be written, labels will be smashed by values.

Those scenario patterns are:

metric_name{label1="value1",label2="value2"} metric_value

with mappings defined as

&MetricsMapping{
	Metrics: map[string]MetricMap{
		"metric_name": Metric("metrics.name"),
	},
				Labels: map[string]LabelMap{
					"rank":  KeyLabel("metric.name.label1"),
					"alive": KeyLabel("metric.name.label2"),
				},

which will lead to labels not included for histograms

@odacremolbap odacremolbap added in progress Pull request is currently in progress. Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Apr 11, 2019
@odacremolbap odacremolbap requested a review from exekias April 11, 2019 07:26
@odacremolbap odacremolbap requested a review from a team as a code owner April 11, 2019 07:26
@odacremolbap odacremolbap mentioned this pull request Apr 11, 2019
Merged
exekias
exekias suggested changes Apr 12, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Thank you for taking this @odacremolbap! please update the description to reflect the fix, also left a comment that should simplify the code

@exekias exekias added review and removed in progress Pull request is currently in progress. labels Apr 12, 2019
@odacremolbap odacremolbap changed the title Extend Prometheus for some KeyLabeled scenarios Fix Prometheus histograms when keylabels and values sit at the same path Apr 12, 2019
@odacremolbap odacremolbap requested a review from exekias April 12, 2019 10:15
exekias
exekias approved these changes Apr 12, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Well done on new tests and debugging this issue!

@odacremolbap odacremolbap mentioned this pull request Apr 12, 2019
Closed
@odacremolbap odacremolbap merged commit eb89eaa into elastic:master Apr 12, 2019
@odacremolbap odacremolbap added the needs_backport PR is waiting to be backported to other branches. label Apr 12, 2019
odacremolbap pushed a commit to odacremolbap/beats that referenced this pull request Apr 12, 2019
@odacremolbap
Fix Prometheus histograms when keylabels and values sit at the same p…
4882066 
…ath (elastic#11759)

- Fix Labels overwriting for Prometheus histograms + keylabels
- Add tests for keylabeled prometheus metrics

(cherry picked from commit eb89eaa)
@odacremolbap odacremolbap mentioned this pull request Apr 12, 2019
Closed
@odacremolbap odacremolbap removed the needs_backport PR is waiting to be backported to other branches. label Apr 12, 2019
@odacremolbap odacremolbap deleted the task/prometheus-keylabeled branch April 12, 2019 16:29
@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Apr 15, 2019

Thanks for fixing this. The part I'm surprised is that this did not have an affect on any of the generated files from the new data generator. Would be great to also add an example with such a case to one of the metricsets.

@odacremolbap
Copy link
Copy Markdown
Contributor Author

odacremolbap commented Apr 15, 2019

CoreDNS seems to be the first module affected by this.
Yes, it includes the case at generated golden files, not at data.json since it only selects one event and dismiss any of the others being generated. I was told that file was used for documentation purposes, so it makes sense to avoid many events from filling the docs.

@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Apr 15, 2019

Only the data.json will only get one event, but the generated golden files for the tests should still contain all the data, see https://github.com/elastic/beats/tree/master/metricbeat/module/coredns/stats/_meta/testdata So if CoreDNS is affected in some examples, tests should fail.

@exekias
Copy link
Copy Markdown
Contributor

exekias commented Apr 16, 2019 •
edited
Loading

afaik only the open PR is affected?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@exekias exekias exekias approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Metricbeat Metricbeat review Team:Integrations Label for the Integrations team v7.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@odacremolbap @ruflin @exekias