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 Prometheus histograms when keylabels and values sit at the same path #11759

Merged
merged 12 commits into from
Apr 12, 2019

Conversation

odacremolbap
Copy link
Contributor

@odacremolbap odacremolbap commented Apr 11, 2019

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

metricbeat/helper/prometheus/prometheus.go Show resolved Hide resolved
@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
Copy link
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 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
…ath (elastic#11759)

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

(cherry picked from commit eb89eaa)
@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
Member

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
Contributor Author

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
Member

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
Contributor

exekias commented Apr 16, 2019

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
Labels
Metricbeat Metricbeat review Team:Integrations Label for the Integrations team v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants