Skip to content
This repository has been archived by the owner on Mar 28, 2020. It is now read-only.

Refacto puma-helper UI #21

Closed
wants to merge 16 commits into from
Closed

Refacto puma-helper UI #21

wants to merge 16 commits into from

Conversation

w3st3ry
Copy link
Contributor

@w3st3ry w3st3ry commented May 28, 2019

Related to #18

  • Suppress the process index and start with the PID directly to simplify the line, start with "└" for the process to make it look like a child of the application line
  • All numbers be padded on a fixed size to preserve alignment
  • Process phase only displayed if it's different from application phase to save space, at the end of the line, in orange
  • Uptime /w 1 space between number and letter, only pad the whole string to maintain alignment -> need this MR validated before do these changes https://github.com/dimelo/puma-helper/pull/20/files
  • Last checkin disabled if there's an issue (> 10 sec)
  • Delete headers lines
  • Remove the number of workers per apps
  • No digit in memory
  • Throw errors at the end of the output
  • MiB to M
  • Active Threads / Threads to Load
  • Sort workers by uptime
  • Add queuing in worker line
  • Better current CPU usage based on PID

Screenshot 2019-06-07 at 14 15 40

Valentin Pichard added 10 commits May 28, 2019 11:24
…y the line, start with └

Signed-off-by: Valentin Pichard <valentin.pichard@dimelo.com>
Signed-off-by: Valentin Pichard <valentin.pichard@dimelo.com>
Signed-off-by: Valentin Pichard <valentin.pichard@dimelo.com>
Signed-off-by: Valentin Pichard <valentin.pichard@dimelo.com>
Signed-off-by: Valentin Pichard <valentin.pichard@dimelo.com>
Signed-off-by: Valentin Pichard <valentin.pichard@dimelo.com>
Signed-off-by: Valentin Pichard <valentin.pichard@dimelo.com>
…m app

Signed-off-by: Valentin Pichard <valentin.pichard@dimelo.com>
Signed-off-by: Valentin Pichard <valentin.pichard@dimelo.com>
Signed-off-by: Valentin Pichard <valentin.pichard@dimelo.com>
@w3st3ry w3st3ry self-assigned this May 28, 2019
@w3st3ry w3st3ry added the enhancement New feature or request label May 28, 2019
@w3st3ry w3st3ry added this to In progress in puma-helper board May 28, 2019
@w3st3ry w3st3ry moved this from In progress to Waiting review in puma-helper board May 28, 2019
@spuyet
Copy link

spuyet commented May 29, 2019

Thanks @w3st3ry that's a good start, here is my feedback:

  • Drop the arrows at line beginning
  • Drop the phase and app path
  • Same app with different path should not be aggregated but considered as different apps
  • There is no Times value asked in our requirements, drop it

Please try to have an output like the one we've asked.

Copy link

@spuyet spuyet left a comment

Choose a reason for hiding this comment

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

I will start to review the code once the output will match our expectations.

@ylecuyer
Copy link
Owner

ylecuyer commented May 29, 2019

I would add :

  • % in CPU Av and M in Mem should either be removed or included into the coloring (personally I would remove them)
  • CPU Av => Just 'CPU'

Signed-off-by: Valentin Pichard <valentin.pichard@dimelo.com>
@w3st3ry
Copy link
Contributor Author

w3st3ry commented May 31, 2019

@spuyet @ylecuyer I done the changes in the last commit, I'll update the screenshot.

Except "same app with different path should not be aggregated but considered as different apps", I asked myself the same thing as #14 (comment)

@w3st3ry w3st3ry requested a review from spuyet May 31, 2019 12:32
Signed-off-by: Valentin Pichard <valentin.pichard@dimelo.com>
@ylecuyer
Copy link
Owner

Why did CPU and Mem disapeared ?

Signed-off-by: Valentin Pichard <valentin.pichard@dimelo.com>
@w3st3ry
Copy link
Contributor Author

w3st3ry commented May 31, 2019

