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

Implementing metrics into the status page #58

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Ionys320
Copy link

@Ionys320 Ionys320 commented May 17, 2024

This is a WIP but allows you to monitor the changes related to issue #56.

This feature is actively using Alpine. Maybe some parts could be improved to use Laravel in a better way (for example, avoid using <?php echo json_encode($metric); ?> in the code).

No issue is currently know.

Please note that for debugging purposes, each metric is displayed twice. The reason is that by default, we only have one metric. It allows testing the individual component with two different instances.

Screen of the current version:
image

@Ionys320
Copy link
Author

Btw I have some questions on how/when the calculation (avg, sum) should be performed. I guess in src/View/Components/Metrics.php, but I'm not sure.

resources/views/components/metric.blade.php Outdated Show resolved Hide resolved
src/View/Components/Metrics.php Outdated Show resolved Hide resolved
src/View/Components/Metrics.php Outdated Show resolved Hide resolved
resources/views/components/metric.blade.php Outdated Show resolved Hide resolved
<div class="flex flex-col gap-8">
@foreach($metrics as $metric)
<x-cachet::metric :metric="$metric" />
<x-cachet::metric :metric="$metric" />
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this duplication can be removed?

Suggested change
<x-cachet::metric :metric="$metric" />

Copy link
Author

Choose a reason for hiding this comment

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

For the moment, I keep it for debugging purposes (by default, we only have one metric. It allows testing the individual component with two different instances.)

@jbrooksuk
Copy link
Member

@Ionys320 how's this going? :)

@Ionys320
Copy link
Author

Ionys320 commented Jul 3, 2024

Hi @jbrooksuk! I would need to know how the avg/sum must be made, and where. I don't really know how this part of the feature is working. And after this, I don't know if something would be missing, let me know!

@jbrooksuk
Copy link
Member

I'd take inspiration from the current code in v2.4 (https://github.com/cachethq/cachet/blob/2.4/app/Repositories/Metric/MySqlRepository.php).

@Ionys320
Copy link
Author

I'd take inspiration from the current code in v2.4 (https://github.com/cachethq/cachet/blob/2.4/app/Repositories/Metric/MySqlRepository.php).

The "problem" is that, actually, ChartJS displays the whole dataset. For example, if I ask the data of the last 30 days, I'll have each point created for the metric X of the last 30 days. I see that the previous code is making a GROUP BY, but I wonder if it's still useful.

May I ask you to check this part? It will probably be simplier.

Thanks!

@jbrooksuk
Copy link
Member

@Ionys320 I've rebased this onto the latest version of main and updated some styling of the charts.

@jbrooksuk
Copy link
Member

@Ionys320 how would you feel about utilizing Filament's own stats component here, like we do for the dashboard? Would that simplify anything?

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

Successfully merging this pull request may close these issues.

2 participants