fixes for get api including MDC metrics - #12741
stevenwinship wants to merge 4 commits into
Conversation
stevenwinship
added
FY26 Sprint 7
|
coverage: 25.408% (-0.003%) from 25.411% — 491-fix-mdc-api-for-modern-ui into develop |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1b90af6 to
76ef0a0
Compare
|
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
| job.add("MDC", JsonPrinter.json(metrics)); | ||
| } | ||
| // total download count from guestbookResponseService and datasetMetricsService(if included) | ||
| job.add("downloadCount", count + mdcCount); |
There was a problem hiding this comment.
There is a total double-counts downloads after MDC processing.
With includeMDC=true, count contains all guestbook downloads, including those after MDCStartDate. Adding mdcCount counts events present in both sources twice.
Steps to reproduce
- Create a new dataset with a downloadable file and zero downloads.
- Set
:MDCStartDateto today, enable logging with:MDCLogPath, and set:DisplayMDCMetrics=true. - Download the file once using a non-admin user.
- Query:
GET /api/datasets/{id}/download/count?includeMDC=true
The response contains:{ "downloadCount": 1, "MDC": { "downloadCount": 0 } } - Run Counter Processor (with
simulate_dateset to tomorrow) and import the report withaddUsageMetricsFromSushiReport. - Without downloading again, Query:
GET /api/datasets/{id}/download/count?includeMDC=true{ "downloadCount": 2, "MDC": { "downloadCount": 1 } }should be shown - Request
includeMDC=false. It returns 0 pre-MDC downloads.
Actual: One download happened, but the API reports 2 after MDC processing. The documented subtraction also gives 2 − 1 = 1, although pre-MDC downloads are 0.
Expected: 0 pre-MDC + 1 MDC = 1 total download.
| Long mdcCount = 0L; | ||
| boolean includeMDCResponse = Boolean.TRUE.equals(includeMDC); | ||
| // Setting `includeMDC` to True will ignore the `:MDCStartDate` setting and return a total count | ||
| LocalDate date = includeMDCResponse ? null : getMDCStartDate(); |
There was a problem hiding this comment.
LocalDate date = getMDCStartDate(); so the guestbook and MDC each cover a different period, with :MDCStartDate as the boundary.
| LocalDate date = includeMDC == null || !includeMDC ? getMDCStartDate() : null; | ||
| Long mdcCount = 0L; | ||
| boolean includeMDCResponse = Boolean.TRUE.equals(includeMDC); | ||
| // Setting `includeMDC` to True will ignore the `:MDCStartDate` setting and return a total count |
There was a problem hiding this comment.
Guestbook responses are only counted before :MDCStartDate; after that date, downloads come from MDC metrics. With includeMDC the total is the pre-MDC guestbook count + the MDC count, so no download is counted twice.
This branch has not been deployed
3 participants
What this PR does / why we need it:Modern UI is always getting the old metrics regardless of the DisplayMDCMetrics setting. JSF UI displays based on the setting. The response to /api/datasets/{id}/download/count?includeMDC=true or false should return the requested metrics where not including the parameter 'includeMDC' should base the output on the setting.
Which issue(s) this PR closes: IQSS/dataverse.harvard.edu#491
Special notes for your reviewer:The old code was showing MDC date when includeMDC was false. The Json response still contains the 'downloadCount' so there is no break in backward compatibility.
Suggestions on how to test this: Test with multiple variations of settings and api parameter. Downloading will increment the old metrics but not MDC. MDC needs the logs to be loaded. PSQL inserts to 'datasetmetrics' for MDC.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?: included
Additional documentation: