follow-ups (fxa-13811): #20695
Open
groovecoder wants to merge 4 commits into
Open
Conversation
… to writer
Because:
- Review feedback: flush errors were logged but not metricked, so
alerting on BQ outages was not possible.
This commit:
- Adds optional StatsD param to RateLimitBqWriter constructor.
- Emits rate_limit.bq_writer.flush_error on insert failures.
- Passes statsd through from key_server.js and NestJS provider.
- Renames describe('bqWriter integration') to describe('bqWriter')
since the writer is mocked.
Fixes FXA-13811
Because: - The rate_limit_checks table must exist before inserts work, but BQ tables can't be created via the normal SQL migration path. Self-creation keeps the schema in code and avoids SRE tickets for every schema change. This commit: - Exports RATE_LIMIT_CHECK_SCHEMA matching RateLimitCheckEvent fields. - Adds ensureTable() that checks existence and creates the table on first startup. Errors are caught — missing permissions degrade to the same insert-time failure as before. - Waits for table readiness before first flush. - Adds tests for table creation, skip-when-exists, and error handling. Fixes FXA-13811
dschom
reviewed
Jun 10, 2026
dschom
reviewed
Jun 10, 2026
| try { | ||
| const [exists] = await this.tableRef.exists(); | ||
| if (!exists) { | ||
| await this.tableRef.create({ schema: RATE_LIMIT_CHECK_SCHEMA }); |
Contributor
There was a problem hiding this comment.
I'm not sure this will actually work. The credentials given to the service probably shouldn't have permissions to do this.
Member
Author
There was a problem hiding this comment.
The ensureTable catch block already handles this. I updated the comment to explicitly note the service account may lack bigquery.tables.create. If it does fail, Sentry captures the error and inserts proceed (they'll fail with a clear error if the table truly doesn't exist).
But should we have SRE pre-create the table and remove ensureTable entirely?
Contributor
There was a problem hiding this comment.
Yes. I think the better approach would be to just have SRE do this. We can still fail, and capture insert failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Because
This pull request
rate_limit.bq_writer.flush_erroron insert failures.rate_limit_checksBigQuery table on startup viaensureTable()withRATE_LIMIT_CHECK_SCHEMA. (Non-fatal if permissions don't allow creation.)Issue that this pull request solves
Closes: FXA-13811
Checklist
Put an
xin the boxes that applyI have added necessary documentation (if appropriate).I have verified that my changes render correctly in RTL (if appropriate).Other information (Optional)
RATE_LIMIT__BIGQUERY__ENABLED=false).