-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add type and outcome event dimensions #30
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,11 @@ class Metric extends ArrayObject | |
| // sdk identity | ||
| 'sdk', 'sdkVersion', | ||
| 'deviceName', 'deviceBrand', 'deviceModel', | ||
| // metric-scoped: what the row is about, and how it ended. | ||
| // Named `category`, not `type`: the SQL adapter already writes a `type` | ||
| // column holding 'event'/'gauge', and a dimension of that name silently | ||
| // overwrote it through array_merge. | ||
| 'category', 'outcome', | ||
| ]; | ||
|
|
||
| /** | ||
|
|
@@ -670,6 +675,13 @@ public static function getEventSchema(): array | |
| // sdk identity | ||
| $stringColumn('sdk', 256), | ||
| $stringColumn('sdkVersion', 255), | ||
| // metric-scoped dimensions. `category` is whatever the metric is | ||
| // broken down by — a messaging channel, a calling code — and | ||
| // `outcome` is how the attempt ended. Both are read with `metric` | ||
| // pinned, so the same column carries different value spaces across | ||
| // metrics, the way resourceId already carries ids of every kind. | ||
| $stringColumn('category', 64), | ||
| $stringColumn('outcome', 32), | ||
|
Comment on lines
+683
to
+684
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If a Database-backed installation already has its event collection, Knowledge Base Used: Prompt To Fix With AIThis is a comment left during a code review.
Path: src/Usage/Metric.php
Line: 683-684
Comment:
**Existing Database schemas stay stale**
If a Database-backed installation already has its event collection, `Database::setup()` swallows the duplicate-collection result without adding these new attributes, while `addBatch()` starts including them in documents. Consequently, upgraded deployments cannot reliably persist events containing `category` or `outcome`.
**Knowledge Base Used:**
- [Usage domain model](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/usage/-/docs/usage-domain-model.md)
- [ClickHouse usage adapter](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/usage/-/docs/clickhouse-usage-adapter.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
||
| $stringColumn('deviceName', 256), | ||
| $stringColumn('deviceBrand', 256), | ||
| $stringColumn('deviceModel', 255), | ||
|
|
@@ -756,14 +768,15 @@ public static function getSchema(): array | |
| public static function getEventIndexes(): array | ||
| { | ||
| $indexed = [ | ||
| 'category', 'outcome', | ||
| 'path', 'method', 'status', | ||
| 'service', 'resourceType', 'resourceId', 'resourceInternalId', | ||
| 'teamId', 'teamInternalId', | ||
| 'country', 'region', 'hostname', 'ip', | ||
| 'osName', 'clientType', 'clientName', 'deviceName', | ||
| ]; | ||
|
|
||
| $setIndexed = ['status', 'method', 'country', 'service', 'clientType', 'osName']; | ||
| $setIndexed = ['status', 'method', 'country', 'service', 'clientType', 'osName', 'category', 'outcome']; | ||
|
|
||
| return array_map( | ||
| static function (string $col) use ($setIndexed): array { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.