Repository navigation
Conversation
Because: * Data Engineering finds FxA account deletions by parsing auth-server logs (fxa_delete_events), which is fragile and blocks decommissioning Amplitude * Glean server telemetry now supports a server-deletion-request ping that the pipeline can consume directly (DENG-4079) This commit: * Sends account.user_id and account.user_id_sha256 in the built-in server-deletion-request ping and regenerates server_events.ts * Submits the ping wherever DB.deleteAccount and accountDeleted.byCloudTask are logged * Keeps the existing deletion logs for a parallel validation period
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It touches account-deletion behavior and server-side telemetry emission in fxa-auth-server, which warrants final human review despite good test coverage.
1 open finding
What changed in this PR
Adds Glean support for FxA account deletions by emitting the built-in server-deletion-request ping from fxa-auth-server, enabling downstream deletion workflows to rely on telemetry rather than log parsing.
Changes:
- Add
account.user_idandaccount.user_id_sha256to theserver-deletion-requestping and enable those metrics to be sent in that ping. - Introduce a
createServerDeletionRequestPinghelper and call it fromDB.deleteAccountand the cloud-task deletion path when no DB row was deleted. - Regenerate
server_events.tsand add unit tests validating ping submission behavior.
| File | Description |
|---|---|
| packages/fxa-shared/metrics/glean/fxa-backend-pings.yaml | Update comment to reflect use of server-deletion-request built-in ping. |
| packages/fxa-shared/metrics/glean/fxa-backend-metrics.yaml | Allow account ID metrics to be sent in server-deletion-request. |
| packages/fxa-auth-server/lib/metrics/glean/server-deletion-request.ts | Add helper to emit server-deletion-request ping with uid + sha256. |
| packages/fxa-auth-server/lib/metrics/glean/server-deletion-request.spec.ts | Add unit tests for deletion-request ping submission/no-op behavior. |
| packages/fxa-auth-server/lib/metrics/glean/server_events.ts | Add generated logger for server-deletion-request ping and factory. |
| packages/fxa-auth-server/lib/metrics/glean/index.ts | Reuse sha256HashUid helper (dedupe hashing implementation). |
| packages/fxa-auth-server/lib/db.ts | Emit deletion-request ping from the central account deletion DB method. |
| packages/fxa-auth-server/lib/db.spec.ts | Test that db.deleteAccount submits the deletion-request ping. |
| packages/fxa-auth-server/lib/account-delete.ts | Emit ping for cloud-task deletions when the account row is already gone. |
| packages/fxa-auth-server/lib/account-delete.spec.ts | Test cloud-task deletion-request ping behavior and non-duplication. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+7201
to
+7202
| * @returns {EventsServerEventLogger} An instance of EventsServerEventLogger. | ||
| */ |
Member
Author
There was a problem hiding this comment.
This is generated by glean_parser, will fix upstream
This branch has not been deployed
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
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 commit
Issue that this pull request solves
Closes: DENG-11729
Checklist
Put an
xin the boxes that apply