Conversation
| reason: | ||
| description: | | ||
| Why the envelope was withheld. One of `not_bound` (the MFA token was | ||
| minted for a different credential), `stale` (the envelope seals a kB |
There was a problem hiding this comment.
Since we've been talking about it - @vpomerleau this includes the stale which is currently withheld. But if we pivot on that then this will also need to be updated. Just noting for so two of us are aware 😃
There was a problem hiding this comment.
🟡 Changes recommended
Metric descriptions need correction and generated bindings must be refreshed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds attributed Glean telemetry for passkey wrap creation and retrieval.
Changes:
- Defines wrap completion and failure events with categorized reasons.
- Emits telemetry from passkey wrap routes.
- Adds route tests and generated Glean bindings.
Review comments:
- Correct misleading metric descriptions that label successful events as denominators.
- Use “fallback” rather than “fall back” and regenerate
server_events.ts. - Document that
not_boundcovers both missing and mismatched credential bindings.
File summaries
| File | Description |
|---|---|
packages/fxa-shared/metrics/glean/fxa-backend-metrics.yaml |
Defines wrap lifecycle metrics. |
packages/fxa-auth-server/lib/routes/passkey-wraps.ts |
Emits wrap success and failure events. |
packages/fxa-auth-server/lib/routes/passkey-wraps.spec.ts |
Tests event emission and reason mapping. |
packages/fxa-auth-server/lib/routes/index.js |
Injects Glean into wrap routes. |
packages/fxa-auth-server/lib/metrics/glean/server_events.ts |
Adds generated event loggers. |
packages/fxa-auth-server/lib/metrics/glean/index.ts |
Exposes wrap event functions. |
packages/fxa-auth-server/lib/metrics/glean/index.spec.ts |
Adds generated logger mocks. |
Review details
Suppressed comments (3)
packages/fxa-shared/metrics/glean/fxa-backend-metrics.yaml:1927
- The documented reason taxonomy does not match the route: a missing credential binding is also emitted as
not_bound, and any unrecognized error is emitted asunexpected. Omitting these meanings makes valid telemetry appear outside the metric contract.
Why the envelope was withheld. One of `not_bound` (the MFA token was
minted for a different credential), `stale` (the envelope seals a kB
the account has since replaced, so the passkey needs re-enrolling),
`wrap_not_found` (the passkey was never given an envelope), or
`credential_not_found`.
packages/fxa-shared/metrics/glean/fxa-backend-metrics.yaml:1905
- Use the noun “fallback” here; “fall back” is the verb form. Regenerate
server_events.tsafter correcting the source metric description.
for the user, in a silent fall back to signing in with a password, so the
packages/fxa-shared/metrics/glean/fxa-backend-metrics.yaml:1880
- The documented
not_boundmeaning excludes the missing-cidcase, butisBoundToreturns false for both a missing binding and a mismatched credential, and both are recorded with this reason. Document both cases so analyses do not interpret every event as a cross-credential mismatch.
Why the write was refused. One of `not_bound` (the MFA token was
minted for a different credential), `conflict` (a different envelope
is already stored), `credential_not_found`, or `unexpected`.
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Because:
- Wrap operations have statsd counters but no attributed metrics, so
enrolment and passwordless sign-in cannot be measured per service,
flow or user.
- A passkey whose wrap never stored can never unseal Sync keys, and
nothing today shows how often that happens.
This commit:
- Adds passkey.wrap_create_complete and wrap_create_failure to
POST /passkey/wraps.
- Adds passkey.wrap_get_complete and wrap_get_failure to
GET /passkey/wraps/{credentialId}.
- Reports a reason of not_bound, conflict, stale, wrap_not_found,
credential_not_found or unexpected on the failure events.
- Threads glean into passkeyWrapsRoutes.
Closes #FXA-14156
Because:
This commit:
Closes #FXA-14156
Checklist
Put an
xin the boxes that applyHow to review (Optional)
Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Any other information that is important to this pull request.