JanssenProject / JanssenProject/jans
fix(jans-fido2): metrics aggregation counts every entry row as an attempt, inflating failure counts
- Dominant language
- Java
- Stars
- 648
- Forks
- 174
- Avg merge
- 1d 18h
- Merged PRs (30d)
- 110
Description
**Is your feature request related to a problem? Please describe.**
`Fido2MetricsService.calculateAggregation` counts **every** row for an operation type as an attempt, regardless of status, then derives failures by subtraction:
```java
long registrationAttempts = entries.stream()
.filter(e -> Fido2MetricsConstants.REGISTRATION.equals(e.getOperationType()))
.count(); // Fido2MetricsService.java:608-610
long registrationFailures = registrationAttempts - registrationSuccesses; // :617
```
(identical logic for authentication at `:628-637`)
A single successful ceremony writes **two** rows — `ATTEMPT` at `/options` and `SUCCESS` at `/result`. So every success is counted as one attempt, one success, *and* one phantom failure. The persisted `jansFido2MetricsAggregation` rows are wrong as a result.
Measured against a live sample (27 Jul 2026), where the entry table holds 31 registration `ATTEMPT` + 7 registration `SUCCESS`, and 19 authentication `ATTEMPT` + 4 authentication `SUCCESS`:
| Metric | Stored in aggregation | Actual |
|---|---|---|
| registrationAttempts | 38 | 31 |
| registrationFailures | 31 | 0 recorded (24 never completed) |
| registrationSuccessRate | 0.184 | 0.226 |
| authenticationAttempts | 23 | 19 |
| authenticationFailures | 19 | 0 recorded (15 never completed) |
| authenticationSuccessRate | 0.231 | 0.211 |
This is also internally inconsistent: two other methods in the same class already do it correctly — `getErrorAnalysis` tallies `ATTEMPT` / `SUCCESS` / `FAILURE` as distinct statuses (`:494-507`), and `getAttestationRejectionAnalysis` filters attempts on `ATTEMPT` status (`:574-577`). Only `calculateAggregation` does not.
**Describe the solution you'd like**
In `calculateAggregation`, for both registration and authentication:
1. Count attempts as rows with status `ATTEMPT` only, matching `getAttestationRejectionAnalysis`.
2. Count failures as actual `FAILURE` rows rather than deriving them by subtraction.
3. Report ceremonies that started but never completed as their own figure (e.g. `abandoned` / drop-off) instead of folding them into `failures`.
4. Keep success rate as `successes / attempts` — it becomes correct automatically once (1) lands.
Existing `jansFido2MetricsAggregation` rows were computed with the old arithmetic and will stay wrong; recomputation or a note in the release notes is needed.
**Describe alternatives you've considered**
- *Keep subtraction but exclude `SUCCESS`/`FAILURE` rows from the attempt count.* Fixes the double-count, but still labels never-completed ceremonies as failures — misleading in exactly the way the sample above shows.
- *Stop writing the `ATTEMPT` row and only record terminal outcomes.* Would make subtraction valid, but loses drop-off measurement entirely, which is the more useful signal.
- *Fix only at the read/API layer and leave the stored aggregation as-is.* Rejected — the wrong values are persisted, so any direct DB consumer still gets bad data.
**Additional context**
Minor related observation, no action assumed: `deviceTypes` groups on `jansFido2MetricsAuthenticatorType`, which is only populated on `SUCCESS`/`FAILURE` rows (`ATTEMPT` rows pass `null`). In the sample it sums to 11, matching the 11 successes — so it counts completed ceremonies, not attempts. Worth confirming that's intended.
Contributor guide
Assessment
This issue has not been assessed yet.