laravel / nova-issues

556 stars 34 forks source link

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

Open dmason30 opened 1 year ago

dmason30 commented 1 year ago

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();
}
crynobone commented 1 year ago

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.

dmason30 commented 1 year ago

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

davidhemphill commented 1 year ago

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

crynobone commented 1 year ago

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.

davidhemphill commented 1 year ago

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...