Skip to content

expose per process metrics in fpm status #9646

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

Closed
wants to merge 1 commit into from

Conversation

cdaguerre
Copy link

@Girgias Girgias requested a review from bukka September 30, 2022 11:34
Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

You should add a test for this as full is not currently used. It might need some tweaking in the tester to make it nicely work by the look of it.

@bukka
Copy link
Member

bukka commented Sep 30, 2022

I think we might pre-format those labels and then have a single string to add there multiple times.

@cdaguerre cdaguerre force-pushed the openmetrics-status branch 10 times, most recently from 2cc3206 to 7ff421e Compare October 5, 2022 11:00
@cdaguerre
Copy link
Author

@bukka I took a different approach...
According to the openmetrics specification, metric values can not be "interleaved", meaning that valid format can not be achieved by iterating over processes and then metrics. You have to iterate over each process for each metric...
While this works it's probably pretty ugly C because it's the first time I get anywhere near C ;)
Some guidance on how I could clean this up would be much appreciated.

@bukka
Copy link
Member

bukka commented Oct 30, 2022

Agreed it's getting quite ugly. Honestly it was already quite bad but with more logic like this, it's becoming even worse. So I think it might require some deeper refactoring so we don't duplicate things and it's more manageable. I just prioritised status related issues on my TODO list and try to fix some bugs first so merging is less painful later (we are merging up from lower branches so if it changes, we have to deal with conflicts). I will think about better structure and then let you know when I have got something.

I think it's fine if you just concentrate on the functionality (fine to keep it ugly for now) and adding some test. Then we could take it from there.

@cdaguerre cdaguerre force-pushed the openmetrics-status branch 4 times, most recently from f208786 to b7690bd Compare November 23, 2023 14:10
@cdaguerre
Copy link
Author

Closed in favor of #9646

@cdaguerre cdaguerre closed this Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants