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

Add metrics/alloc_bytes for showing memory allocated #5715

Merged

Conversation

anderseknert
Copy link
Member

Add a simple endpoint (/metrics/alloc) to quickly show memory utilization of OPA. This can be done via e.g. Prometheus already, but having a human friendlier interface for this is nice.

$ opa run -s https://bundles.example.com/my-big-bundle.tar.gz
$ curl 'localhost:8181/metrics/alloc?pretty=true'
3.3GB

Without the pretty query string, the raw number of bytes is shown. I'm considering whether the pretty format should be the default, and something like format=raw, or pretty=false should be the option to show bytes, as that's likely less useful to a human operator. Ideas on that would be appreciated.

NOTE: There's a slight discrepancy between what Go reports and what top and other programs do (Go reports less memory used), but I think this is good enough for providing a ballpark metric.

srenatus
srenatus previously approved these changes Mar 1, 2023
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Code looks good. My only concern would be that the same thing could be achieved by reading the existing metrics and doing the addition and pretty printing using some favourite scripting language. But it surely adds some convenience over that.

@srenatus
Copy link
Contributor

srenatus commented Mar 1, 2023

NOTE: There's a slight discrepancy between what Go reports and what top and other programs do (Go reports less memory used), but I think this is good enough for providing a ballpark metric.

I haven't read the detailed descriptions of the metrics added together here, but if anything, the runtime metrics should be more accurate over top etc, shouldn't they be?

@anderseknert
Copy link
Member Author

You're absolutely right: this is only for convenience. And I'm all about convenience :)

As for the accuracy, that might very well be. Reported metrics by Go multiplied by ~ 1.125 seems to give almost exactly what top and Activity Monitor reports, so I guess it's either some overhead of the application Go does not account for, or it is the OS allocating some overhead (?) which is not seen from inside of the app. I have frankly no idea 😅

What do you think regarding the default formatting?

@srenatus
Copy link
Contributor

srenatus commented Mar 1, 2023

I think alloc_bytes would be a better name; or total_alloc_bytes.

Re Default formatting, I'd keep it as it is right now. But I'm not having a strong opinion here. If we change the default, the name should perhaps include _pretty? 🤔

Another thing, do we need to do something to ensure that the route won't clash with existing ones, or future ones, of the prom handler?

@anderseknert
Copy link
Member Author

alloc_bytes is fine with me, although it sorta "conflicts" with the pretty=true option, or what do you think?

I suppose we could have one /metrics/alloc_bytes and another /metrics/alloc_pretty? 🤔

I have checked to make sure /metrics still works as before. The chance of Prometheus also adding an /alloc endpoint to scrape from should be slim, but if we're worried about it, we could use some other endpoint altogether. It does feel rather "metrics" oriented though.

@srenatus
Copy link
Contributor

srenatus commented Mar 1, 2023

👍

@anderseknert
Copy link
Member Author

@srenatus I'm sorry, was the thumb up about

I suppose we could have one /metrics/alloc_bytes and another /metrics/alloc_pretty?

or that /metrics still works as expectd? 🙂

@srenatus
Copy link
Contributor

srenatus commented Mar 1, 2023

The second. LGTM.

var m runtime.MemStats
runtime.ReadMemStats(&m)

total := m.HeapInuse + m.StackInuse + m.MCacheInuse + m.MSpanInuse
Copy link
Member

Choose a reason for hiding this comment

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

Can you help me understand why we these metrics and not something like HeapAlloc?

Copy link
Member Author

Choose a reason for hiding this comment

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

Alloc/HeapAlloc will show a lower number, moving the metrics reported here further away from what other tools report.

@ashutosh-narkar
Copy link
Member

I suppose we could have one /metrics/alloc_bytes and another /metrics/alloc_pretty?

I agree something like /metrics/alloc_bytes or metrics/mem_stats or /metrics/alloc_mem_bytes would be better.

@anderseknert
Copy link
Member Author

Alright, I'll update to use alloc_bytes tomorrow. Thanks to you both!

@srenatus
Copy link
Contributor

srenatus commented Mar 2, 2023

I think there's another metric that could be useful for triangulating the memstats we're adding here, and what htop etc show: process_resident_memory_bytes. It's gathered by the prometheus process collector, and I think enabled by default. I'd expect that one to be closer to what htop presents (it's also called RSS, like htop)

@anderseknert
Copy link
Member Author

Thanks @srenatus. I can definitely add that too. Do you know which of the memstat metrics that corresponds to?

@srenatus
Copy link
Contributor

srenatus commented Mar 2, 2023

Do you know which of the memstat metrics that corresponds to?

I don't think it corresponds to any: it's using /proc to find data about the process' memory usage as seen from the kernel. But I just realised it's linux-only, so that might not be helpful here.

@netlify
Copy link

netlify bot commented Mar 2, 2023

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit 143a326
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/640088719de55f0009586f4c
😎 Deploy Preview https://deploy-preview-5715--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

srenatus
srenatus previously approved these changes Mar 2, 2023
internal/prometheus/prometheus.go Outdated Show resolved Hide resolved
@anderseknert anderseknert changed the title Add metrics/alloc for showing memory allocated Add metrics/alloc_bytes for showing memory allocated Mar 2, 2023
Signed-off-by: Anders Eknert <anders@styra.com>
@anderseknert anderseknert merged commit 74ac69c into open-policy-agent:main Mar 2, 2023
@anderseknert anderseknert deleted the metrics-mem-endopoint branch March 2, 2023 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants