Fix usage record count #4193
Fix usage record count #4193
Conversation
|
@blueorangutan package |
|
@Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos7 ✔debian. JID-1499 |
|
Hi @Pearl1594 @rhtyd , Thank you for the quick fix. The best would probably be to move the filtering from the ApiResponseHelper to the actual search in UsageServiceImpl, but that could be difficult and/or inefficient. |
|
@blueorangutan package |
|
@Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos7 ✔debian. JID-1511 |
| networkId = ip.getSourceNetworkId(); | ||
| } | ||
| NetworkDetailVO networkDetail = _networkDetailsDao.findDetail(networkId, Network.hideIpAddressUsage); | ||
| if (networkDetail != null && networkDetail.getValue() != null && networkDetail.getValue().equals("true")) { |
There was a problem hiding this comment.
As this code is in a test if (usageId != null), it will remove IP address usage only if the API command listUsageRecords was called with a specific usageId parameter.
For API calls like command=listUsageRecords&startdate=xxx&enddate=xxx, it will include all usage records, including for IP addresses on networks with "Hide usage".
|
@blueorangutan package |
1 similar comment
|
@blueorangutan package |
|
@Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos7 ✔debian. JID-1517 |
|
@blueorangutan test |
|
@Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1964)
|
|
Can you @davidjumani review this? Thanx |
olivierlemasle
left a comment
olivierlemasle
left a comment
There was a problem hiding this comment.
Thanks, LGTM (not tested).
I hope this will not have a bad impact on performances.
There was a problem hiding this comment.
This is still not a full fix for this without impacting the performance; the fix is not fully right as it does not count whether or not the IPs being listed/found existed if the list API was called with a start/end date. The simplest solution would be to simply return the actual total count; the edge case of hiding shared network IP usages could be handled in a separate PR.
|
@rhtyd since startdate and enddate are mandatory fields in the API , the usageRecords fetched from the database will be within the (date) range provided and further processing is then done on that list obtained |
|
Thanks for checking the mandatory params @Pearl1594, you're right with the argument I hadn't considered start/end date as mandatory parameters. In that case, perhaps we can spend some time in future to optimise it, for example, get all the IPs given a list of IPs (unique usage IDs) and then join against the details table to check how many IPs need to be hidden, and run a quick loop to find number of counts we've to reduce if any; the current code may significantly impact the list API response which would require many DB calls. |
|
Hi @Pearl1594 @rhtyd What are the required steps before merging this? |
|
@olivierlemasle It will probably require another round of testing before it can be merged. |
There was a problem hiding this comment.
I've done some manual tests on this PR (actually, on this PR rebased on master). Sadly, it does not work as expected.
These jq filters display the value of "count", then the number of records in the page. They show that this works when there's no pagination:
➜ ~ cmk list usagerecords startdate=2020-07-21 enddate=2020-07-21 | jq ".count, .usagerecord | length"
76
76
➜ ~ cmk list usagerecords startdate=2020-07-21 enddate=2020-07-21 pagesize=100 page=1 | jq ".count, .usagerecord | length"
76
76
But with pagination, here are the results:
➜ ~ cmk list usagerecords startdate=2020-07-21 enddate=2020-07-21 pagesize=20 page=1 | jq ".count, .usagerecord | length"
82
18
➜ ~ cmk list usagerecords startdate=2020-07-21 enddate=2020-07-21 pagesize=20 page=2 | jq ".count, .usagerecord | length"
82
18
➜ ~ cmk list usagerecords startdate=2020-07-21 enddate=2020-07-21 pagesize=20 page=3 | jq ".count, .usagerecord | length"
82
18
➜ ~ cmk list usagerecords startdate=2020-07-21 enddate=2020-07-21 pagesize=20 page=4 | jq ".count, .usagerecord | length"
82
18
➜ ~ cmk list usagerecords startdate=2020-07-21 enddate=2020-07-21 pagesize=20 page=5 | jq ".count, .usagerecord | length"
84
4
As we can see, there is still 76 records (18*4 + 4), which is right. The difference between 18 (usage records in a page) and 20 (pagesize) is the IP addresses on networks with hideIpAddressUsage.
However, "count" is 82 or 84 (depending on the page), instead of 76.
Actually, 84 is the total amount of unfiltered usage records (the original count value). count is decremented to remove the number of usage records for IP addresses on networks with hideIpAddressUsage, but only for the current page...
|
@Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2021 |
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2023 |
|
@blueorangutan package |
|
@Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2036 |
|
@Pearl1594 Any chance merging this PR? |
|
this is a 4.13 based change @rhtyd do we need to push this towards 4.16? |
|
IMHO, this should be backported in 4.13, 4.14 and recent releases, as it fixes a regression in 4.13. |
|
@Pearl1594 Any new on this? |
|
It's good to go @olivierlemasle |
|
@blueorangutan test |
|
@Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-3016)
|
|
@blueorangutan test |
|
@Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-3029)
|
|
Thanks @DaanHoogland @Pearl1594 |
Co-authored-by: Pearl Dsilva <[email protected]>
Co-authored-by: Pearl Dsilva <[email protected]>
6 participants
Description
listUsageRecords returns the count equal to the number of objects returned in the page, rather than the total count
Addresses: #4191
Types of changes