feat(core): expand telemetry to schema v2 with heartbeat and new signals#4933
feat(core): expand telemetry to schema v2 with heartbeat and new signals#4933dlhck wants to merge 9 commits into
Conversation
Adds schemaVersion 2 to the telemetry payload along with a repeating 24h heartbeat send (sendReason startup/heartbeat), process uptime, and a runtime section (runtime type, package manager, ts-node, cpu/memory). Extends DB metrics with order lifecycle and i18n breadth, adds API and security posture flags plus dotted customized-strategy paths to config, and a multiCurrency feature flag.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR expands anonymous telemetry: new ChangesTelemetry expansion
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TelemetryService
participant FeaturesCollector
participant SystemInfoCollector
participant DatabaseCollector
participant Endpoint
TelemetryService->>TelemetryService: schedule delayed sendTelemetry('startup')
TelemetryService->>DatabaseCollector: collect entity/order/i18n metrics
DatabaseCollector-->>TelemetryService: metrics (with currency count)
TelemetryService->>SystemInfoCollector: collect runtime info
SystemInfoCollector-->>TelemetryService: runtime data
TelemetryService->>FeaturesCollector: collect(config, currencyCount)
FeaturesCollector-->>TelemetryService: features (multiCurrency)
TelemetryService->>Endpoint: POST payload (schemaVersion, sendReason=startup)
TelemetryService->>TelemetryService: scheduleHeartbeat() (jittered, self-rescheduling)
loop every ~24h
TelemetryService->>Endpoint: POST payload (sendReason=heartbeat)
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…tryService documentation
|
Dashboard Preview: https://admin-dashboard-eumn087p5-vendure.vercel.app |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/core/src/telemetry/collectors/system-info.collector.ts (1)
51-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment claims parity with
helpers.tsbut the fallback behavior differs.The comment says this "mirrors"
packages/create/src/helpers.ts, but that helper defaults unrecognized/absent user-agents to'npm', while this method returns'unknown'in the same case. Not a bug (an explicit "unknown" is arguably more honest for telemetry), but the comment is misleading for future maintainers.📝 Suggested comment fix
- // Mirrors the parsing in packages/create/src/helpers.ts + // Parses similarly to packages/create/src/helpers.ts, but reports + // 'unknown' (rather than defaulting to 'npm') when unrecognized.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/telemetry/collectors/system-info.collector.ts` around lines 51 - 62, Update the inline note in system-info.collector.ts near getPackageManager so it no longer claims parity with packages/create/src/helpers.ts if the fallback behavior differs; either remove the “Mirrors” wording or clarify that this method intentionally diverges by returning unknown for unrecognized or absent user agents. Keep the implementation in getPackageManager unchanged and make the comment accurately describe the current telemetry behavior.packages/core/src/telemetry/collectors/config.collector.ts (1)
179-188: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse the shared superadmin defaults here
getDefaultSuperadminCredentials()should compare againstSUPER_ADMIN_USER_IDENTIFIERandSUPER_ADMIN_USER_PASSWORDfrom@vendure/common/lib/shared-constants, not hardcoded'superadmin'literals. That keeps this telemetry flag aligned withdefaultConfigand prevents it from drifting if the defaults ever change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/telemetry/collectors/config.collector.ts` around lines 179 - 188, The getDefaultSuperadminCredentials() method is using hardcoded 'superadmin' literals instead of the shared superadmin defaults. Update the comparison in config.collector.ts to use SUPER_ADMIN_USER_IDENTIFIER and SUPER_ADMIN_USER_PASSWORD from `@vendure/common/lib/shared-constants`, keeping the telemetry check aligned with defaultConfig and preventing drift if the defaults change.packages/core/src/telemetry/collectors/database.collector.ts (1)
82-82: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDrop the unnecessary
as anycast on the draft-order count.Order.statealready usesOrderState, and'Draft'is a valid member; removing the cast keeps the type check intact and avoids hiding typos or future state changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/telemetry/collectors/database.collector.ts` at line 82, The draft-order count in database.collector.ts is using an unnecessary type cast that hides type checking. Update the count logic in the collector method that calls repo.count for the Draft state by removing the as any cast and using the existing OrderState type directly, so the Order.state filter remains type-safe and continues to catch invalid state values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/telemetry/collectors/config.collector.ts`:
- Around line 179-188: The getDefaultSuperadminCredentials() method is using
hardcoded 'superadmin' literals instead of the shared superadmin defaults.
Update the comparison in config.collector.ts to use SUPER_ADMIN_USER_IDENTIFIER
and SUPER_ADMIN_USER_PASSWORD from `@vendure/common/lib/shared-constants`, keeping
the telemetry check aligned with defaultConfig and preventing drift if the
defaults change.
In `@packages/core/src/telemetry/collectors/database.collector.ts`:
- Line 82: The draft-order count in database.collector.ts is using an
unnecessary type cast that hides type checking. Update the count logic in the
collector method that calls repo.count for the Draft state by removing the as
any cast and using the existing OrderState type directly, so the Order.state
filter remains type-safe and continues to catch invalid state values.
In `@packages/core/src/telemetry/collectors/system-info.collector.ts`:
- Around line 51-62: Update the inline note in system-info.collector.ts near
getPackageManager so it no longer claims parity with
packages/create/src/helpers.ts if the fallback behavior differs; either remove
the “Mirrors” wording or clarify that this method intentionally diverges by
returning unknown for unrecognized or absent user agents. Keep the
implementation in getPackageManager unchanged and make the comment accurately
describe the current telemetry behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e532066d-a5f3-4b1d-aa4e-7e031246f2d1
📒 Files selected for processing (14)
docs/docs/reference/core-plugins/asset-server-plugin/s3asset-storage-strategy.mdxdocs/docs/reference/typescript-api/telemetry/telemetry-service.mdxdocs/manifest.jsonpackages/core/src/telemetry/collectors/config.collector.spec.tspackages/core/src/telemetry/collectors/config.collector.tspackages/core/src/telemetry/collectors/database.collector.spec.tspackages/core/src/telemetry/collectors/database.collector.tspackages/core/src/telemetry/collectors/features.collector.spec.tspackages/core/src/telemetry/collectors/features.collector.tspackages/core/src/telemetry/collectors/system-info.collector.spec.tspackages/core/src/telemetry/collectors/system-info.collector.tspackages/core/src/telemetry/telemetry.service.spec.tspackages/core/src/telemetry/telemetry.service.tspackages/core/src/telemetry/telemetry.types.ts
Update the telemetry disclosure docs and TelemetryService docblock to cover the 24h heartbeat and new data categories. Allowlist order-type keys against the OrderType enum so arbitrary varchar values cannot leak. Replace the fixed heartbeat interval with a self-rescheduling, jittered timeout and bound the entity-count sweep concurrency. Parallelize the order sub-queries, run order and i18n collection concurrently, return undefined for an empty orders result, detect the deprecated root-level entityIdStrategy in customizedStrategies, and extract a named TelemetryI18nMetrics interface.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/docs/guides/developer-guide/telemetry/index.mdx (2)
32-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving "Process uptime" under "Runtime and Hardware Shape".
"Process uptime in seconds at the time of the event" (line 32) is listed under "Version Information," but the newly added "Runtime and Hardware Shape" section (lines 34–39) is a more natural home for this metric alongside runtime type, CPU count, and memory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/docs/guides/developer-guide/telemetry/index.mdx` around lines 32 - 39, Move the “Process uptime in seconds at the time of the event” entry from the Version Information list into the Runtime and Hardware Shape section so it sits with the runtime/CPU/memory metrics. Update the telemetry docs in the developer guide index accordingly, keeping the existing wording but relocating the bullet to the new section.
109-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the distinction between "order data" and "Order Lifecycle Metrics".
Line 110 states "Customer data, order data, or any business data" is not collected, while lines 71–73 describe "Order Lifecycle Metrics" (range-bucketed counts). A reader may find this contradictory. Consider adding a brief parenthetical to line 110, e.g., "Customer data, order data (actual order contents), or any business data" to make clear that only aggregate counts are collected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/docs/guides/developer-guide/telemetry/index.mdx` around lines 109 - 115, Clarify the telemetry exclusions in the docs by distinguishing actual order contents from aggregate metrics. Update the bullets in the telemetry guide around the “Customer data, order data, or any business data” item so it explicitly says “order data” means actual order contents, not the aggregated “Order Lifecycle Metrics” described earlier. Keep the wording aligned with the existing telemetry section and use the same list item block so readers don’t read the metrics description as contradictory.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/docs/guides/developer-guide/telemetry/index.mdx`:
- Around line 32-39: Move the “Process uptime in seconds at the time of the
event” entry from the Version Information list into the Runtime and Hardware
Shape section so it sits with the runtime/CPU/memory metrics. Update the
telemetry docs in the developer guide index accordingly, keeping the existing
wording but relocating the bullet to the new section.
- Around line 109-115: Clarify the telemetry exclusions in the docs by
distinguishing actual order contents from aggregate metrics. Update the bullets
in the telemetry guide around the “Customer data, order data, or any business
data” item so it explicitly says “order data” means actual order contents, not
the aggregated “Order Lifecycle Metrics” described earlier. Keep the wording
aligned with the existing telemetry section and use the same list item block so
readers don’t read the metrics description as contradictory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b18f4349-8e1d-4dc9-aafb-2ec8fbed4617
📒 Files selected for processing (11)
docs/docs/guides/developer-guide/telemetry/index.mdxdocs/docs/reference/typescript-api/telemetry/telemetry-service.mdxdocs/manifest.jsonpackages/core/src/telemetry/collectors/config.collector.spec.tspackages/core/src/telemetry/collectors/config.collector.tspackages/core/src/telemetry/collectors/database.collector.spec.tspackages/core/src/telemetry/collectors/database.collector.tspackages/core/src/telemetry/collectors/system-info.collector.spec.tspackages/core/src/telemetry/telemetry.service.spec.tspackages/core/src/telemetry/telemetry.service.tspackages/core/src/telemetry/telemetry.types.ts
✅ Files skipped from review due to trivial changes (2)
- docs/manifest.json
- docs/docs/reference/typescript-api/telemetry/telemetry-service.mdx
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/core/src/telemetry/collectors/system-info.collector.spec.ts
- packages/core/src/telemetry/collectors/config.collector.ts
- packages/core/src/telemetry/collectors/config.collector.spec.ts
- packages/core/src/telemetry/collectors/database.collector.spec.ts
- packages/core/src/telemetry/collectors/database.collector.ts
- packages/core/src/telemetry/telemetry.types.ts
- packages/core/src/telemetry/telemetry.service.spec.ts
The telemetry config collector deliberately reads the deprecated adminApiPlayground/shopApiPlayground options and the deprecated root-level entityIdStrategy: measuring remaining adoption of deprecated options is what tells the core team when they can be removed. Annotate these reads with // NOSONAR so SonarCloud does not flag rule S1874, with a comment explaining the intent.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| private async sendTelemetry(): Promise<void> { | ||
| const payload = await this.collectPayload(); | ||
| private scheduleHeartbeat(): void { | ||
| const delay = HEARTBEAT_INTERVAL_MS + Math.floor(Math.random() * HEARTBEAT_MAX_JITTER_MS); |
michaelbromley
left a comment
michaelbromley
left a comment
There was a problem hiding this comment.
The design is solid and the privacy engineering is careful — I traced the customizedStrategies comparison through mergeConfig/simpleDeepClone and it's genuinely sound (un-overridden strategy instances are kept by reference, and comparing by class name correctly handles plugins re-instantiating a default strategy). No privacy leaks found: corsWildcardOrigin only ever reduces to a boolean, tokenMethod is a closed union so nothing can leak via stringification, and the superadmin-credentials check never puts the values anywhere. Failure isolation and the guard re-checking claims both hold up.
Two must-fixes though, both cheap:
- Order-metrics queries are unindexed full scans on a table that can have millions of rows, and they fire on the startup send too — see inline comment on
database.collector.ts. onApplicationBootstrap()isn't idempotent, and a second call creates a heartbeat chain that shutdown can't cancel — see inline comments ontelemetry.service.tsand the spec.
Nits (non-blocking):
SINGLE_STRATEGY_PATHSomitsauthOptions.entityAccessControlStrategyandorderOptions.activeOrderStrategy— the latter especially is commonly customized and worth adding (inline comment).collectPayload()awaitsinstallationIdCollector.collect()anddatabaseCollector.collect()sequentially even though they're independent — could bePromise.all'd for consistency with the parallelization used elsewhere in this PR. Immaterial at a 24h cadence.- The diff picks up an unrelated line-number drift in
docs/docs/reference/core-plugins/asset-server-plugin/s3asset-storage-strategy.mdx(sourceLine="155"→"156") — harmless doc-generator side effect, but worth a note in the PR description so reviewers aren't puzzled by an S3 doc in a telemetry PR.
Test quality overall is strong — config sections that throw on access, minified class names, rogue OrderType values, jitter timing, unref, shutdown-clears-timer are all pinned. The one gap is the double-bootstrap test (inline comment).
| // A handful of small, indexed queries — run them in parallel (unlike | ||
| // the larger entity-count sweep). | ||
| const [placed, active, draft, placedLast30d, byType] = await Promise.all([ | ||
| this.safeBucket(() => repo.count({ where: { orderPlacedAt: Not(IsNull()) } })), | ||
| this.safeBucket(() => repo.count({ where: { active: true } })), | ||
| this.safeBucket(() => repo.count({ where: { state: 'Draft' as any } })), | ||
| this.safeBucket(() => repo.count({ where: { orderPlacedAt: MoreThanOrEqual(since) } })), | ||
| this.collectOrdersByType(repo), | ||
| ]); |
There was a problem hiding this comment.
Must fix: the comment says "small, indexed queries", but Order has no index on active, state, or type — only code, orderPlacedAt, aggregateOrder and customer are indexed (see order.entity.ts). So count({ where: { active: true } }), count({ where: { state: 'Draft' } }) and the GROUP BY o.type in collectOrdersByType are full-scan-class queries on what is typically the largest table in a production shop (abandoned carts easily push it into the millions).
And this isn't just a heartbeat cost: collectOrderMetrics() runs on the startup send too, 5 seconds after bootstrap — exactly when a freshly-deployed server is least able to absorb an unplanned scan of its biggest table.
Options, roughly in order of preference:
- collect order metrics on
heartbeatsends only, skipping them forstartup; - gate collection behind the already-collected total Order count (skip when the bucket is
100k+); - add indexes on
activeandstatevia a migration (heavier, affects all installs).
| this.delayTimeout = setTimeout(() => { | ||
| this.delayTimeout = undefined; | ||
| this.sendTelemetry().catch(() => { | ||
| this.sendTelemetry('startup').catch(() => { | ||
| // Silently ignore all errors | ||
| }); | ||
| }, 5000); | ||
| this.scheduleHeartbeat(); | ||
| }, STARTUP_DELAY_MS); |
There was a problem hiding this comment.
Must fix: onApplicationBootstrap() isn't idempotent — this assigns a fresh timeout without clearing any existing one, and scheduleHeartbeat() does the same with this.heartbeatTimeout. If the hook ever fires twice on the same instance (module double-imported without @Global(), hybrid multi-app setups, test harnesses re-triggering lifecycle hooks), you get two startup sends and — worse — two independent self-rescheduling heartbeat chains. Since each chain reassigns this.heartbeatTimeout on every reschedule, onApplicationShutdown() only holds a reference to whichever timer was scheduled last: it can cancel one chain, and the other keeps sending duplicate heartbeats for the life of the process with no way to stop it.
Cheap fix: a hasBootstrapped guard here, or clearTimeout any existing ref before reassigning in both places.
Relatedly, the existing spec 'handles double onApplicationBootstrap call without error' (telemetry.service.spec.ts, ~line 663) is too weak to catch this: toHaveBeenCalled() passes whether one send happens or two. Asserting toHaveBeenCalledTimes(1) there would fail against the current implementation — and would pin the idempotency fix once it's in.
| const SINGLE_STRATEGY_PATHS: Record<string, string[]> = { | ||
| authOptions: [ | ||
| 'sessionCacheStrategy', | ||
| 'passwordHashingStrategy', | ||
| 'passwordValidationStrategy', | ||
| 'verificationTokenStrategy', | ||
| 'adminApiKeyStrategy', | ||
| 'shopApiKeyStrategy', | ||
| 'customerChannelAssignmentStrategy', | ||
| ], | ||
| assetOptions: ['assetNamingStrategy', 'assetStorageStrategy', 'assetPreviewStrategy'], | ||
| catalogOptions: [ | ||
| 'productVariantPriceSelectionStrategy', | ||
| 'productVariantPriceCalculationStrategy', | ||
| 'productVariantPriceUpdateStrategy', | ||
| 'stockDisplayStrategy', | ||
| 'stockLocationStrategy', | ||
| ], | ||
| orderOptions: [ | ||
| 'orderItemPriceCalculationStrategy', | ||
| 'stockAllocationStrategy', | ||
| 'mergeStrategy', | ||
| 'checkoutMergeStrategy', | ||
| 'orderCodeStrategy', | ||
| 'orderByCodeAccessStrategy', | ||
| 'changedPriceHandlingStrategy', | ||
| 'orderLineDiscountDistributionStrategy', | ||
| 'orderPlacedStrategy', | ||
| 'orderSellerStrategy', | ||
| 'guestCheckoutStrategy', | ||
| ], | ||
| taxOptions: ['taxZoneStrategy', 'taxLineCalculationStrategy', 'orderTaxCalculationStrategy'], | ||
| entityOptions: ['entityIdStrategy', 'moneyStrategy', 'slugStrategy'], | ||
| importExportOptions: ['assetImportStrategy'], | ||
| jobQueueOptions: ['jobQueueStrategy', 'jobBufferStorageStrategy'], | ||
| schedulerOptions: ['schedulerStrategy'], | ||
| systemOptions: ['cacheStrategy', 'instrumentationStrategy'], | ||
| }; |
There was a problem hiding this comment.
Nit: this allowlist omits two real single-strategy fields from default-config.ts: authOptions.entityAccessControlStrategy and orderOptions.activeOrderStrategy. Not a correctness bug — just silent under-coverage. activeOrderStrategy in particular is a commonly-customized field that seems worth the signal.
| const installationId = await this.installationIdCollector.collect(); | ||
| const databaseInfo = await this.databaseCollector.collect(); |
There was a problem hiding this comment.
Nit: these two collects are independent and could be Promise.all'd — immaterial at this cadence, just inconsistent with the parallelization pattern this PR uses in the collectors themselves.
3 participants
Description
Expands the anonymous telemetry payload to schema v2, closing the largest signal gaps in the current collection: send cadence, order lifecycle, internationalization breadth, runtime environment, security posture, and strategy customization.
What's new
Cadence & payload metadata
schemaVersion: 2,sendReason(startup|heartbeat) anduptimeSeconds.unref()ed so it never keeps the process alive, is cleared on shutdown, and all existing guards (worker process,VENDURE_DISABLE_TELEMETRY, CI detection) are re-checked on every send. This makes uptime, retention and churn measurable — previously an installation only reported when it restarted.runtimesection (SystemInfoCollector)runtimeType(node/bun/deno),packageManager(parsed fromnpm_config_user_agent, same approach as@vendure/create),tsNode,cpuCount,totalMemoryGb.metrics.orders(DatabaseCollector)placed,active,draft,placedLast30d, andbyType(Regular/Seller/Aggregate). Total Order count is dominated by abandoned carts; this separates real trading activity and surfaces draft-order and multi-vendor adoption directly.metrics.i18n(DatabaseCollector)features.multiCurrencyflag.Config posture fields (ConfigCollector)
apiIntrospectionEnabled,apiPlaygroundEnabled,apiDebugEnabled,trustProxyEnabled,corsWildcardOrigin,tokenMethods,requireVerification,authDisabled,defaultSuperadminCredentials,cookieSecure,cookieSameSite,settingsStoreFieldCount.customizedStrategies: dotted config paths (e.g.orderOptions.guestCheckoutStrategy) of single-strategy fields whose live class differs fromdefaultConfig— compared dynamically, so defaults stay the single source of truth.Privacy
No raw values are collected anywhere: no hostnames, CORS origins, API paths, secrets, credentials, DB details, filesystem paths, or custom strategy class names.
defaultSuperadminCredentialsis a boolean comparison against the well-known defaults; the configured values are never read into the payload. Entity counts remain range-bucketed. Everything sensitive reduces to booleans, short enums, counts, or config paths.All new payload fields are optional on the receiving side, so this remains backward/forward compatible with the collection endpoint.
Testing
defaultSuperadminCredentials,customizedStrategies(default vs customized), order-metric bucketing andbyType, i18n counting,multiCurrency, and heartbeat scheduling (second send after 24h, interval unref'ed, cleared on shutdown, not scheduled when telemetry is disabled).tsc --noEmitclean; eslint/prettier clean on all changed files.count(where), query-buildergroupBy) — no raw SQL.Breaking changes
None. Telemetry remains opt-out via
VENDURE_DISABLE_TELEMETRYand disabled in CI; the only behavioral change is the added daily heartbeat send.Checklist
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit