Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(790)

Issue 245260043: DCF collectors

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 9 months ago by Tom Henderson
Modified:
8 years, 8 months ago
Reviewers:
buherman
CC:
ns-3-reviews_googlegroups.com, ll024_bucknell.edu, Felipe Perrone, tomh_tomh.org
Visibility:
Public.

Description

This is an update to the ns-3 Data Collection Framework, primarily authored by Li Li at Bucknell University. The basic idea is to provide a base Collector class (left out of the previous DCF code merge) and a few basic collector types, including an Event Driven Collector, a Time Series Collector, and a Time Averaging Collector.

Patch Set 1 #

Total comments: 27
Unified diffs Side-by-side diffs Delta from patch set Stats (+3507 lines, -642 lines) Patch
M CHANGES.html View 1 chunk +16 lines, -0 lines 2 comments Download
M doc/manual/Makefile View 1 chunk +0 lines, -1 line 0 comments Download
M doc/tutorial/source/data-collection.rst View 7 chunks +77 lines, -31 lines 0 comments Download
M examples/tutorial/seventh.cc View 2 chunks +8 lines, -10 lines 0 comments Download
R src/stats/doc/adaptor.rst View 1 chunk +0 lines, -48 lines 0 comments Download
M src/stats/doc/collector.rst View 1 chunk +53 lines, -6 lines 0 comments Download
M src/stats/doc/data-collection.rst View 1 chunk +0 lines, -1 line 0 comments Download
M src/stats/doc/data-collection-helpers.rst View 16 chunks +69 lines, -64 lines 0 comments Download
M src/stats/doc/scope-and-limitations.rst View 2 chunks +2 lines, -11 lines 0 comments Download
A src/stats/examples/event-driven-collector-example.cc View 1 chunk +166 lines, -0 lines 0 comments Download
M src/stats/examples/file-helper-example.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M src/stats/examples/gnuplot-helper-example.cc View 1 chunk +17 lines, -7 lines 1 comment Download
A src/stats/examples/scaling-collector-example.cc View 1 chunk +166 lines, -0 lines 0 comments Download
A src/stats/examples/time-averaging-collector-example.cc View 1 chunk +172 lines, -0 lines 0 comments Download
M src/stats/examples/time-probe-example.cc View 5 chunks +10 lines, -11 lines 0 comments Download
A src/stats/examples/time-series-collector-example.cc View 1 chunk +168 lines, -0 lines 0 comments Download
M src/stats/examples/wscript View 1 chunk +10 lines, -0 lines 0 comments Download
M src/stats/helper/file-helper.h View 12 chunks +119 lines, -18 lines 1 comment Download
M src/stats/helper/file-helper.cc View 12 chunks +151 lines, -98 lines 4 comments Download
M src/stats/helper/gnuplot-helper.h View 12 chunks +141 lines, -14 lines 2 comments Download
M src/stats/helper/gnuplot-helper.cc View 14 chunks +264 lines, -59 lines 4 comments Download
A src/stats/model/collector.h View 1 chunk +104 lines, -0 lines 6 comments Download
A src/stats/model/collector.cc View 1 chunk +98 lines, -0 lines 1 comment Download
M src/stats/model/data-collection-object.h View 4 chunks +72 lines, -2 lines 4 comments Download
M src/stats/model/data-collection-object.cc View 2 chunks +23 lines, -2 lines 1 comment Download
A src/stats/model/event-driven-collector.h View 1 chunk +77 lines, -0 lines 0 comments Download
A src/stats/model/event-driven-collector.cc View 1 chunk +89 lines, -0 lines 0 comments Download
A src/stats/model/scaling-collector.h View 1 chunk +76 lines, -0 lines 0 comments Download
A src/stats/model/scaling-collector.cc View 1 chunk +96 lines, -0 lines 0 comments Download
A src/stats/model/time-averaging-collector.h View 1 chunk +121 lines, -0 lines 0 comments Download
A src/stats/model/time-averaging-collector.cc View 1 chunk +158 lines, -0 lines 1 comment Download
M src/stats/model/time-probe.h View 3 chunks +4 lines, -4 lines 0 comments Download
M src/stats/model/time-probe.cc View 6 chunks +5 lines, -5 lines 0 comments Download
R src/stats/model/time-series-adaptor.h View 1 chunk +0 lines, -129 lines 0 comments Download
R src/stats/model/time-series-adaptor.cc View 1 chunk +0 lines, -115 lines 0 comments Download
A src/stats/model/time-series-collector.h View 1 chunk +109 lines, -0 lines 0 comments Download
A src/stats/model/time-series-collector.cc View 1 chunk +146 lines, -0 lines 0 comments Download
A src/stats/test/event-driven-collector-test-suite.cc View 1 chunk +170 lines, -0 lines 0 comments Download
M src/stats/test/examples-to-run.py View 1 chunk +4 lines, -0 lines 0 comments Download
A src/stats/test/scaling-collector-test-suite.cc View 1 chunk +172 lines, -0 lines 0 comments Download
A src/stats/test/time-averaging-collector-test-suite.cc View 1 chunk +188 lines, -0 lines 0 comments Download
A src/stats/test/time-series-collector-test-suite.cc View 1 chunk +168 lines, -0 lines 0 comments Download
M src/stats/wscript View 3 chunks +15 lines, -3 lines 0 comments Download

Messages

Total messages: 4
buherman
Hi, There's quite a lot of stuff to be reviewed here. At the moment I ...
8 years, 9 months ago (2015-06-23 13:30:04 UTC) #1
Tom Henderson
On 2015/06/23 13:30:04, buherman wrote: > Hi, > > There's quite a lot of stuff ...
8 years, 8 months ago (2015-06-25 14:23:25 UTC) #2
buherman
On 2015/06/25 14:23:25, Tom Henderson wrote: > On 2015/06/23 13:30:04, buherman wrote: > > Hi, ...
8 years, 8 months ago (2015-06-29 08:49:18 UTC) #3
buherman
8 years, 8 months ago (2015-06-29 08:53:54 UTC) #4
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-he...
File src/stats/examples/gnuplot-helper-example.cc (right):

https://codereview.appspot.com/245260043/diff/1/src/stats/examples/gnuplot-he...
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....
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....
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....
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....
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....
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-help...
File src/stats/helper/gnuplot-helper.cc (right):

https://codereview.appspot.com/245260043/diff/1/src/stats/helper/gnuplot-help...
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-help...
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-help...
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-help...
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-help...
File src/stats/helper/gnuplot-helper.h (right):

https://codereview.appspot.com/245260043/diff/1/src/stats/helper/gnuplot-help...
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-help...
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#...
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#n...
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#n...
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#n...
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#n...
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#n...
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#n...
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-averagin...
File src/stats/model/time-averaging-collector.cc (right):

https://codereview.appspot.com/245260043/diff/1/src/stats/model/time-averagin...
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?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b