Skip to content
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

DIS 250: Fixed Usage Tables Not Aggregating Data Properly #2212

Closed

Conversation

LeoStoyanovByWater
Copy link
Contributor

  • Added a unique composite index on (userAgentId, year, month, instance) for usage_by_user_agent.
  • Added $usageByUserAgent->find(true) before incrementing request counters.
  • Added $usageByIPAddress->find(true) in various files before DB updates.
  • Added update call in bootstrap.php to update IP Address request counter.

To test:

  1. Open Aspen in a browser or local environment. Click around the site to accumulate user requests.
  2. Query the tables to see that before the patch, numerous rows would be created for the same user/IP.
  3. Once this patch is applied, the same (user/IP, month, year) row as before should change (e.g., accumulate requests).

Copy link
Member

@mdnoble73 mdnoble73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the inline updates requested, this will also need a function in the database updates to aggregate all the existing rows prior to making the unique key

@@ -117,6 +117,8 @@ protected function forbidAPIAccess()
try{
$usageByIPAddress->numBlockedApiRequests++;
if (SystemVariables::getSystemVariables()->trackIpAddresses) {
// This will match that specific IP/year/month/instance row.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LeoStoyanovByWater it looks like this is using incorrect indenting. Can you correct to use tabs?

@@ -117,6 +117,8 @@ protected function forbidAPIAccess()
try{
$usageByIPAddress->numBlockedApiRequests++;
if (SystemVariables::getSystemVariables()->trackIpAddresses) {
// This will match that specific IP/year/month/instance row.
$usageByIPAddress->find(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LeoStoyanovByWater this search will need to be done before the number of blocked requests is incremented

@@ -5732,7 +5732,7 @@ CREATE TABLE `usage_by_user_agent` (
`numRequests` int(11) NOT NULL DEFAULT 0,
`numBlockedRequests` int(11) NOT NULL DEFAULT 0,
PRIMARY KEY (`id`),
KEY `userAgentId` (`userAgentId`,`year`,`instance`,`month`)
UNIQUE KEY `userAgentId` (`userAgentId`,`year`,`instance`,`month`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes need to be done with a database update rather than by simply modifying the default database

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants