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

feature: stacked graphs #7725

Merged
merged 15 commits into from Dec 2, 2017

Conversation

Projects
None yet
5 participants
@crcro
Copy link
Contributor

crcro commented Nov 14, 2017

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

as per: #7409

before:
screen shot 2017-11-14 at 3 43 01 am

after:
screen shot 2017-11-14 at 3 43 19 am

crcro added some commits Nov 14, 2017

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Nov 14, 2017

Looks great!

You should be using Config instead of a custom function. Like Config::get('graphs.port.stacked').
You should allow this to be set in the webui, under the webui -> graph settings section.

Also, you probably shouldn't call it MRTG graphs, just stacked graphs is fine.

@crcro crcro changed the title feature: mrtg stacked graphs feature: stacked graphs Nov 14, 2017

crcro added some commits Nov 14, 2017

unified config option
added webui graph config entry
added sql entry for webui config
@laf
Copy link
Member

laf left a comment

Great work on this :) Some comments in line.

@@ -49,3 +49,6 @@ $config['enable_billing'] = 1;

# Enable the in-built services support (Nagios plugins)
$config['show_services'] = 1;

# Enable the MRTG style graphs if set to true
$config['graph']['stacked'] = false;

This comment has been minimized.

@laf

laf Nov 14, 2017

Member

Maybe $config['graphs']['stacked'] = false;

We already have graphs for a few other config options.

This comment has been minimized.

@laf

laf Nov 14, 2017

Member

In fact, I'd say remove it from here anyway as we want users to use the webui to config this

@@ -24,6 +24,10 @@
'svg' => 'svg',
),
),
array('name' => 'webui.graph_stacked',

This comment has been minimized.

@laf

laf Nov 14, 2017

Member

You've used two variables now for this. I'd actually say graphs.stacked is better but I realise I used webui.graph_type for svg so I'd be fine with keeping this as well.

require 'includes/graphs/common.inc.php';
if (Config::get('graph.stacked') == true) {
$transparency = 95;

This comment has been minimized.

@laf

laf Nov 14, 2017

Member

What's the reason for the different transparency values in the files?

Overall we should put this block into a function to make it re-usable

This comment has been minimized.

@crcro

crcro Nov 14, 2017

Contributor

this graph is used on device ports overview and lower values make it unpleasant to view (too much transparency).

This comment has been minimized.

@laf

laf Nov 14, 2017

Member

Is that not the same for other pages then?

crcro added some commits Nov 14, 2017

@laf
Copy link
Member

laf left a comment

I've pulled this in and it currently doesn't work as the config option set in the DB isn't the one used in the function.

Can you add this config option to the configuration.md doc please. You don't need to put the $config value in just explain it can be changed and where.

For generate_stacked_graphs() it might be worth returning key/value pairs in the array to make it easier to reference and see what's going on.

In the actual graph pages, rather than constantly calling generate_stacked_graphs(), do it once early on and then store it to an array in that file. Unset it at the end as well.

The colours for me don't work. In traffic graphs we had green and purple before indicating in / out, now the colours are so close together that it's hard to tell the difference.

@laf

This comment has been minimized.

Copy link
Member

laf commented Nov 15, 2017

image

image

changes in code
added docs about stacked graphs
@crcro

This comment has been minimized.

Copy link
Contributor

crcro commented Nov 15, 2017

is better this way?
about the colors ... should change them?

@laf

This comment has been minimized.

Copy link
Member

laf commented Nov 16, 2017

Better but I would still use a key value return on the array, return array('transparency' => x, 'conversion' => y); or something so that using the variables within the graph code is obvious to someone else without having to look at what the function does.

The colours definitely need fixing, it's indistinguishable right now on what is what.

@crcro

This comment has been minimized.

Copy link
Contributor

crcro commented Nov 16, 2017

screen shot 2017-11-16 at 1 17 32 pm

screen shot 2017-11-16 at 1 17 50 pm

is this acceptable @laf ?

@laf

This comment has been minimized.

Copy link
Member

laf commented Nov 16, 2017

It's definitely better but why can't we just have the original colours with some transparency?

@crcro

This comment has been minimized.

Copy link
Contributor

crcro commented Nov 16, 2017

because if they overlay you will get that greyed tone (like in your picture where you have same in/out):

screen shot 2017-11-16 at 2 01 38 pm

screen shot 2017-11-16 at 2 01 49 pm

@laf

This comment has been minimized.

Copy link
Member

laf commented Nov 16, 2017

because if they overlay you will get that greyed tone (like in your picture where you have same in/out):

Ah sorry, right so you've not changed the colours it's just how it looks because of the transparency.

I'm not the target audience for this really so I'll leave it up to others comment on if they think it looks ok..

@kkrumm1

This comment has been minimized.

Copy link
Member

kkrumm1 commented Nov 18, 2017

are there other color options for stacked?

@crcro

This comment has been minimized.

Copy link
Contributor

crcro commented Nov 18, 2017

@kkrumm1 ... the scope is to use the same colors for inverted and stacked graphs. currently libre uses greens and blues for bits graph but at @laf points i've come with greens and purples ...

@kkrumm1

This comment has been minimized.

Copy link
Member

kkrumm1 commented Nov 18, 2017

Looks good to me then :) thank you @crcro

@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Nov 27, 2017

The inspection completed: 1 updated code elements

@laf

laf approved these changes Nov 27, 2017

@laf

laf approved these changes Nov 27, 2017

Copy link
Member

laf left a comment

@laf laf added this to the 1.35 milestone Nov 27, 2017

@kkrumm1

This comment has been minimized.

Copy link
Member

kkrumm1 commented Nov 28, 2017

👍

@laf laf merged commit 995b706 into librenms:master Dec 2, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@laf laf added Feature WebUI labels Dec 2, 2017

@kkrumm1

This comment has been minimized.

Copy link
Member

kkrumm1 commented Dec 3, 2017

When I try enable stacked graphs in the WebUI I get this error.
image

@laf

This comment has been minimized.

Copy link
Member

laf commented Dec 3, 2017

If this is on a test install you try different PRs on then it might be because your schema was already at or past this version. Run this in mysql:

INSERT INTO `config` (`config_name`, `config_value`, `config_default`, `config_descr`, `config_group`, `config_group_order`, `config_sub_group`, `config_sub_group_order`, `config_hidden`, `config_disabled`) VALUES('webui.graph_stacked', 'false', 'false', 'Display stacked graphs instead of inverted graphs', 'webui', 0, 'graph', 0, '0', '0');
@kkrumm1

This comment has been minimized.

Copy link
Member

kkrumm1 commented Dec 3, 2017

That worked... Also, this was on my production server. I never applied this PR to it this was after daily updates.

@crcro crcro deleted the crcro:mrtg-style-graphs branch Dec 4, 2017

@lock

This comment has been minimized.

Copy link

lock bot commented May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.