Skip to content
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

chore: change to per-product usageV2 topic #6113

Merged
merged 4 commits into from
Jan 30, 2025

Conversation

arcoraven
Copy link
Contributor

@arcoraven arcoraven commented Jan 30, 2025

PR-Codex overview

This PR focuses on changing the event tracking in the UsageV2 system to use per-product topics, enhancing the granularity of data collection.

Detailed summary

  • Updated sendUsageV2Events to accept productName and use it for topic naming.
  • Modified UsageV2Event interface by removing source and changing data to allow more flexible types.
  • Introduced getTopicName function to generate topics based on productName.
  • Adjusted UsageV2Producer to use a dynamic topic based on productName.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@arcoraven arcoraven requested a review from a team as a code owner January 30, 2025 06:22
Copy link

changeset-bot bot commented Jan 30, 2025

🦋 Changeset detected

Latest commit: cb53011

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@thirdweb-dev/service-utils Patch

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

Copy link

vercel bot commented Jan 30, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
thirdweb-www ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 30, 2025 6:36am
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
docs-v2 ⬜️ Skipped (Inspect) Jan 30, 2025 6:36am
thirdweb_playground ⬜️ Skipped (Inspect) Jan 30, 2025 6:36am
wallet-ui ⬜️ Skipped (Inspect) Jan 30, 2025 6:36am

Copy link

graphite-app bot commented Jan 30, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • merge-queue - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

/**
* The source of the event. Example: "storage"
*/
source: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer needed because events are sent to product-specific topics (and stored to product-specific tables).

Copy link

codecov bot commented Jan 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.88%. Comparing base (1616b7f) to head (cb53011).
Report is 17 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6113      +/-   ##
==========================================
+ Coverage   56.81%   56.88%   +0.06%     
==========================================
  Files        1152     1153       +1     
  Lines       63906    63896      -10     
  Branches     5178     5183       +5     
==========================================
+ Hits        36311    36349      +38     
+ Misses      26865    26819      -46     
+ Partials      730      728       -2     
Flag Coverage Δ *Carryforward flag
legacy_packages 65.68% <ø> (ø) Carriedforward from f107954
packages 55.08% <ø> (+0.08%) ⬆️

*This pull request uses carry forward flags. Click here to find out more.

see 25 files with indirect coverage changes

@vercel vercel bot temporarily deployed to Preview – wallet-ui January 30, 2025 06:24 Inactive
@vercel vercel bot temporarily deployed to Preview – thirdweb_playground January 30, 2025 06:24 Inactive
@vercel vercel bot temporarily deployed to Preview – docs-v2 January 30, 2025 06:24 Inactive
Comment on lines 90 to 98
return {
...event,
id: event.id ?? randomUUID(),
created_at: event.created_at ?? new Date(),
source: event.source,
action: event.action,
// Remove the "team_" prefix, if any.
team_id: event.team_id.startsWith("team_")
? event.team_id.slice(5)
: event.team_id,
project_id: event.project_id,
sdk_name: event.sdk_name,
sdk_platform: event.sdk_platform,
sdk_version: event.sdk_version,
sdk_os: event.sdk_os,
product_name: event.product_name,
product_version: event.product_version,
data: JSON.stringify(event.data),
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider moving ...event after the team_id assignment. Currently, the explicit property assignments will be overridden by the spread operator, preventing the defaults from taking effect. The intended order should be:

return {
  id: event.id ?? randomUUID(),
  created_at: event.created_at ?? new Date(),
  team_id: event.team_id.startsWith("team_")
    ? event.team_id.slice(5)
    : event.team_id,
  ...event
};

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

*/
data: Record<string, unknown>;
[key: string]: boolean | number | string | Date | null | undefined;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using an index signature here could cause TypeScript to miss type errors on the explicitly defined properties in UsageV2Event. A safer approach would be to add a dedicated data property with type Record<string, boolean | number | string | Date | null> to handle the arbitrary key-value pairs, while keeping the explicit properties type-safe.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

@vercel vercel bot temporarily deployed to Preview – docs-v2 January 30, 2025 06:28 Inactive
@vercel vercel bot temporarily deployed to Preview – thirdweb_playground January 30, 2025 06:28 Inactive
@vercel vercel bot temporarily deployed to Preview – wallet-ui January 30, 2025 06:28 Inactive
Copy link
Contributor

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
thirdweb (esm) 46.6 KB (0%) 933 ms (0%) 569 ms (+25.72% 🔺) 1.6 s
thirdweb (cjs) 122.39 KB (0%) 2.5 s (0%) 1.6 s (+5.97% 🔺) 4 s
thirdweb (minimal + tree-shaking) 5.58 KB (0%) 112 ms (0%) 210 ms (+398.15% 🔺) 321 ms
thirdweb/chains (tree-shaking) 506 B (0%) 10 ms (0%) 9 ms (-43.86% 🔽) 19 ms
thirdweb/react (minimal + tree-shaking) 19.28 KB (0%) 386 ms (0%) 313 ms (+208.79% 🔺) 698 ms

@arcoraven arcoraven merged commit 6818cba into main Jan 30, 2025
29 of 30 checks passed
@arcoraven arcoraven deleted the ph/serviceUtilsProductTopics branch January 30, 2025 06:32
@joaquim-verges joaquim-verges mentioned this pull request Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants