-
Notifications
You must be signed in to change notification settings - Fork 216
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
feat: add metricscardinality to heartbeat #1235
feat: add metricscardinality to heartbeat #1235
Conversation
4efa73e
to
b0beff5
Compare
b0beff5
to
7af0632
Compare
I think a simpler solution may just be to enumerate over the metrics and take the length of the metrics slice https://gist.github.com/matmerr/f2e31d9be0b5af0eec750fa944edc617#file-counter_test-go-L58 // count all metrics and combination of labels currently exposed on
// prometheus endpoint
metrics, err := registry.Gather()
if err != nil {
fmt.Println("Error gathering metrics: ", err)
}
// count all metrics exposed for each metric
var metriccounter int
for _, metric := range metrics {
metriccounter += len(metric.Metric)
} where the metrics endpoint looks like this: |
@matmerr if I only loop through the metrics and add the length of each, this will cause a difference between the count seen through scraping and by this code. Metrics of type histogram and summary expose more time series for quantile/bucket, count and sum. Is it ok to exist this difference in the counting between what's seen in the |
@alexcastilio How much is the difference? Is it 2x or a constant number? |
@anubhabMajumdar the difference is:
|
The issue like @alexcastilio showed me is that histogram's create more metrics that can't be summed by just length, and our apiserver metrics latency uses histogram type. retina/pkg/exporter/prometheusexporter.go Line 79 in 23c34b8
Confirmed with him offline that this diff outputs the correct results. Sample endpoint, including metric of histogram type: |
9ce3044
to
5372dac
Compare
Signed-off-by: Alex Castilio dos Santos <[email protected]>
5372dac
to
68b258d
Compare
# Description Add `metricscardinality` to heartbeat. This will give visibility on the number of time series being exposed by retina. ## Related Issue microsoft#1040 ## Checklist - [x] I have read the [contributing documentation](https://retina.sh/docs/contributing). - [x] I signed and signed-off the commits (`git commit -S -s ...`). See [this documentation](https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification) on signing commits. - [x] I have correctly attributed the author(s) of the code. - [x] I have tested the changes locally. - [x] I have followed the project's style guidelines. - [x] I have updated the documentation, if necessary. - [x] I have added tests, if applicable. ## Screenshots (if applicable) or Testing Completed Metrics exported with heartbeat: ![image](https://github.com/user-attachments/assets/6fb4d76b-5780-4751-91c0-46e3c4f0fb85) ## Additional Notes Metrics of types `histogram` and `summary` expose multiple time series during a scrape. Code is counting according to number of time series exposed at /metrics endpoint. Ref: https://prometheus.io/docs/concepts/metric_types/#histogram https://prometheus.io/docs/concepts/metric_types/#summary --- Please refer to the [CONTRIBUTING.md](../CONTRIBUTING.md) file for more information on how to contribute to this project. Signed-off-by: Alex Castilio dos Santos <[email protected]>
maps.Copy(props, cpuProps) | ||
maps.Copy(props, t.profile.GetMemoryUsage()) | ||
t.TrackEvent("heartbeat", props) | ||
} | ||
func metricsCardinality() (int, error) { | ||
metricFamilies, err := exporter.CombinedGatherer.Gather() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CombinedGatherer
can be nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anubhabMajumdar addressed in #1293
case io_prometheus_client.MetricType_HISTOGRAM: | ||
metrics := mf.GetMetric() | ||
for _, m := range metrics { | ||
metricscardinality += len(m.GetHistogram().GetBucket()) + 3 // +3 for le="+Inf", _sum and _count |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetHistogram
can return nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anubhabMajumdar addressed in #1293
Description
Add
metricscardinality
to heartbeat. This will give visibility on the number of time series being exposed by retina.Related Issue
#1040
Checklist
git commit -S -s ...
). See this documentation on signing commits.Screenshots (if applicable) or Testing Completed
Metrics exported with heartbeat:
![image](https://private-user-images.githubusercontent.com/6101006/404321935-6fb4d76b-5780-4751-91c0-46e3c4f0fb85.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkxMDkxODYsIm5iZiI6MTczOTEwODg4NiwicGF0aCI6Ii82MTAxMDA2LzQwNDMyMTkzNS02ZmI0ZDc2Yi01NzgwLTQ3NTEtOTFjMC00NmUzYzRmMGZiODUucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIwOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMDlUMTM0ODA2WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9NzU0YTQyOGM5ZjgwYjBjY2IxNTMyYmUwZTc2MGFjZDhmZTk2ZTBmYjBhMGVkMWU3M2FiYWY5MmQ4OTliZDg4MSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.5Iw7uwAHNjwE4GM5-JZvuh8RLbg6u2TIxn2ylwIfI7U)
Additional Notes
Metrics of types
histogram
andsummary
expose multiple time series during a scrape. Code is counting according to number of time series exposed at /metrics endpoint.Ref:
https://prometheus.io/docs/concepts/metric_types/#histogram
https://prometheus.io/docs/concepts/metric_types/#summary
Please refer to the CONTRIBUTING.md file for more information on how to contribute to this project.