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

Added aggregate config option to Billing 95th percentile calculations #10202

Merged
merged 4 commits into from May 28, 2019

Conversation

Projects
None yet
5 participants
@llarian0
Copy link
Contributor

commented May 9, 2019

This is very small change but it is required for our environment and I'm hoping to be able to upgrade without continually re-merging these files.

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

Added configuration options to aggregate input and output bits before…
… making 95th percentile billing calculations
@CLAassistant

This comment has been minimized.

Copy link

commented May 9, 2019

CLA assistant check
All committers have signed the CLA.

@@ -123,6 +123,21 @@ function getLastMeasurement($bill_id)
return ($return);
}//end getLastMeasurement()
function get95thagg($bill_id, $datefrom, $dateto)
{
$mq_sql = "SELECT count(delta) FROM bill_data WHERE bill_id = '".mres($bill_id)."'";

This comment has been minimized.

Copy link
@murrant

murrant May 10, 2019

Member

mres() is a do nothing function, please use parameterized queries;
like dbFetchRows('SELECT * FROM devices WHERE device_id=?', [$device_id])

if ($data['rate_95th_out'] > $data['rate_95th_in']) {
$data['rate_95th'] = $data['rate_95th_out'];
$data['dir_95th'] = 'out';
if( $config['billing_aggregate_95th'] == 1 ) {

This comment has been minimized.

Copy link
@murrant

murrant May 10, 2019

Member

We are trying to convert all $config calls to Config::get()

This comment has been minimized.

Copy link
@llarian0

llarian0 May 10, 2019

Author Contributor

That makes sense. I just cargo culted the existing billing module, but I can refactor it to use your current standards easily enough. I'll submit a new pull request when that's done.

This comment has been minimized.

Copy link
@PipoCanaja

PipoCanaja May 11, 2019

Contributor

@llarian0 : Or you can update this one directly :)

This comment has been minimized.

Copy link
@llarian0

llarian0 May 11, 2019

Author Contributor

Ah, thanks. New-ish to public github stuff.

@murrant

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Overall looks fine to me, but shouldn't this be a per-bill option?

@murrant

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Should this be a per-bill option? is more important than converting to Config. Any response to that?

@llarian0

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Should this be a per-bill option? is more important than converting to Config. Any response to that?

I'm sorry, I thought I responded to that. Yes, it probably should be. I'd guess most environments are all or nothing, but for inclusion in upstream per-bill makes more sense. The 95th percentile direction column could probably be used for that, if its set to "agg" the poller combines the values, otherwise is uses the existing behavior of setting the direction to in/out and using the max value for the billing cycle. I'll take a look at that a little more closely later this week to see if there are any pitfalls there, but that saves adding another DB column that probably isn't necessary.

@murrant

This comment has been minimized.

Copy link
Member

commented May 13, 2019

That sounds like a good approach. Funny you can't use "both" because it is varchar(3)...

@llarian0

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

That sounds like a good approach. Funny you can't use "both" because it is varchar(3)...

Yeah, I noticed that. I considered "tot" instead of "agg"

@murrant

This comment has been minimized.

Copy link
Member

commented May 13, 2019

agg is fine, you can show whatever in the web ui ;)

llarian0 added some commits May 17, 2019

Changed aggregate to per-bill instead of global. Added config options…
… for making aggregate the default selected option. Refactored out mres() calls in touched files. Changed to Config::get where appropriate.
@murrant

This comment has been minimized.

Copy link
Member

commented May 19, 2019

Looks good, thank for fixing all the SQL injection issues ;) You can use [] instead of array(), we plan to convert them all at some point. There was still a function naming inconsistency (pre-existing).

This isn't that easy for me to test, but I will try.

add the following the `config.php`:

