[service-utils] feat: Add usageV2 support #6021
Conversation
🦋 Changeset detectedLatest commit: 32fe958 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| if (!resp.ok) { | ||
| throw new Error( | ||
| `[UsageV2] unexpected response ${resp.status}: ${await resp.text()}`, | ||
| ); | ||
| } | ||
| resp.body?.cancel(); |
There was a problem hiding this comment.
The response body is consumed by resp.text() before throwing the error, making the subsequent resp.body?.cancel() call potentially unsafe since the body stream has already been read. Consider either:
- Moving the
text()call before constructing the error:
const text = await resp.text();
if (!resp.ok) {
throw new Error(`[UsageV2] unexpected response ${resp.status}: ${text}`);
}- Or removing the
cancel()call since the body is already consumed
Either approach would prevent attempting to operate on an already consumed response body.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
size-limit report 📦
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6021 +/- ##
==========================================
+ Coverage 54.87% 54.89% +0.01%
==========================================
Files 1151 1151
Lines 61169 61175 +6
Branches 5152 5156 +4
==========================================
+ Hits 33566 33579 +13
+ Misses 26876 26869 -7
Partials 727 727
*This pull request uses carry forward flags. Click here to find out more. |
| return { | ||
| id: id ?? randomUUID(), | ||
| created_at: created_at ?? new Date(), | ||
| data: JSON.stringify(data), |
There was a problem hiding this comment.
According to the schema, the data field is already defined as a JSON object (Record<string, unknown>). The JSON.stringify() call here will convert it to a string, which is incorrect. Please remove the JSON.stringify() wrapper around data to maintain the correct type.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
arcoraven
changed the title
| @@ -0,0 +1,45 @@ | |||
| import type { UsageV2Event } from "src/core/usageV2.js"; | |||
There was a problem hiding this comment.
Change import path from 'src/core/usageV2.js' to '../core/usageV2.js' to use relative path instead of absolute path
Spotted by Graphite Reviewer (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| @@ -0,0 +1,45 @@ | |||
| import type { UsageV2Event } from "src/core/usageV2.js"; | |||
There was a problem hiding this comment.
Change import path from 'src/core/usageV2.js' to '../core/usageV2.js' to use relative path instead of absolute path
Spotted by Graphite Reviewer (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| @@ -0,0 +1,45 @@ | |||
| import type { UsageV2Event } from "src/core/usageV2.js"; | |||
There was a problem hiding this comment.
Change import path from 'src/core/usageV2.js' to '../core/usageV2.js' to use relative path
Spotted by Graphite Reviewer (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| @@ -0,0 +1,45 @@ | |||
| import type { UsageV2Event } from "src/core/usageV2.js"; | |||
There was a problem hiding this comment.
Change import path from 'src/core/usageV2.js' to '../core/usageV2.js' to use relative path instead of src-based path
Spotted by Graphite Reviewer (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| @@ -0,0 +1,45 @@ | |||
| import type { UsageV2Event } from "src/core/usageV2.js"; | |||
There was a problem hiding this comment.
Change import path from 'src/core/usageV2.js' to '../core/usageV2.js' to use relative path instead of absolute path
Spotted by Graphite Reviewer (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| @@ -0,0 +1,45 @@ | |||
| import type { UsageV2Event } from "src/core/usageV2.js"; | |||
There was a problem hiding this comment.
Import path should use relative path '../core/usageV2.js' instead of absolute path 'src/core/usageV2.js'
Spotted by Graphite Reviewer (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| @@ -0,0 +1,45 @@ | |||
| import type { UsageV2Event } from "src/core/usageV2.js"; | |||
There was a problem hiding this comment.
Change import path from 'src/core/usageV2.js' to '../core/usageV2.js' to use relative path instead of absolute path
Spotted by Graphite Reviewer (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| @@ -0,0 +1,45 @@ | |||
| import type { UsageV2Event } from "src/core/usageV2.js"; | |||
There was a problem hiding this comment.
Change import path from 'src/core/usageV2.js' to '../core/usageV2.js' to fix TypeScript module resolution
Spotted by Graphite Reviewer (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| @@ -46,6 +46,7 @@ | |||
| "sideEffects": false, | |||
| "dependencies": { | |||
| "aws4fetch": "1.0.20", | |||
| "kafkajs": "^2.2.4", | |||
There was a problem hiding this comment.
pin the version
| "kafkajs": "^2.2.4", | |
| "kafkajs": "2.2.4", |
| @@ -0,0 +1,45 @@ | |||
| import type { UsageV2Event } from "src/core/usageV2.js"; | |||
| import { randomUUID } from "node:crypto"; | ||
| import { checkServerIdentity } from "node:tls"; | ||
| import { Kafka, type Producer } from "kafkajs"; | ||
| import type { UsageV2Event } from "src/core/usageV2.js"; |
| * A unique identifier for the event. Defaults to a random UUID. | ||
| * Useful if your service retries sending events. | ||
| */ | ||
| id?: `${string}-${string}-${string}-${string}-${string}`; |
There was a problem hiding this comment.
might be too strict on the type here, just string should be fine - unless you want to specifically enforce uuidv4
| /** | ||
| * The source of the event. Example: "storage" | ||
| */ | ||
| source: string; |
There was a problem hiding this comment.
COULD consider enforcing the actual allowed service names here
| id: id ?? randomUUID(), | ||
| created_at: created_at ?? new Date(), | ||
| data: JSON.stringify(data), | ||
| ...rest, |
There was a problem hiding this comment.
might be better to just spell out each field here
There was a problem hiding this comment.
for team_id field we MAY want to de-normalize or normalize it - since it might be passed in as team_<id> or just <id> based on how the service consumes it
| await this.producer.send({ | ||
| topic: TOPIC_USAGE_V2, | ||
| messages: parsedEvents.map((event) => ({ | ||
| value: JSON.stringify(event), | ||
| })), | ||
| }); |
There was a problem hiding this comment.
what should happen if this were to throw?
https://linear.app/thirdweb/issue/CORE-711/usage-v2-service-utils
PR-Codex overview
This PR introduces support for
usageV2in theservice-utilspackage, enhancing event handling and integration with Kafka for usage tracking.Detailed summary
kafkajsas a dependency.sendUsageV2Eventsfunction to send events to Kafka.UsageV2Eventinterface with detailed event properties.UsageV2Producerclass for managing Kafka connections and sending events.