@ylecuyer fixed. understood the opposite.
@spuyet WDYT about hide Queue field if it's equal to 0?

@jarthod
Copy link

jarthod commented May 31, 2019

Yes please hide the queue when it's 0, like the other warnings (phase, last update, etc..)

@ylecuyer
Copy link
Owner

Can you also rename "CPU Av" to just "CPU"

Signed-off-by: Valentin Pichard <valentin.pichard@dimelo.com>
@spuyet
Copy link

spuyet commented Jun 3, 2019

Can you add some new screenshots once you've modified the output ?

@ylecuyer
Copy link
Owner

ylecuyer commented Jun 3, 2019

Screenshot_2019-06-03_15-58-17
:/

@w3st3ry
Copy link
Contributor Author

w3st3ry commented Jun 6, 2019

@spuyet done.
@ylecuyer fixed, normally it will works on any terminal now (not just iTerm :D)

Signed-off-by: Valentin Pichard <valentin.pichard@dimelo.com>
@ylecuyer
Copy link
Owner

ylecuyer commented Jun 6, 2019

I'm not sure which version is on staging for the moment but the Mem output is not good:

Screenshot_2019-06-06_17-42-34

It should be 237 not 237002752

@w3st3ry
Copy link
Contributor Author

w3st3ry commented Jun 6, 2019

@ylecuyer I'm actually doing some tests (I can't test on my own env to have a perfect output), that's why. Don't worry, I'm aware of this.

Signed-off-by: Valentin Pichard <valentin.pichard@dimelo.com>
@w3st3ry
Copy link
Contributor Author

w3st3ry commented Jun 6, 2019

@spuyet @jarthod I used a new way to calculate the current CPU usage of a pid, based on a tiny lib who implement ps and an history. Tested locally, it works fine. Please give me your feedback about it.
I set the minimum timer to get an approx. average.

@jarthod
Copy link

jarthod commented Jun 9, 2019

Ok nice, can we test this somewhere?

@w3st3ry
Copy link
Contributor Author

w3st3ry commented Jun 10, 2019

@jarthod on staging server.

@ylecuyer
Copy link
Owner

They are still issues with the display:

Screenshot_2019-06-11_13-57-12

I'm not sure about the new CPU stats method. Now only available values are 0, 50, 100 or 200

@spuyet
Copy link

spuyet commented Jun 11, 2019

I checked on staging and the apps are still aggregated by name and should not:

answers_integration application
38423 (/home/answers_integration/current/tmp/pids/puma.state) | Load: 0[░░]2
 └ 12601 CPU: 0.0 Mem: 257 Uptime: 197h28m57s Load: 0[░]1
 └ 12789 CPU: 0.0 Mem: 254 Uptime: 197h28m51s Load: 0[░]1

38424 (/home/answers_integration/current/tmp/pids/puma_api.state) | Load: 0[░░]2
 └ 12560 CPU: 0.0 Mem: 184 Uptime: 197h28m57s Load: 0[░]1
 └ 12775 CPU: 0.0 Mem: 183 Uptime: 197h28m52s Load: 0[░]1

should be:


38423 answers_integration                      Load: 0[░░]2
 └ 12601 CPU: 0.0 Mem: 257M Uptime: 197h28m57s Load: 0[░]1
 └ 12789 CPU: 0.0 Mem: 254M Uptime: 197h28m51s Load: 0[░]1

38424 answers_integration_api                  Load: 0[░░]2
 └ 12560 CPU: 0.0 Mem: 184M Uptime: 197h28m57s Load: 0[░]1
 └ 12775 CPU: 0.0 Mem: 183M Uptime: 197h28m52s Load: 0[░]1

Please remove the pipe before the load and add some padding to keep it aligned. Also add back the memory unit please, we want to know if we're using kilobyte, megabyte, gigabyte, etc

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
puma-helper board
  
Waiting review
Development

Successfully merging this pull request may close these issues.

None yet

4 participants