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

feat(citizen-devtools): added GET_RESOURCE_MONITOR_DATA #3221

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nova-px
Copy link
Contributor

@Nova-px Nova-px commented Mar 8, 2025

Goal of this PR

Providing the ability to get data from the resource monitor to visualize it in grafana or something else

How is this PR achieving the goal

Added the GET_RESOURCE_MONITOR_DATA native

This PR applies to the following area(s)

Server, Natives

Successfully tested on

Platforms: Windows

Checklist

  • Code compiles and has been tested successfully.
  • Code explains itself well and/or is documented.
  • My commit message explains what the changes do and what they are for.
  • No extra compilation warnings are added by these changes.

Fixes issues

Sorry, something went wrong.

@github-actions github-actions bot added the invalid Requires changes before it's considered valid and can be (re)triaged label Mar 8, 2025
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please add proper structure to the documentation?

@prikolium-cfx
Copy link
Collaborator

Thank you for this contribution. Could you also add client side version as well?

@AvarianKnight
Copy link
Contributor

I'm generally curious on why expose like this vs adding more Prometheus endpoints

@Nova-px
Copy link
Contributor Author

Nova-px commented Mar 21, 2025

I'm generally curious on why expose like this vs adding more Prometheus endpoints

Also thought about that, but I don't have much knowledge of the codebase and how it should be implemented

@prikolium-cfx
Copy link
Collaborator

I'm generally curious on why expose like this vs adding more Prometheus endpoints

But in this case how should we provide client side data?

@Nova-px
Copy link
Contributor Author

Nova-px commented Mar 21, 2025

I'm generally curious on why expose like this vs adding more Prometheus endpoints

But in this case how should we provide client side data?

Maybe add the native only on client-side and add another Prometheus endpoint on the server-side, I think that would also reduce the performance cost of the implementation to redirect the data from the native to Grafana / Prometheus / etc.

@AvarianKnight
Copy link
Contributor

AvarianKnight commented Mar 21, 2025

I don't think that the client side should be included in this PR at all, there should be some proper way for the client to send the server performance metrics if that is wanted, otherwise we'll end up with a weird "everyone makes there own, and there's no nice/standardized way to do it" and it will be a big mess.

IMO, The server should add more endpoints (i.e. /perf/resources) and expand any performance/statistics logging from there, client stuff can be figured out later with some nice standardized way

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid Requires changes before it's considered valid and can be (re)triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants