-
Notifications
You must be signed in to change notification settings - Fork 492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MDC handle legacy counts #6543
MDC handle legacy counts #6543
Conversation
Thanks @qqmyers for this PR. I'm holding this from review/QA a few days so that I can discuss with Merce and others about prioritizing a migration of old counts to new counts. Hopefully we can figure out this piece and just display one number. |
@qqmyers and others who are interested, a bit of an update... We're still discussing how to best handle the old counts and have reached out to MDC and received feedback. Thanks @pdurbin @TaniaSchlatter and @kcondon for discussing this after standup today. I think our first pass of this will be to get a PoC for an API that migrates (maybe not the best term for this) the old counts to the tables/logs for the new counts. Thanks @pdurbin for offering to create an issue for this. The goal remains to show one number on the page, so if we're able to do that satisfactorily, we can revisit the need for this PR. |
@djbrooke - is the ~migration going to be done via log file mining? Or is this something that can be done solely with info already in the DB. I ask because I don't think QDR has all logs, but if there's a way to generate one number that has a clear meaning given what we do have, that's probably preferable for QDR as well. In the meantime, after discussion at QDR, I'm going to modify the code/strings slightly to be a little more clear. Depending on the requirements for ~migrating, we may then go ahead and deploy this or wait for more info. No need to review/merge if others won't need it. FWIW, we did discuss making details visible in a popup, but unless the main 'total' number includes the pre-MDC counts somehow, this won't be enough. |
@qqmyers I believe what we're proposing will only use information already in the DB, but definitely keep an eye out for the new issue from @pdurbin for more details. The goal remains that we can include pre-MDC counts and MDC counts in a single number and provide some explanation in a tooltip/popover. |
Here's the new issue but for now it's just an image. We're in sprint planning now but after I'll beef up the description: IQSS/dataverse.harvard.edu#75 |
Conflicts: src/main/java/propertyFiles/Bundle.properties
Conflicts: src/main/java/propertyFiles/Bundle.properties
Thanks @qqmyers for the update, I'll close it for now. We're also hopeful for the single number solution in IQSS/dataverse.harvard.edu#75 :) |
I've updated the branch (only 8854 commits!) and am reopening so it can be looked at for IQSS/dataverse-pm#8. I did not run the updated version - the main conflict point was in some css in the MDC metrics block so that could be off. |
At standup we decided we'd like to give this PR a try: IQSS/dataverse.harvard.edu#75 (comment) |
This has been merged with develop. @pdurbin - not sure where it should go on the board(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a quick look and left a comment.
We gave it a 10 today at standup. It's a small code change but testing will take some time. We put it on the main board under Sprint Ready.
I'd like to remind the collective consciousness that @donsizemore implemented support for Counter Processor in https://github.com/GlobalDataverseCommunityConsortium/dataverse-ansible which might help make testing easier.
MDCLogPath, | ||
DisplayMDCMetrics, | ||
MDCStartDate, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can :MDCStartDate
be changed from a database setting to MPCONFIG?
And can we please have a release note about the new setting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a quick release note (via a PR, since I don't have a write on the branch directly).
Other than that it's ready to go.
release note
What this PR does / why we need it:
This PR adds 'legacy' download counts prior to Make Data Count being enabled to the metrics display and adds some messages helping to explain why, for example, the file download counts may not match the MDC downloads.
Which issue(s) this PR closes:
Closes #6542
Special notes for your reviewer:
Set the :MDCStartDate setting to the day MDC logging was enabled , e.g. '2020-12-31'
MDC logging and display also need to be enabled.
Suggestions on how to test this:
Datasets with downloads prior to MDC should show the legacy counts, others should not. See issue for images.
Does this PR introduce a user interface change?:
Yes - see issue #6542 for images/description
Is there a release notes update needed for this change?:
Probably should note that MDCStartDate should be set (default is not set which leaves legacy counting off).
Additional documentation:
If this looks like something that will be merged, I can add docs/release notes. For now, just wanted to get the changes out.