Open henrikno opened 2 weeks ago
Thanks @henrikno for creating this. Let me pull @lalit-satapathy in as his team is the one owning the prometheus integration.
Thanks @henrikno for creating this. Let me pull @lalit-satapathy in as his team is the one owning the prometheus integration.
@shmsr can you take a look?
I'll take a look.
Hi @henrikno 👋🏽
Apologies for taking some time to reply as I was busy investigating a couple of other bugs.
I do understand it is not working as expected for you but this is how it is.
So when use_types: true
, this part of the code is used to form the event: https://github.com/elastic/beats/blob/367d94e1dc40a82713d651735eac7921bb584474/x-pack/metricbeat/module/prometheus/collector/data.go#L147 and as you can see *_sum
and *_count
are intentionally left out. I am not sure why this decision was taken ~5years ago to omit it out.
Also, see this logic here how Prometheus Histogram is converted to an ES histogram: https://github.com/elastic/beats/blob/367d94e1dc40a82713d651735eac7921bb584474/x-pack/metricbeat/module/prometheus/collector/histogram.go#L16
As have a value of 23 for all buckets and hence the rate is turning out to be 0; so it's because of the sample input that you provided. I'd recommend reading the full logic there and further discussion here: https://github.com/elastic/apm-agent-python/pull/1165#discussion_r651397014. So, this is the reason why counts gives you an array filled with 0s.
Also, if you use_types: false
, it seems you can get *_sum
and *_count
. Here: https://github.com/elastic/beats/blob/367d94e1dc40a82713d651735eac7921bb584474/metricbeat/module/prometheus/collector/data.go#L137
But yes you miss out the transformation from Prometheus Histogram to ES Histogram here as use_types
is false
. But I recommend trying this out and see if this suits you?
I need to check if it is safe to add *_sum
and *_count
when use_types: true
and why the decision of not adding them was taken back then.
Also, about the docs example that you mentioned: https://www.elastic.co/docs/current/integrations/prometheus#histograms-and-types-1
{
"prometheus": {
"labels": {
"instance": "172.27.0.2:9090",
"job": "prometheus"
},
"prometheus_target_interval_length_seconds_count": {
"counter": 1,
"rate": 0
},
"prometheus_target_interval_length_seconds_sum": {
"counter": 15.000401344,
"rate": 0
}
"prometheus_tsdb_compaction_chunk_range_seconds_bucket": {
"histogram": {
"values": [50, 300, 1000, 4000, 16000],
"counts": [10, 2, 34, 7]
}
}
},
}
If you notice prometheus_target_interval_length_seconds
and prometheus_tsdb_compaction_chunk_range_seconds
are two different metrics where the latter is a histogram and the former a counter (rate counter). For histogram here you can there's no *_sum
and *_count
when use_types
is true
.
I hope this explanation helps.
I get that it was an "intentional" decision to not add it, but I think that was a mistake as it makes it very difficult or impossible to query it. There's a TODO about adding it. https://github.com/elastic/beats/blob/367d94e1dc40a82713d651735eac7921bb584474/x-pack/metricbeat/module/prometheus/collector/data.go#L147-L148 There's an enhancement issue that considers adding it https://github.com/elastic/integrations/issues/5042 In the docs for histogram https://prometheus.io/docs/practices/histograms/
Note that the number of observations (showing up in Prometheus as a time series with a _count suffix) is inherently a counter (as described above, it only goes up). The sum of observations (showing up as a time series with a _sum suffix) behaves like a counter, too, as long as there are no negative observations.
So a histogram is also "counter", so it should behave similar to a counter, so we can query it with a rate, avg etc.
Thanks Henrik for updating.
As previously discussed on Slack (and sharing here for those who weren't part of that conversation): I reviewed the Slack thread asking for histogram changes and Andrew's proposal regarding histogram implementation enhancements filed more than a year ago. Since this is Andrew's proposal, it requires further discussion and it'd be great if he can also share his views on this. We need to evaluate this thoroughly while considering customers who are using the current histogram implementation. Making changes has implications, so this needs detailed discussion with the Prometheus package owners and members of ES/ Kibana.
The current code works as intended according to the original design assumptions. While I agree with Henrik that improvements are needed, we should discuss this in detail within our team.
This prometheus histogram metric:
Gets turned into this in ES:
We are using use_types and rate, 1. because it's the default in the elastic-agent integration, and 2. to be able to query them in Kibana. https://www.elastic.co/docs/current/integrations/prometheus#histograms-and-types-1 However, the values don't look like the example, and what we expect. The
_count
and_sum
is missing. I was hoping to query the rate of the count/sum.Sidenote, if the buckets are diffed, I expected it to be named .rate. I was confused why the values did not match what the prometheus endpoint waas reporting at all. This is what it does for counters. Also for counters it keeps the original, but I can see that that would increase the storage.
Another thing that looks funny is the empty values
{"values":[],"counts":[]}
. Looks like this is happening because we use TSDS, which is also the default in the prometheus integration.I tried to reproduce it with just metricbeat 8.15.3.
Put the example at the top in a file called
metrics
, then runpython3 -m http.server 9000
Set this in metricbeat.yml
With use_types: true, rate_counters: false, I get a bunch of zeroes:
With rate_counters: false I expected to get the exact same values that prometheus reports. Also missing _sum and _count. If I disable use_types, I do see the values, but there's one document per bucket which is extremely difficult to query.