```php
$config['billing']['95th_default_agg'] = 1; // Set aggregate 95th as default

This comment has been minimized.

Copy link
@murrant

murrant May 23, 2019

Member

I think it would be better to allow all three types to be set as the default in the config.
$config['billing']['95th_default'] = 'agg';

This comment has been minimized.

Copy link
@llarian0

llarian0 May 23, 2019

Author Contributor

There's only really 2 types. Max of in/out, or aggregate total.

The existing codebase sets direction to in or out based on the highest value, so I'm setting that field to 'agg' (which obviously doesn't change) to override that behavior.

For the standard functionality, a new bill gets set to 'in' by default and then the billing-calculate script changes it based on real values once there's sufficient data to set a max direction for that billing cycle.

That said, the config knob you suggest would probably make the config more readable, so its not a bad idea. That'd be a pretty simple change

This comment has been minimized.

Copy link
@murrant

murrant May 24, 2019

Member

Sounds good, yeah, somehow there only be two slipped my mind when I wrote this. The in/out makes sense as you don't know which direction is "Down" or "Up".

@murrant murrant merged commit 9c837be into librenms:master May 28, 2019

4 of 6 checks passed

Travis CI - Pull Request Build Failed
Details
codeclimate 17 issues to fix
Details
Inspection Summary
Details
Node: analysis
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
@mctaguma

This comment has been minimized.

Copy link

commented May 29, 2019

Hi all,

I've found a problem in your code, causing Traffic Bill graphs to fail to load, with the following error (and stack trace):
[2019-05-2911:48:13] production.ERROR: Too few arguments to function getRates(), 3 passed in /opt/librenms/includes/html/graphs/bill/bits.inc.php on line 6 and exactly 4 expected {"userId":6,"email":"user.email@domain.com","exception":"[object] (Symfony\\Component\\Debug\\Exception\\FatalThrowableError(code: 0): Too few arguments to function getRates(), 3 passed in /opt/librenms/includes/html/graphs/bill/bits.inc.php on line 6 and exactly 4 expected at /opt/librenms/includes/billing.php:169) [stacktrace] #0 /opt/librenms/includes/html/graphs/bill/bits.inc.php(6): getRates('8', '20190528114500', '20190529114500') #1 /opt/librenms/includes/html/graphs/graph.inc.php(48): include('/opt/librenms/i...') #2 /opt/librenms/html/graph.php(29): require('/opt/librenms/i...') #3 {main}"}

This error is caused by https://github.com/librenms/librenms/blame/0ae1f30f7dbcadd3e97002e5bdaa9215a3eaa7d4/includes/billing.php#L169.

It would appear that "function getRates($bill_id, $datefrom, $dateto)" was changed to "function getRates($bill_id, $datefrom, $dateto, $dir_95th)", without updating "$rates = getRates($vars['id'], $datefrom, $dateto);" at https://github.com/librenms/librenms/blame/master/includes/html/graphs/bill/bits.inc.php#L6.

Can someone possibly fix this? Thanks!

@llarian0

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Hi all,

I've found a problem in your code, causing Traffic Bill graphs to fail to load, with the following error (and stack trace):
[2019-05-2911:48:13] production.ERROR: Too few arguments to function getRates(), 3 passed in /opt/librenms/includes/html/graphs/bill/bits.inc.php on line 6 and exactly 4 expected {"userId":6,"email":"user.email@domain.com","exception":"[object] (Symfony\\Component\\Debug\\Exception\\FatalThrowableError(code: 0): Too few arguments to function getRates(), 3 passed in /opt/librenms/includes/html/graphs/bill/bits.inc.php on line 6 and exactly 4 expected at /opt/librenms/includes/billing.php:169) [stacktrace] #0 /opt/librenms/includes/html/graphs/bill/bits.inc.php(6): getRates('8', '20190528114500', '20190529114500') #1 /opt/librenms/includes/html/graphs/graph.inc.php(48): include('/opt/librenms/i...') #2 /opt/librenms/html/graph.php(29): require('/opt/librenms/i...') #3 {main}"}

This error is caused by https://github.com/librenms/librenms/blame/0ae1f30f7dbcadd3e97002e5bdaa9215a3eaa7d4/includes/billing.php#L169.

It would appear that "function getRates($bill_id, $datefrom, $dateto)" was changed to "function getRates($bill_id, $datefrom, $dateto, $dir_95th)", without updating "$rates = getRates($vars['id'], $datefrom, $dateto);" at https://github.com/librenms/librenms/blame/master/includes/html/graphs/bill/bits.inc.php#L6.

Can someone possibly fix this? Thanks!

Thank you for the heads up, I'll get this fixed ASAP.

@mctaguma

This comment has been minimized.

Copy link

commented May 29, 2019

Thanks! Apologies for not suggesting a specific fix, not familiar enough with the code.

@llarian0

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Thanks! Apologies for not suggesting a specific fix, not familiar enough with the code.

Not a problem, I have no idea how I missed that. Fixed in my branch.

@llarian0 llarian0 referenced this pull request May 29, 2019

Merged

Fixed Quick Graphs bug w/ Aggregate 95th code #10269

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.