fix(jans-fido2): measure user-adoption metrics against the right population - #14860
Draft
imran-ishaq wants to merge 1 commit into
Draft
fix(jans-fido2): measure user-adoption metrics against the right population#14860imran-ishaq wants to merge 1 commit into
imran-ishaq wants to merge 1 commit into
Conversation
…lation Signed-off-by: imran <imranishaq7071@gmail.com>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
imran-ishaq
had a problem deploying
to
integration-tests
August 25, 2026 12:59 — with
GitHub Actions
Failure
imran-ishaq
had a problem deploying
to
integration-tests
August 25, 2026 12:59 — with
GitHub Actions
Failure
Member
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
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.




Prepare
Description
Target issue
closes #14830
Implementation Details
Fido2MetricsService.getUserAdoptionMetricsreported three figures that did not mean what theirnames said:
adoptionRatefell as adoption succeeded. It wasnewUsers / uniqueUsers, whereuniqueUserscounted only users active in the query window. Once everyone had enrolled and was only signing in,
newUserstended to zero and the rate reported near-zero adoption exactly when adoption wascomplete.
newUsersdid not mean first registration. The filter wasREGISTRATION+SUCCESSinside thewindow, with no check for prior registrations — an existing user enrolling a second passkey counted
as new, and the number changed meaning with the date picker.
returningUserswas derived by subtraction (uniqueUsers - newUsers), so a user who bothregistered and authenticated in the same window was counted only as new, never as returning.
The fix.
getUserAdoptionMetricsnow issues a second, targeted query against the metrics store —getUsersRegisteredBefore(startTime)— for users whose registration already succeeded before thewindow began ("prior adopters"). Everything downstream is derived from that set directly rather than
from window-only activity:
newUsers= registrations succeeding in the window, minus prior adopters — first-ever success only.returningUsers= users active this window who are already prior adopters — computed directly, notby subtracting
newUsersfromuniqueUsers.adoptionRate=newUsers / (priorAdopters + newUsers)— new users against the cumulativepopulation of everyone who has ever registered as of
endTime, so it tracks growth instead offalling toward zero as sign-in-only activity comes to dominate. When nobody has ever registered, the
rate is
nullrather than a misleading0.0.This is intentionally the self-contained option: no new dependency on a directory-wide user count, and
no response-shape drop of
adoptionRate— see the issue's "needs a product decision" note for the twoalternatives considered. It is bounded by the metrics retention policy: a user whose only prior
registration entry has already been cleaned up by
cleanupOldDatais reported as new again. Thattradeoff is documented on
getUsersRegisteredBefore's Javadoc.Unrelated bug found while implementing this, filed separately as #14859:
Fido2AnalyticsService. generateExecutiveSummaryrecomputes its own adoption rate fromtotalUniqueUsers/newUsersinsteadof using this method's
adoptionRate, and casts thoseIntegervalues toLong, throwingClassCastException. Not touched here — it's a different file with its own review, and the class iscurrently unwired from any controller.
Test and Document the changes
Tests —
Fido2MetricsServiceTest, 5 added (34 total in the file, all green):adoptionRateis against cumulative adopters, not window activityadoptionRateisnull, not0.0, when nobody has ever registeredPlease check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.