- 
                Notifications
    You must be signed in to change notification settings 
- Fork 574
fix(record count): do not count records from deleted connections #4919
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
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 | 
|---|---|---|
|  | @@ -4,7 +4,7 @@ import * as cron from 'node-cron'; | |
| import { billing as usageBilling } from '@nangohq/billing'; | ||
| import { getLocking } from '@nangohq/kvstore'; | ||
| import { records } from '@nangohq/records'; | ||
| import { connectionService, environmentService } from '@nangohq/shared'; | ||
| import { connectionService } from '@nangohq/shared'; | ||
| import { flagHasUsage, getLogger, metrics } from '@nangohq/utils'; | ||
|  | ||
| import { envs } from '../env.js'; | ||
|  | @@ -95,31 +95,43 @@ const observability = { | |
| exportRecordsMetrics: async (): Promise<void> => { | ||
| await tracer.trace<Promise<void>>('nango.cron.exportUsage.observability.records', async (span) => { | ||
| try { | ||
| const res = await records.metrics(); | ||
| if (res.isErr()) { | ||
| throw res.error; | ||
| } | ||
|  | ||
| // records metrics are per environment, we need to get the account ids | ||
| const envIds = res.value.map((r) => r.environmentId); | ||
| const envs = await environmentService.getEnvironmentsByIds(envIds); | ||
| const metricsWithAccount = res.value.flatMap(({ environmentId, count, sizeBytes }) => { | ||
| const env = envs.find((e) => e.id === environmentId); | ||
| if (!env) { | ||
| return []; | ||
| const metricsByAccount = new Map<number, { count: number; sizeBytes: number }>(); | ||
| // records metrics are per environment, so we fetch the record counts first and then we need to: | ||
| // - get the account ids | ||
| // - filter out connections that have been deleted | ||
| // - aggregate per account | ||
| // | ||
| // Performance note: This nested pagination approach is necessary because records and connections | ||
| // are stored in separate databases, making SQL JOINs impossible. | ||
| // To reconsider when record counts table becomes very large (currently <10k entries) | ||
| for await (const recordCounts of records.paginateRecordCounts()) { | ||
| if (recordCounts.isErr()) { | ||
| throw recordCounts.error; | ||
| } | ||
| if (recordCounts.value.length === 0) { | ||
| continue; | ||
| } | ||
| return [{ environmentId, accountId: env.account_id, count, sizeBytes }]; | ||
| }); | ||
|  | ||
| // Group by account | ||
| const metricsByAccount = new Map<number, { count: number; sizeBytes: number }>(); | ||
| for (const metric of metricsWithAccount) { | ||
| const existing = metricsByAccount.get(metric.accountId); | ||
| if (existing) { | ||
| existing.count += metric.count; | ||
| existing.sizeBytes += metric.sizeBytes; | ||
| } else { | ||
| metricsByAccount.set(metric.accountId, { count: metric.count, sizeBytes: metric.sizeBytes }); | ||
| const connectionIds = recordCounts.value.map((r) => r.connection_id); | ||
| for await (const res of connectionService.paginateConnections({ connectionIds })) { | ||
| if (res.isErr()) { | ||
| throw res.error; | ||
| } | ||
| for (const value of res.value) { | ||
| const recordCount = recordCounts.value.find((r) => r.connection_id === value.connection.id); | ||
| 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. Potentially avoidable o(n^2), but probably negligible compared to queries? 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. This entire analytics logic only works because the scale is small. We are gonna need to come up with a better solution as our user base of customers saving records expands | ||
| if (recordCount) { | ||
| const existing = metricsByAccount.get(value.account.id); | ||
| if (existing) { | ||
| existing.count += recordCount.count; | ||
| existing.sizeBytes += recordCount.size_bytes; | ||
| } else { | ||
| metricsByAccount.set(value.account.id, { | ||
| count: recordCount.count, | ||
| sizeBytes: recordCount.size_bytes | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|  | ||
|  | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated. I just noticed we were logging the error twice. Here and in the catch