On Wed, Dec 16, 2009 at 1:18 PM, Carlo Marcelo Arenas Belon <
care...@sajinet.com.pe> wrote:
> On Tue, Dec 15, 2009 at 02:32:07PM +0100, Sebastien Termeau wrote:
> > Dear Ganglia Developers,
> >
> > Please find below a patch that brings trends to Ganglia.
>
> Really interesting, would you mind filing and enhancement bug on
> www.ganglia.info?, that would be also a great place for attaching
> those images you said were also needed.
>
Sure, I will take a look at that.
>
> > It uses RRD's LSLSLOPE, LSLINT and PREDICT (requires RRD >= 1.4.0 ) to
> > provide two kinds of trends.
> > Trends can be disabled by modifying conf.php ($with_trends).
>
> considering that RRD >= 1.4.0 isn't that common yet would assume is
> probably
> better to have this turned off by default for now
>
The code below checks which version of RRD is installed and only displays
what can be done.
So if RRD >= 1.4.0 then the PREDICT trends are available.
I should also do the same for LSLSLOPE and LSLINT since they are only
available since 1.2.12.
>
> > I also modified the host_view template to use tables instead of sending a
> > <BR> after "n" metrics.
>
> would you mind doing this change in an independent that would be applied
> before the one that adds your feature?, it would be also better if the
> patch is done against svn trunk as it will be easier to integrate that
> way but shouldn't be that difficult to correct that either if you don't
> feel comfortable working with subversion or git (using git svn).
>
No problem.
>
> more comments inlined with the code.
>
> > -----------
> > diff -ur ganglia.ori/conf.php ganglia/conf.php
> > --- ganglia.ori/conf.php 2009-12-14 15:19:23.000000000 +0100
> > +++ ganglia/conf.php 2009-12-15 12:05:49.000000000 +0100
> > @@ -64,6 +64,18 @@
> > #
> > $show_meta_snapshot = "yes";
> >
> > +#
> > +# Show trends icons next to each single metric graph
> > +#
> > +$with_trends = "yes";
>
> have you tested it this as "false" as well?, what about a version
> of RRD that doesn't support this?, would recommend having this
> off by default and adding a comment saying only to turn on if
> using the right version of rrdtool.
>
Here is the check "if ($with_trends == 'yes'){"
So anything but 'yes' will disable the trends.
Do you think we should also accept "true" and "1" for instance?
>
> > @@ -140,6 +142,12 @@
> > # Get_context makes start negative.
> > $start = $sourcetime + $start;
> > }
> > +
> > +# For trends, we double the time range
> > +if ($trend_type != ''){
> > + $rrdtool_graph['end']=$end + ($end-$start);
> > +}
> > +
> > # Fix from Phil Radden, but step is not always 15 anymore.
> > if ($range=="month")
> > $rrdtool_graph['end'] = floor($rrdtool_graph['end'] / 672) * 672;
>
> could you elaborate on why this is needed?, other than of course allow for
> a trend to be
> visible, should we allow the trend end target to be configurable instead?
>
The purpose is to display 1 hour in the futur if we are looking at the
hourly datas, 1 day in the futur for daily datas etc ...
Of course we can add parameters to allow users to customize the view but I
am not sure it is really useful.
>
> > diff -ur ganglia.ori/host_view.php ganglia/host_view.php
> > --- ganglia.ori/host_view.php 2009-12-14 15:19:23.000000000 +0100
> > +++ ganglia/host_view.php 2009-12-15 14:16:46.000000000 +0100
> > @@ -161,10 +161,26 @@
> > $tpl->newBlock("vol_metric_info");
> > $tpl->assign("graphargs", $v['graph']);
> > $tpl->assign("alt", "$hostname $name");
> > +
>
> huh?
>
This blank line is really important :p
>
> > if (isset($v['description']))
> > $tpl->assign("desc", $v['description']);
> > - if ( !(++$i % $metriccols) )
> > - $tpl->assign("br", "<BR>");
> > + # PREDICT supported in 1.4.0
> > + if ($with_trends == 'yes'){
> > + if( version_compare($version["rrdtool"], '1.4.5') >= 0) {
>
> 1.4.5?
>
Oops, should be 1.4.0. I forgot to remove that after a test I made.
>
> > + $tpl->newBlock("trend_predict");
> > + $tpl->assign("graphargs", $v['graph']);
> > +
> $tpl->assign("images","./templates/$template_name/images");
> > + }
>
Here I should check for version >= 1.2.12.
I will modify that part.
> > + else {
> > + $tpl->newBlock("trend");
> > + $tpl->assign("graphargs", $v['graph']);
> > +
> $tpl->assign("images","./templates/$template_name/images");
> > + }
> > + }
> > + if ( !(++$i % $metriccols) ){
> > + $tpl->gotoBlock ("vol_metric_info");
> > + $tpl->assign("new_row", "</TR><TR>");
> > + }
>
> who gets the last "</TR>" addded to close the table?
>
The last </tr> is always added just before closing the table :
<!-- END BLOCK : vol_metric_info -->
+</tr>
+</table>
</DIV>
<!-- END BLOCK : vol_group_info -->
As suggested, I will submit a separate patch for that.
> Carlo
>
------------------------------------------------------------------------------
This SF.Net email is sponsored by the Verizon Developer Community
Take advantage of Verizon's best-in-class app development support
A streamlined, 14 day to market process makes app distribution fast and easy
Join now and get one step closer to millions of Verizon customers
http://p.sf.net/sfu/verizon-dev2dev
_______________________________________________
Ganglia-developers mailing list
Ganglia-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ganglia-developers
