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.

Sign up for GitHub

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

Jump to bottom

[WIP] System Status and Performance Info UI #6062

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[WIP] System Status and Performance Info UI #6062

wants to merge 3 commits into from

Conversation

empty-codes
Copy link
Contributor

@empty-codes empty-codes commented Dec 13, 2024 •
edited
Loading

What this PR does

Documents progress in building a user-facing UI to present system status and performance information for the dashboard

Current Look:

2024-12-19.00-12-07.mp4

ragesoss
ragesoss reviewed Dec 16, 2024
View reviewed changes
app/services/get_system_metrics.rb Outdated
# using Stats::History
end

# def fetch_system_health_metrics
Copy link
Member

Choose a reason for hiding this comment

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

My personal preference is to avoid leaving this kind of thing in the code, if it's not going to be part of a minimal initial implementation that we can merge and deploy. It think a first version that includes only sidekiq stats will already be useful, so we should probably plan on rolling that out before implementing any additional not-yet-defined metrics.

@empty-codes
Creates SystemStatusController and SystemMetricsService for status in…
d19a4a0 
…fo UI + Defines functions to get stats and queue info from Sidekiq
@empty-codes
Creates first version of UI consisting of queue details and sidekiq s…
2978545 
…tats:

- Adds route for UI to config
- Controller fetches metrics from service class
- Service class renamed and fetches data and manipulates it as needed
- View created using haml template to display information
- Adds descriptions of queues / metrics in locales en.yml
@ragesoss
Copy link
Member

You don't need to merge from master unless there is a conflict. If you want to make sure it's based on up-to-date code, you can rebase instead. With the merge, it makes for a very noisy set of changes that includes many things unrelated to the PR.

@empty-codes
Copy link
Contributor Author

You don't need to merge from master unless there is a conflict. If you want to make sure it's based on up-to-date code, you can rebase instead. With the merge, it makes for a very noisy set of changes that includes many things unrelated to the PR.

Sorry about that, I've fixed it now

ragesoss
ragesoss reviewed Dec 18, 2024
View reviewed changes
app/views/system_status/index.html.haml Outdated
.container.queues
.module
.section-header
%h3 Queues Overview
Copy link
Member

Choose a reason for hiding this comment

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

Use i18n for all the text.

Copy link
Contributor Author

@empty-codes empty-codes Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Got it, Any other changes?

Copy link
Member

Choose a reason for hiding this comment

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

Not at the moment! Is this a complete MVP? If so, mark it ready for review and I can probably review it tomorrow. Also @Formasitchijoh please do a code review if you have a chance.

@empty-codes empty-codes marked this pull request as ready for review December 19, 2024 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ragesoss ragesoss ragesoss left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@empty-codes @ragesoss