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 Process memory usage and fix process state update #1516

Merged
merged 6 commits into from
Nov 21, 2024

Conversation

noma4i
Copy link
Contributor

@noma4i noma4i commented Oct 13, 2024

Hi!

I have found that the code around process refresh was shadowed by the reload instruction. So basically state field was never refreshed in the database. For this case I have added additional line in the spec file.

Initially the idea was to add memory usage to the process state. Like that:
image

This change uncovered the state update issue.

So far these changes are in this PR:

  • Fix spec and code to actually perform state update in the DB
  • Add memory usage to the state
  • Add memory usage to the process list in the UI

@Earlopain
Copy link
Collaborator

Earlopain commented Oct 13, 2024

Hi there, thanks for the PR!

I don't love the idea of adding a dependency for this, especially since it is dashboard only and memory usage is usually better tracked through other tools.

That said, how about something like this:

def process_pss(pid)
  data = File.read("/proc/#{pid}/smaps_rollup")
  pss_line = data.lines.find { |line| line.start_with?("Pss:") }
  pss_line.split(" ")[1].to_i * 1024
end

If that file doesn't exist on macos (you seem to use it, good opportunity to find out), probably need to do some switching based on platform like sidekiq does here (and also to not break on windows): https://github.com/sidekiq/sidekiq/blob/d6354bc74204004fc8b446c96875a333c50d27eb/lib/sidekiq/launcher.rb#L227

It will be unix only but that seems perfectly fine.

@noma4i
Copy link
Contributor Author

noma4i commented Oct 13, 2024

Hi @Earlopain!
GoodJob depends pretty much on Ruby on Rails so will have all deps already(activesupport depends on bigdecimal and ruby-vips on ffi). Idea to use get_process_mem comes from some comments from @bensheldon on the issues here.

@bensheldon
Copy link
Owner

@noma4i thanks for putting together this PR!

I'm in agreement with @Earlopain about the reluctance to add dependencies (and unfortunately GoodJob's java CI build are broken at the moment so I'm not able to verify it right now). Also seeing it now, I have a thought of "gosh, I wish that was PSS rather than RSS" (which matters less for the $ good_job command, but does when running in Puma workers)

I wonder:

  • Can the functionality be vendored? similar to vendoring systemd integration
  • Can the functionally be added by the application (e.g. if get_process_mem gem is installed by the application, then this adds in that behavior)
  • Could it be easier for the application to more easily add to the process state it collects via an override/extension or configuration?

@noma4i
Copy link
Contributor Author

noma4i commented Oct 14, 2024

Updated. @bensheldon I have used the sidekiq approach to copy same multiplatform behaviour. Macos works, Linux code tested:
image

@noma4i
Copy link
Contributor Author

noma4i commented Oct 25, 2024

@bensheldon any other change you want to see in this PR?

@bensheldon
Copy link
Owner

@noma4i would you be able to fix the lint errors? (bin/lint)

@bensheldon
Copy link
Owner

I fixed it. I'll poke at this a tiny bit and then looks good 👍🏻

@bensheldon bensheldon changed the title Fix state update + Add Memory usage Add Process memory usage and fix process state update Nov 21, 2024
@bensheldon bensheldon merged commit 27c2a6d into bensheldon:main Nov 21, 2024
25 checks passed
@bensheldon bensheldon added enhancement New feature or request bug Something isn't working labels Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

3 participants