feat: Configure AI usage counter - #11
Conversation
Code Review SummaryThe PR introduces AI usage tracking and plan entitlements. However, the implementation has critical logic errors in how features are authorized and identified, rendering the entitlement system largely ineffective in its current state. 🚀 Key Improvements
💡 Minor Suggestions
🚨 Critical Issues
|
35627ef to
ea97334
Compare
ea97334 to
d97f1f4
Compare
nfebe
left a comment
There was a problem hiding this comment.
This looks good but since we have multiple projects that the question of tokens will come up. I built and publish a package that will simplify things there is already a task to implement it in the core and it may remove the need for one table you have here.
See: https://github.com/whilesmartphp/eloquent-agent-metrics
|
@nfebe what's the issue number on trakli core? I can't find it |
|
I already implemented it so its closed |
# Conflicts: # src/CloudServiceProvider.php # Conflicts: # src/CloudServiceProvider.php
5c0d2c0 to
b89795f
Compare
nfebe
left a comment
There was a problem hiding this comment.
There is already a way to measure the usage in core
| $table->unsignedBigInteger('user_id'); | ||
| $table->morphs('owner'); | ||
| $table->timestamp('period_start'); | ||
| $table->integer('tokens_used'); |
There was a problem hiding this comment.
Could you add this to the webservice and see how we can eliminate this table?
https://github.com/whilesmartphp/eloquent-agent-metrics
Was this solved?
There was a problem hiding this comment.
I'm still working on this and the other open pr.
There was a problem hiding this comment.
So I already integrated the metrics tracking in the core : https://github.com/trakli/webservice/blob/dev/composer.json#L26
So you just need to used it and remove the custom tracking tables
|
This doesn't load against current Dropping the counter table is addressed, and
This should be a class TokenMeterUsage implements UsageMeter
{
public function remaining(?Model $owner, string $meter, int|float $allowance): int|float
{
if ($meter !== 'ai_tokens' || $owner === null || is_infinite($allowance)) {
return INF;
}
return max(0, $allowance - $owner->tokensUsed(now()->startOfMonth()));
}
public function consume(?Model $owner, string $meter, int $amount): void
{
// core already records through TokenMeter
}
}That needs plans carrying |
|
@nfebe are we taking away the cloudplans.php config? I see that most of the information in the config is in the Eloquent Entitlements plans table |
| /** | ||
| * Determine if the owner is allowed to use a given feature. | ||
| */ | ||
| public function allows(?Model $owner, string $feature): bool |
There was a problem hiding this comment.
The allows method is currently hardcoded to return true, which bypasses all plan-based feature restrictions. This should delegate to the check method to ensure that entitlements are properly enforced across the application.
| public function allows(?Model $owner, string $feature): bool | |
| public function allows(?Model $owner, string $feature): bool | |
| { | |
| return $this->check($owner, $feature)->allowed(); | |
| } |
| $planCode = $this->getPlanCode($owner); | ||
| $plan = config("cloudplans.plans.{$planCode}"); | ||
|
|
||
| // Check if the feature is listed in the plan's features or permissions |
There was a problem hiding this comment.
The features array in config/cloudplans.php contains human-readable strings for the UI (e.g., 'Up to 3 wallets'). Checking for a programmatic feature key (like 'ai_chat') against this array will always fail. You should introduce a machine-readable permissions array in the config or map feature keys to these strings.
| // Check if the feature is listed in the plan's features or permissions | |
| // Suggested: Use a dedicated permissions array for programmatic keys | |
| $permissions = $plan['permissions'] ?? []; | |
| if ($plan && in_array($feature, $permissions, true)) { | |
| return AccessResult::allow($feature); | |
| } |
| { | ||
| $subscription = $this->activeSubscriptionFor($owner); | ||
| if ($subscription && $subscription->plan) { | ||
| return explode('-', $subscription->plan->key)[0]; // gets monthly from monthly-eu |
There was a problem hiding this comment.
Determining the plan code by splitting the key string is brittle. If a plan key doesn't follow the exact prefix-suffix format (e.g., a three-part key), this logic might return an incorrect value. Consider using a more robust mapping or Laravel's Str::before helper.
| return explode('-', $subscription->plan->key)[0]; // gets monthly from monthly-eu | |
| return (string) \Illuminate\Support\Str::before($subscription->plan->key, '-'); |
Fixes #7
Depends on #10