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

enhanced monitor, process commands #261

Conversation

haristku
Copy link
Contributor

@haristku haristku commented Oct 9, 2024

Description:

breaking down the previous PR to specific part

  • enhanced monitor, process commands

Copy link
Contributor

@snake14 snake14 left a comment

Choose a reason for hiding this comment

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

The Process changes look good and work as expected 👍

When I tried testing the Monitor changes, the paging didn't appear to work. It would insert new lines above the window height and the visible portion of the window didn't change. It appeared to be appending the help text after some of the lines as well.

@haristku
Copy link
Contributor Author

@snake14
what OS are you using to run the Monitor command? do you use *nix terminal or windows PowerShell or CMD?

unfortunately, i'm not running matomo in a Windows environment, so i don't have a chance to see the "Monitor" output in a Windows console like PowerShell or CMD..

🙏 ✌️

@snake14
Copy link
Contributor

snake14 commented Oct 14, 2024

@snake14 what OS are you using to run the Monitor command? do you use *nix terminal or windows PowerShell or CMD?

unfortunately, i'm not running matomo in a Windows environment, so i don't have a chance to see the "Monitor" output in a Windows console like PowerShell or CMD..

🙏 ✌️

@haristku I'm running Ubuntu 22.04 and using the stock terminal.

@haristku
Copy link
Contributor Author

haristku commented Oct 15, 2024

The Process changes look good and work as expected 👍

When I tried testing the Monitor changes, the paging didn't appear to work. It would insert new lines above the window height and the visible portion of the window didn't change. It appeared to be appending the help text after some of the lines as well.

what if you add 3 rows per page args:

php console queuedtracking:monitor -p 3

what's the result? is the paging working or still not?


the default rows per page is 16, if we have 16 workers it means we only have 1 page, so the paging feature won't work, but if we set the workers to 34, it means we have 3 pages and the paging feature will work, and we can use the right, left, up, down arrows to navigate through the page. the total pages is displayed at the bottom Q [0-15] | page 1/3 | press (0-9.,q) or arrow(L,R,U,D) | diff/sec 0

and the help description also included in:

php console help queuedtracking:monitor

Description:
  Shows and updates the current state of the queue every 2 seconds.
  Key ,=first page, .=last page, 0-9=move to page section, arrow LEFT=prev page, RIGHT=next page, UP=next 10 pages, DOWN=prev 10 pages, q=quit

-p, --perpage=PERPAGE                Number of queue worker displayed per page. [default: 16]

we already split the PR into smaller parts, in this PR we cannot have workers higher than 16, therefore we can use -p parameter to simulate pages


i still want to update this PR to remove the use of PCNTL completely... i'll update it tomorrow...

ty.

Copy link
Contributor

@snake14 snake14 left a comment

Choose a reason for hiding this comment

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

Thank you for the suggestion of changing the page size. That helped me test some variations. Although, I tested without setting a specific page size again and it worked correctly. I'm not sure what the issue was last time I tested. I'm approving the PR 👍

Copy link
Contributor

@AltamashShaikh AltamashShaikh left a comment

Choose a reason for hiding this comment

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

Looks okay to me, left 1 comment.

haristku and others added 3 commits October 16, 2024 14:30
Co-authored-by: Altamash Shaikh <[email protected]>
…, coz cursor hide and show mechanism cannot be restored without pcntl sigterm
@haristku
Copy link
Contributor Author

Looks okay to me, left 1 comment.

pcntl usage removed. the consequence is blinking cursor on monitor command… coz cursor hide and show mechanism cannot be restored without pcntl sigterm.

@AltamashShaikh @snake14 , have you ever tried the monitor command in a Windows environment?
i hope the output and functionality can be the same in all environments.

@snake14
Copy link
Contributor

snake14 commented Oct 17, 2024

Looks okay to me, left 1 comment.

pcntl usage removed. the consequence is blinking cursor on monitor command… coz cursor hide and show mechanism cannot be restored without pcntl sigterm.

@AltamashShaikh @snake14 , have you ever tried the monitor command in a Windows environment? i hope the output and functionality can be the same in all environments.

@haristku Thank you for all of your effort. I think that the PR looks ready to merge. Sadly, I don't think we currently have a Windows environment to test with. @AltamashShaikh Do you think that we're alright to merge anyway?

@AltamashShaikh
Copy link
Contributor

@snake14 We can merge this and address the concern for windows if any

@AltamashShaikh AltamashShaikh merged commit 08647ea into matomo-org:5.x-dev Oct 18, 2024
5 checks passed
@snake14 snake14 deleted the feature/enhance_monitor_and_processor branch October 18, 2024 02:53
@snake14 snake14 restored the feature/enhance_monitor_and_processor branch October 18, 2024 02:53
@haristku
Copy link
Contributor Author

@haristku Thank you for all of your effort. I think that the PR looks ready to merge. Sadly, I don't think we currently have a Windows environment to test with. @AltamashShaikh Do you think that we're alright to merge anyway?

thanks for creating matomo and this plugin in the first place. 👍

btw, i forgot to remove these 2 lines, my intention is to clear the screen, but it seems unnecessary, you can remove them.✌️

@AltamashShaikh
Copy link
Contributor

@haristku Did you mean this 2 lines ?

@haristku
Copy link
Contributor Author

@haristku Did you mean this 2 lines ?

yes...

@haristku
Copy link
Contributor Author

haristku commented Oct 21, 2024

and 1 more update, change this line to this:

$diffSumInQueue = $diffSumInQueue < 0 ? "<fg=red;options=bold>" . number_format(abs($diffRps)) . "</>" : "<fg=green;options=bold>" . number_format($diffRps) . "</>";

✌️

@AltamashShaikh
Copy link
Contributor

@haristku All done and released v5.1.0 of the plugin.

@haristku
Copy link
Contributor Author

ty sir... 🫡

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants