chore: include rule id in pyroscope_metrics_exporter_series_sent_total #4454
Conversation
| seriesByRuleID[ruleID]++ | ||
| } | ||
| for ruleID, count := range seriesByRuleID { | ||
| e.metrics.seriesSent.WithLabelValues(tenantId, ruleID).Add(float64(count)) |
There was a problem hiding this comment.
Can you estimate the extra cardinality that this will add?
There was a problem hiding this comment.
Cardinality could be described as "total number of tenants using recording rules times the number of instances of compaction workers in that cell". After the change, we would describe as "total number of rules defined by all the tenants times the number of instances of compaction workers in that cell".
So it's safe to talk about a x10-x50 increase in cardinality. In the case of ops we would go from 32 to 320 series
There was a problem hiding this comment.
While it is okay with current usage, it easy to see how this could go through the roof in the future, with this multiplier.
My gut feel is this is better kept in logs. (I am also not super firm with my position here, decide yourself).
| seriesByRuleID[ruleID]++ | ||
| } | ||
| for ruleID, count := range seriesByRuleID { | ||
| e.metrics.seriesSent.WithLabelValues(tenantId, ruleID).Add(float64(count)) |
There was a problem hiding this comment.
While it is okay with current usage, it easy to see how this could go through the roof in the future, with this multiplier.
My gut feel is this is better kept in logs. (I am also not super firm with my position here, decide yourself).
|
I've decided to merge as is. As I expect most of the tenants to have <5 rules. Happy to get rid of this if recording rules gets a lot of adoption |
The idea is to easily find rules that contribute the most to an eventual alert on tenants hitting cardinality limits.
We can make this more efficient in a previous layer of the code, but I avoided changing interfaces too much, while this code runs in a separate thread I think we can deal with it