Support parameters

[svenbw]

Description

All requests are logged however any parameters which are configured in the route are not, this PR adds the support to log all the parameters to the database as a json string.

Since this might not be the desired behavior there is a new configuration option in the config file store_parameters that stores the parameters of the requests if set to true. This option is also available as an environment variable ROUTE_STORE_PARAMETERS.

Does this close any currently open issues?

No

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)
• This change requires a documentation update

...

Any other comments?

...

Checklist

• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[bilfeldt]

Thanks for the PR @svenbw 👍

Can you help me understand the use case for this? Is it because you want to monitor usage for a route like GET /posts/{post} but not count all requests regardless of which post, but instead monitor GET /posts/1, GET /posts/2, ... separatly?

I like the idea of having a "properties" array sitting there and you can then basically add whatever you want to it - not just route parameters. For example, you could decide to measure a route's usage for admins $parameters['is_admin'] = true separately from non-admins.

[svenbw]

I indeed want to monitor routes with arguments different arguments seperatly to track which user accessed which post.
The new parameters column will take all the route parameters and store them as a json array. From the route name and the route parameters it should be possible to reconstruct the original (full) route url if preferred.

I didn't add support to add additional properties: I wanted to keep the spirit of the library and log the information about the routes. Since it's additional content I think that this might require another column.

[bilfeldt]

I really like this 👍 I think it makes sense to implement, but please see my comments.

As far as I can see then this is a breaking change because the migration will need to be published for this to not break right?

As far as I can see, the implementation is global, meaning that either you include or exclude route parameters, there is no in-between where you do it for one route but not another right?

[bilfeldt]

I get that this is in theory a small performance improvement, but it does break if you start adding fields that are not nessesarely DB fields but accessors.

Suggested change

	} else {
	$query->select($fields);

[svenbw]

I didn't include it for the performance it was rather that I used a selector that was not in the table. I have a local change that splits up successors (e.g. for grouping) and real column names. In this change I also did some checks if other options where valid. Since it was out of scope of this PR I didn't add it. I will revert this to the original code.

[bilfeldt]

I think this is where you ment to post this @svenbw :)

As it turns out console table doesn't like an array, so I need to map each table element. And I now remember WHY the select is in place

$results = $query
    ->limit($this->option('limit'))
    ->get()
    ->map(function(RouteStatistic $item) {
        $data = $item->toArray();
        if (config('route-statistics.store_parameters')) {
            $data['parameters'] = json_encode($data['parameters']);
        } else {
            unset($data['parameters']); // Hide parameters if not supported in config
        }
        return $data;
    });

Instead of only selecting the fields in the query, then I suggest that we just pluck those fields AFTER query execution (which will then include all fields). That way it would work with accessors as well + solve your issue with arrays?

[svenbw]

Forgetting the parameters key by looping over the collection works if parameters is not enabled. However if they are still there it still needs to be casted to a string for table.

$results = $query
    ->limit($this->option('limit'))
    ->get()
    ->map(function (RouteStatistic $item) {
        $data = $item->toArray();
        if (config('route-statistics.store_parameters')) {
            $data['parameters'] = json_encode($data['parameters']);
        } else {
            unset($data['parameters']);
        }
        return $data;
    });

or

$results = $query
    ->limit($this->option('limit'))
    ->get()
    ->toArray();

if (config('route-statistics.store_parameters')) {
    Arr::map($results, function (array $item) {
        $item['parameters'] = json_encode($item['parameters']);
    });
} else {
    $results = Arr::map($results, function (array $item) {
        unset($item['parameters']);
        return $item;
    });
}

This is less efficient because php has to loop though each entry, else the data is collected by the database.

Can you explain why you would include other columns that are not in the fields? The table will be faulty because the headers of the table are generated by the getFields() function. If there is an additional column in the database the table will included empty headers and depending on the order even a field name above another column.

And @bilfeldt thanks for keeping the discussion going, I think we are getting there.

[bilfeldt]

Maybe something like:

$results = $query
    ->limit($this->option('limit'))
    ->get()
    ->pluck($fields)
    ->map(function (array $data): array {
        return array_map(function ($item): string {
            if (is_array($item)) {
                return json_encode($item);
            }

            return (string) $data;
        }, $data);
    }) 

[svenbw]

pluck doesn't work, since it will only pluck items with specific keys from an array, and at that point there are still rows.
I filtered each row with only which will eliminate the parameters column.

I think the idea of pluck is the same as using select, where select has the benefit of offloading the complexity to the database, and thus slightly more performant.

[svenbw]

• Not sure if I'm happy with the array casting, due to the impact on the console command.
• select is still in place but I split up fields and column selection

[bilfeldt]

Thanks 👍

[bilfeldt]

I am planning on releasing a new major version of this package with this PR, but due to holidays this will not be for the next 2 weeks.

[svenbw]

Thanks for the reviews and help.