nova-issues icon indicating copy to clipboard operation
nova-issues copied to clipboard

Dashboard metrics loop all dashboards to find a metric can lead to performance issue

Open dmason30 opened this issue 2 years ago • 5 comments
trafficstars

  • Nova Version: 4.25.x

Description:

I have just started adding multiple dashboards in nova and realised that the DashboardMetricController route loops all metrics across all dashboards on every request.

In our case this is an issue as we are adding a dashboard that shows cards based on a query, see example below:

// app/Nova/Dashboards/AffiliateDashboard.php
public function cards() {
     return Affiliate::query()
         // Some conditions here based on authorised user
         ->limit(20)
         ->get()
         ->map(function (Affiliate $model) {
             // The model will make the uriKey of this trend unique by appending the id onto it
             return SomeTrendMetric::make()->forAffiliate($model);           
         })
         ->toArray();
}

However, if we, for example, have another dashboard CustomerDashboard whenever any metric requests are run for that dashboard then it will also call the AffiliateDashboard::cards() method and run the above query unnecessarily.

We do intend to cache the above query but I have deliberately kept the example simple.

Suggested changes

Change the route definition to accept the dashboard uri key, obviously, this will require some changes in the metric vue components :

// laravel/nova/routes/api.php
- Route::get('/metrics/{metric}', DashboardMetricController::class)
+ Route::get('/dashboards/{dashboard}/metrics/{metric}', DashboardMetricController::class)

Then change the code that loops though metrics to filter based on dashboard uri key:

// laravel/nova/src/Nova.php
public static function allAvailableDashboardCards(NovaRequest $request)
{
    return collect(static::$dashboards)
+      ->where('uriKey', $request->input('dashboard'))
        ->filter
        ->authorize($request)
        ->flatMap(function ($dashboard) {
            /** @var \Laravel\Nova\Dashboard $dashboard */
            return $dashboard->cards();
        })->unique()
        ->filter
        ->authorize($request)
        ->values();
}

dmason30 avatar Jun 19 '23 16:06 dmason30

The cards component doesn't have access to the dashboard name and changing this on patch releases may cause massive breaking changes, especially for 3rd party packages. We can only look into this for Laravel Nova 5.

crynobone avatar Jun 26 '23 03:06 crynobone

Thought that might be the case 👍 Thanks for looking into it!

dmason30 avatar Jun 26 '23 09:06 dmason30

Revisiting this, I'm not sure how it's a breaking change?

davidhemphill avatar Nov 28 '23 01:11 davidhemphill

Mainly because we will need to change this logic:

https://github.com/laravel/nova/blob/1e0684663c3482d866e0bc3c7136aabb66e41c2b/resources/js/components/Metrics/ValueMetric.vue#L163-L171

CleanShot 2023-11-28 at 11 16 17

This can be done for built-in metrics but any existing 3rd party metrics wouldn't have this logic available until they update their code.

crynobone avatar Nov 28 '23 03:11 crynobone

Assuming we leave the original endpoint, we could update internal metrics to use a new one depending on where they come from (dashboards, resource index, resource detail, lens), yeah?

But also, I'm sort of the opinion that custom metrics shouldn't use internal Nova API endpoints...

davidhemphill avatar Nov 28 '23 17:11 davidhemphill