Code review - Issue 245260043: DCF collectorshttps://codereview.appspot.com/2015-06-29T08:53:54+00:00rietveld
Message from unknown
2015-06-12T01:23:57+00:00Tom Hendersonurn:md5:5a48e25c32a49fc5f941d5d113bb139e
Message from budiarto.herman@magister.fi
2015-06-23T13:30:04+00:00buhermanurn:md5:2d0ab6e225d49e36336bd8904d4e8d30
Hi,
There's quite a lot of stuff to be reviewed here. At the moment I have a few comments about enabling/disabling data collection objects (DCO).
The first is about the initial state of collectors. The 'Enabled' attribute of DCO is "true" by default. However, I find that TimeSeriesCollector and TimeAveragingCollector don't report any data downstream to the aggregator. This issue is apparently fixed when I explicitly call Enable() on the collectors.
My preference to the above is to have collectors pass the data downstream without any need to call Enable().
The second is about SetEnabledPeriod() method of DCO. It doesn't make the DCO enabled nor disabled at the specified times, because there is no code to handle that. Is this an unfinished feature?
And also please find below some minor comments.
Regards,
-budi-
https://codereview.appspot.com/245260043/diff/1/CHANGES.html
File CHANGES.html (right):
https://codereview.appspot.com/245260043/diff/1/CHANGES.html#newcode66
CHANGES.html:66: <li> (stats) The GnuplotHelper class method PlotProbe() has been deprectated, replaced by a similar AddProbe() to enable multiple data series.
deprectated -> deprecated
https://codereview.appspot.com/245260043/diff/1/CHANGES.html#newcode68
CHANGES.html:68: <li> (stats) The FileHelper class method WriteProbe() has been deprectated, replaced by a similar AddProbe(), for API alignment with GnuplotHelper.
(idem)
https://codereview.appspot.com/245260043/diff/1/src/stats/model/data-collection-object.cc
File src/stats/model/data-collection-object.cc (right):
https://codereview.appspot.com/245260043/diff/1/src/stats/model/data-collection-object.cc#newcode105
src/stats/model/data-collection-object.cc:105: // automatically enable the collector in order for TimeDrivenCollector
TimeDrivenCollector -> TimeSeriesCollector and TimeAveragingCollector
https://codereview.appspot.com/245260043/diff/1/src/stats/model/data-collection-object.h
File src/stats/model/data-collection-object.h (right):
https://codereview.appspot.com/245260043/diff/1/src/stats/model/data-collection-object.h#newcode75
src/stats/model/data-collection-object.h:75: * Collector::Enable() or Collector::Disable() again. The Collector class
Collector -> DataCollectionObject
https://codereview.appspot.com/245260043/diff/1/src/stats/model/data-collection-object.h#newcode78
src/stats/model/data-collection-object.h:78: * A good pratice will be that after calling SetEnabledPeriod(), do not
pratice -> practice
https://codereview.appspot.com/245260043/diff/1/src/stats/model/data-collection-object.h#newcode82
src/stats/model/data-collection-object.h:82: void SetEnabledPeriod (Time startTime, Time endTime);
Time -> const Time &
https://codereview.appspot.com/245260043/diff/1/src/stats/model/data-collection-object.h#newcode130
src/stats/model/data-collection-object.h:130: void ReportData (void);
This method is not implemented in the .cc file. I suggest to remove this.
Message from tomh.org@gmail.com
2015-06-25T14:23:25+00:00Tom Hendersonurn:md5:1f938e92c0b0cc52fe3f0d781185bfc1
On 2015/06/23 13:30:04, buherman wrote:
> Hi,
>
> There's quite a lot of stuff to be reviewed here. At the moment I have a few
> comments about enabling/disabling data collection objects (DCO).
>
> The first is about the initial state of collectors. The 'Enabled' attribute of
> DCO is "true" by default. However, I find that TimeSeriesCollector and
> TimeAveragingCollector don't report any data downstream to the aggregator. This
> issue is apparently fixed when I explicitly call Enable() on the collectors.
>
> My preference to the above is to have collectors pass the data downstream
> without any need to call Enable().
Agree in general, but I think what is attempted here is to suppress data generation of these regularly reporting collectors when they haven't received any data from upstream. The first such data arrival should stimulate their data reporting , as well as an explicit call to Enable().
What do you think should be the behavior of a collector reporting data for which no probe has sent it data yet? export 0, NaN, or just keep quiet?
>
> The second is about SetEnabledPeriod() method of DCO. It doesn't make the DCO
> enabled nor disabled at the specified times, because there is no code to handle
> that. Is this an unfinished feature?
Probably.
>
> And also please find below some minor comments.
Agree with these, thanks.
Message from budiarto.herman@magister.fi
2015-06-29T08:49:18+00:00buhermanurn:md5:308d3c05749c08c194eef3f67a5b7b19
On 2015/06/25 14:23:25, Tom Henderson wrote:
> On 2015/06/23 13:30:04, buherman wrote:
> > Hi,
> >
> > There's quite a lot of stuff to be reviewed here. At the moment I have a few
> > comments about enabling/disabling data collection objects (DCO).
> >
> > The first is about the initial state of collectors. The 'Enabled' attribute of
> > DCO is "true" by default. However, I find that TimeSeriesCollector and
> > TimeAveragingCollector don't report any data downstream to the aggregator.
> This
> > issue is apparently fixed when I explicitly call Enable() on the collectors.
> >
> > My preference to the above is to have collectors pass the data downstream
> > without any need to call Enable().
>
> Agree in general, but I think what is attempted here is to suppress data
> generation of these regularly reporting collectors when they haven't received
> any data from upstream. The first such data arrival should stimulate their data
> reporting , as well as an explicit call to Enable().
In contrast to the above, I find that the first arrival of data does *not* stimulate data reporting nor explicit call to Enable().
>
> What do you think should be the behavior of a collector reporting data for which
> no probe has sent it data yet? export 0, NaN, or just keep quiet?
I think this behaviour should be up to the collector.
- For collectors that involve counting from zero, then it should be zero. This code review does not have any such collectors.
- For collectors that involve averaging function (e.g., TimeSeries, TimeAveraging), then it should output NaN.
- For collectors that are not limited to specific reporting period (e.g., EventDriven, Scaling), then it should keep quiet.
>
> >
> > The second is about SetEnabledPeriod() method of DCO. It doesn't make the DCO
> > enabled nor disabled at the specified times, because there is no code to
> handle
> > that. Is this an unfinished feature?
>
> Probably.
If it's not ready, I would suggest to remove them from the initial push for now, and possibly include them later on.
>
> >
> > And also please find below some minor comments.
>
> Agree with these, thanks.
Some more comments are coming.
Message from budiarto.herman@magister.fi
2015-06-29T08:53:54+00:00buhermanurn:md5:d895d6e60c141593ba91d7ddf6c48e2f
The following is some comments around the Collector base class and the helper classes.
-budi-
https://codereview.appspot.com/245260043/diff/1/src/stats/examples/gnuplot-helper-example.cc
File src/stats/examples/gnuplot-helper-example.cc (right):
https://codereview.appspot.com/245260043/diff/1/src/stats/examples/gnuplot-helper-example.cc#newcode134
src/stats/examples/gnuplot-helper-example.cc:134: //plotHelper.SetTimeSeriesCollectorPeriod(Seconds(3));
Uncommenting this results in compile error. It should be: plotHelper.SetCollectorType ("ns3::TimeSeriesCollector", "Period", TimeValue (Seconds (3)));
https://codereview.appspot.com/245260043/diff/1/src/stats/helper/file-helper.cc
File src/stats/helper/file-helper.cc (left):
https://codereview.appspot.com/245260043/diff/1/src/stats/helper/file-helper.cc#oldcode232
src/stats/helper/file-helper.cc:232: // Add this time series adaptor to the map so that it can be used.
time series adaptor -> collector
https://codereview.appspot.com/245260043/diff/1/src/stats/helper/file-helper.cc#oldcode478
src/stats/helper/file-helper.cc:478: // probe's context, a unique adaptor needs to be created for each
adaptor -> collector
https://codereview.appspot.com/245260043/diff/1/src/stats/helper/file-helper.cc
File src/stats/helper/file-helper.cc (right):
https://codereview.appspot.com/245260043/diff/1/src/stats/helper/file-helper.cc#newcode548
src/stats/helper/file-helper.cc:548: // Connect the probe to the adaptor.
adaptor -> collector
https://codereview.appspot.com/245260043/diff/1/src/stats/helper/file-helper.cc#newcode616
src/stats/helper/file-helper.cc:616: MakeCallback (&Collector::TraceSinkDouble,
TraceSinkDouble -> TraceSinkTime
https://codereview.appspot.com/245260043/diff/1/src/stats/helper/file-helper.h
File src/stats/helper/file-helper.h (right):
https://codereview.appspot.com/245260043/diff/1/src/stats/helper/file-helper.h#newcode111
src/stats/helper/file-helper.h:111: std::string n7 = "", const AttributeValue &v7 = EmptyAttributeValue ());
std::string -> const std:string & in order to make it consistent with the other methods of this class.
https://codereview.appspot.com/245260043/diff/1/src/stats/helper/gnuplot-helper.cc
File src/stats/helper/gnuplot-helper.cc (right):
https://codereview.appspot.com/245260043/diff/1/src/stats/helper/gnuplot-helper.cc#newcode409
src/stats/helper/gnuplot-helper.cc:409: // Add this collector to the map so that it can be used.
A little bit too much indenting.
https://codereview.appspot.com/245260043/diff/1/src/stats/helper/gnuplot-helper.cc#newcode523
src/stats/helper/gnuplot-helper.cc:523: }
Several lines above this are indented too long.
https://codereview.appspot.com/245260043/diff/1/src/stats/helper/gnuplot-helper.cc#newcode614
src/stats/helper/gnuplot-helper.cc:614: // the ScalingCollector object to the aggregator
IMO, one of the use cases of ScalingCollector is to convert values to a different unit. Because of this, ScalingCollector should be positioned before the collector, not after like above.
Let's use Magister's DistributionCollector as an example. In DistributionCollector, the sample values are transposed to X-axis, while Y-axis displays the distribution of the values, i.e., a histogram. Note that ScalingCollector scales the values in Y-axis. Because of that, when using ScalingCollector for unit conversion purpose, it has to be done before DistributionCollector, otherwise the ScalingCollector will convert the distributions instead.
https://codereview.appspot.com/245260043/diff/1/src/stats/helper/gnuplot-helper.cc#newcode631
src/stats/helper/gnuplot-helper.cc:631: }
The position of curly braces in the above block is not according to ns-3 style.
https://codereview.appspot.com/245260043/diff/1/src/stats/helper/gnuplot-helper.h
File src/stats/helper/gnuplot-helper.h (right):
https://codereview.appspot.com/245260043/diff/1/src/stats/helper/gnuplot-helper.h#newcode116
src/stats/helper/gnuplot-helper.h:116: void SetScalingFactor (double scalingFactor);
This method is useful, but I don't see the same method implemented in FileHelper. Is there any reason for not including it in FileHelper?
https://codereview.appspot.com/245260043/diff/1/src/stats/helper/gnuplot-helper.h#newcode148
src/stats/helper/gnuplot-helper.h:148: std::string n7 = "", const AttributeValue &v7 = EmptyAttributeValue ());
std::string -> const std:string & in order to make it consistent with the other methods of this class.
https://codereview.appspot.com/245260043/diff/1/src/stats/model/collector.cc
File src/stats/model/collector.cc (right):
https://codereview.appspot.com/245260043/diff/1/src/stats/model/collector.cc#newcode22
src/stats/model/collector.cc:22: #include <cfloat>
cmath and cfloat are not needed in this file.
https://codereview.appspot.com/245260043/diff/1/src/stats/model/collector.h
File src/stats/model/collector.h (right):
https://codereview.appspot.com/245260043/diff/1/src/stats/model/collector.h#newcode29
src/stats/model/collector.h:29: #include "ns3/nstime.h"
Duplicate include statement.
https://codereview.appspot.com/245260043/diff/1/src/stats/model/collector.h#newcode55
src/stats/model/collector.h:55: virtual void TraceSinkBoolean (bool oldValue, bool newValue);
This method will convert from "true" to 1.0, and from "false" to 0.0. How about adding a note here to clarify that?
https://codereview.appspot.com/245260043/diff/1/src/stats/model/collector.h#newcode79
src/stats/model/collector.h:79: virtual void TraceSinkUinteger8 (uint8_t oldValue, uint8_t newValue);
How about TraceSinkUinteger64?
https://codereview.appspot.com/245260043/diff/1/src/stats/model/collector.h#newcode87
src/stats/model/collector.h:87: virtual void TraceSinkSequenceNumber32 (SequenceNumber32 &oldValue, SequenceNumber32 &newValue);
SequenceNumber32 & -> const SequenceNumber32 &
https://codereview.appspot.com/245260043/diff/1/src/stats/model/collector.h#newcode95
src/stats/model/collector.h:95: virtual void TraceSinkTime (Time &oldValue, Time &newValue);
Time & -> const Time &
https://codereview.appspot.com/245260043/diff/1/src/stats/model/collector.h#newcode95
src/stats/model/collector.h:95: virtual void TraceSinkTime (Time &oldValue, Time &newValue);
Add a note here that the input Time values are converted to seconds.
https://codereview.appspot.com/245260043/diff/1/src/stats/model/time-averaging-collector.cc
File src/stats/model/time-averaging-collector.cc (right):
https://codereview.appspot.com/245260043/diff/1/src/stats/model/time-averaging-collector.cc#newcode81
src/stats/model/time-averaging-collector.cc:81: m_reportEventId = Simulator::Schedule (m_period, &TimeAveragingCollector::ReportData, this);
If m_period is 0, the simulation will run indefinitely. Magister's IntervalRateCollector handles this case by not reporting anything downstream. Another option is to raise a fatal error. This leaves us with a question for discussion: what should be the right way to handle this case?