Conversation
There was a problem hiding this comment.
The pr that updates accountAuthorizations has several migrations so this is going to hit conflicts 🥴
Because: - FxA lacks a durable per-(userId, clientId, service) activity signal needed for inactive-account deletion, per-RP liveness, and support tooling. - Existing token tables either disappear on revoke, miss Firefox Desktop's online fxa-credentials service-token mints, or are sampled and Redis-biased and therefore under-detect active users. This commit: - Adds a new accountActivity table to the fxa_oauth schema (patch 33->34), keyed by (userId, clientId, service) with secondary indexes for time-window and per-service cohort queries. - Adds a fire-and-forget recordActivity helper that resolves the service tag from the requested scopes (scopeToServiceName) with a clientId fallback (oauthServer.clientIdToServiceNames), then writes via a SQL-throttled upsert. The throttle predicate uses `VALUES(x) > x + ?` to avoid BIGINT UNSIGNED underflow on out-of-order timestamps. - Wires a single write site at the OAuth grant convergence point in tokenHandler so all four grant types (authorization_code, fxa-credentials, refresh_token, token-exchange) are recorded uniformly. - Extends _removeTokensAndCodes so accountActivity rows are cleaned up alongside tokens on account deletion. - Stores scopeToServiceName as a JSON string in convict to work around mozilla/node-convict#250 (dotted/URL keys would otherwise be split as nested config paths during validation). - Emits accountActivity.recorded / accountActivity.write_failed StatsD metrics with service + grantType tags. Unclassified ('') rows trigger a deduped log.warn once per process per clientId. Closes FXA-13662
| throttleMs | ||
| ); | ||
| statsd?.increment('accountActivity.recorded', metricTags); | ||
| if (service === '' && !warnedUnclassifiedClientIds.has(clientIdHex)) { |
There was a problem hiding this comment.
On the surface this makes sense to record an empty unclassified service scopes as an empty string '', BUT, I also wonder if there is risk in doing this and we should just drop the empty values. Rather, if there are unclassified scopes, we scrub them from being written at all AND log some kind of error to sentry with the "wrong" scope, and/or a statsd metric to track. It means we'd miss a 'sign in' potentially, but by recording an empty scope we're also missing what they signed into anyways 🤷
| statsd?.increment('accountActivity.recorded', metricTags); | ||
| if (service === '' && !warnedUnclassifiedClientIds.has(clientIdHex)) { | ||
| warnedUnclassifiedClientIds.add(clientIdHex); | ||
| log?.warn('accountActivity.unclassified', { |
There was a problem hiding this comment.
I wonder if the log here should be a Sentry alert instead? If we have a mismatching scopeToServiceName or clientidToServicenames then we need to know that and logs aren't great
Because:
This commit:
oauthServer.clientIdToServiceNames), then writes via a SQL-throttled upsert. The throttle predicate usesVALUES(x) > x + ?to avoid BIGINT UNSIGNED underflow on out-of-order timestamps._removeTokensAndCodessoaccountActivityrows are cleaned up alongside tokens on account deletion.accountActivity.recorded/accountActivity.write_failedStatsD metrics with service + grantType tags. Unclassified ('') rows trigger a dedupedlog.warnonce per process per clientId.Closes FXA-13662
Put an
xin the boxes that applyHow to review (Optional)
Screenshots (Optional)
Here's a screenshot of the table after I signed in/up through 123Done, then signed in with

scopeKeysOther information (Optional)
Any other information that is important to this pull request.