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

Issue 318800043: Stats module enhancements

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 11 months ago by jani.puttonen
Modified:
4 years, 10 months ago
Reviewers:
Tom Henderson
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

We have implemented a set of ns-3 statistics module enhancements, which we are widely using in the statistics collection of the satellite module of SNS3 (http://satellite-ns3.com/). The statistics module enhancements can be summarized as follows: - Modified gnuplot-aggregator - Moved application-packet-probe to stats from applications module - A set of (example) helper classes for setting up the delay and throughput statistics - A stats-helper-example illustrating the usage of the additions - A test suite for distribution collectors - Sphinx documentation (.rst files) has been updated related to the additions - A set of new probes, collectors and aggragators listed below New probes: - bytes-probe - application packet probe New collectors - collector-map - distribution-collector - interval-rate-collector - scalar-collector - unit-conversion-collector New aggregators: - multi-file-aggregator Other - traffic-time-tag - stats-callback-definitions

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+10169 lines, -262 lines) Patch
R src/applications/model/application-packet-probe.h View 1 chunk +0 lines, -127 lines 0 comments Download
R src/applications/model/application-packet-probe.cc View 1 chunk +0 lines, -123 lines 0 comments Download
M src/applications/wscript View 2 chunks +0 lines, -2 lines 0 comments Download
M src/stats/doc/aggregator.rst View 1 chunk +55 lines, -0 lines 0 comments Download
M src/stats/doc/collector.rst View 1 chunk +267 lines, -1 line 0 comments Download
M src/stats/doc/data-collection-helpers.rst View 3 chunks +178 lines, -2 lines 0 comments Download
M src/stats/doc/probe.rst View 1 chunk +1 line, -0 lines 0 comments Download
M src/stats/doc/scope-and-limitations.rst View 2 chunks +9 lines, -2 lines 0 comments Download
A src/stats/examples/stats-helper-example.cc View 1 chunk +178 lines, -0 lines 0 comments Download
M src/stats/examples/wscript View 1 chunk +4 lines, -1 line 0 comments Download
A src/stats/helper/stats-delay-helper.h View 1 chunk +196 lines, -0 lines 0 comments Download
A src/stats/helper/stats-delay-helper.cc View 1 chunk +693 lines, -0 lines 0 comments Download
A src/stats/helper/stats-helper.h View 1 chunk +326 lines, -0 lines 0 comments Download
A src/stats/helper/stats-helper.cc View 1 chunk +426 lines, -0 lines 0 comments Download
A src/stats/helper/stats-throughput-helper.h View 1 chunk +171 lines, -0 lines 0 comments Download
A src/stats/helper/stats-throughput-helper.cc View 1 chunk +510 lines, -0 lines 0 comments Download
A src/stats/model/application-delay-probe.h View 1 chunk +146 lines, -0 lines 0 comments Download
A src/stats/model/application-delay-probe.cc View 1 chunk +128 lines, -0 lines 0 comments Download
A src/stats/model/application-packet-probe.h View 1 chunk +127 lines, -0 lines 0 comments Download
A src/stats/model/application-packet-probe.cc View 1 chunk +123 lines, -0 lines 0 comments Download
A src/stats/model/bytes-probe.h View 1 chunk +119 lines, -0 lines 1 comment Download
A src/stats/model/bytes-probe.cc View 1 chunk +117 lines, -0 lines 0 comments Download
A src/stats/model/collector-map.h View 1 chunk +496 lines, -0 lines 0 comments Download
A src/stats/model/collector-map.cc View 1 chunk +145 lines, -0 lines 0 comments Download
A src/stats/model/distribution-collector.h View 1 chunk +614 lines, -0 lines 0 comments Download
A src/stats/model/distribution-collector.cc View 1 chunk +826 lines, -0 lines 0 comments Download
M src/stats/model/gnuplot-aggregator.h View 3 chunks +17 lines, -0 lines 0 comments Download
M src/stats/model/gnuplot-aggregator.cc View 6 chunks +40 lines, -4 lines 0 comments Download
A src/stats/model/interval-rate-collector.h View 1 chunk +328 lines, -0 lines 0 comments Download
A src/stats/model/interval-rate-collector.cc View 1 chunk +474 lines, -0 lines 0 comments Download
A src/stats/model/multi-file-aggregator.h View 1 chunk +486 lines, -0 lines 0 comments Download
A src/stats/model/multi-file-aggregator.cc View 1 chunk +986 lines, -0 lines 0 comments Download
A src/stats/model/scalar-collector.h View 1 chunk +260 lines, -0 lines 0 comments Download
A src/stats/model/scalar-collector.cc View 1 chunk +345 lines, -0 lines 0 comments Download
A src/stats/model/stats-callback-definitions.h View 1 chunk +61 lines, -0 lines 0 comments Download
A src/stats/model/traffic-time-tag.h View 1 chunk +62 lines, -0 lines 0 comments Download
A src/stats/model/traffic-time-tag.cc View 1 chunk +94 lines, -0 lines 0 comments Download
A src/stats/model/unit-conversion-collector.h View 1 chunk +295 lines, -0 lines 0 comments Download
A src/stats/model/unit-conversion-collector.cc View 1 chunk +344 lines, -0 lines 0 comments Download
A src/stats/test/distribution-collector-test-suite.cc View 1 chunk +494 lines, -0 lines 0 comments Download
M src/stats/wscript View 5 chunks +28 lines, -0 lines 0 comments Download

Messages

Total messages: 1
Tom Henderson
4 years, 10 months ago (2016-12-09 05:53:34 UTC) #1
Sorry for the delay in reviewing.  It is a large patch and I am still thinking
about parts of it.

In general, the patch assumes that stats module depends on network and
application modules, but we have in the past tried to avoid such dependency. 
That is why, for example, application-packet-probe.h is in applications module. 
I would rather locate these pieces with non-core dependencies back to other
modules.

I left some comments inline about BytesProbe.

There are many collectors, and we have gone too long without any in mainline, so
although I would like to review those carefully, I generally think it will be
good to start including into stats.

I haven't tried multi-file-aggregator but it is probably fine to add.

StatsHelper may be better off in network/utils or network/helper?  It is kind of
like a node-scoped helper (I might rename usage of 'identifier' term to
'scope').

I realize that SNS3 blocks on this merge or else we need to find other homes
(e.g. back in SNS3) for the code, so I will return to this with more comments
sometime soon.

https://codereview.appspot.com/318800043/diff/1/src/stats/model/bytes-probe.h
File src/stats/model/bytes-probe.h (right):

https://codereview.appspot.com/318800043/diff/1/src/stats/model/bytes-probe.h...
src/stats/model/bytes-probe.h:51: *   value, but BytesProbe is.
I agree that this is needed additional capability (to trace a
'TracedCallback<uint32_t>' as opposed to a 'TracedValue<uint32_t>').  However, I
question whether it needs to have 'Bytes' semantics rather than just the
underlying data type like all of the other probe types.

Ideally, it seems to me that something like:

template<typename T>
class TracedValueProbe<T> ...

and 

template<typenameT>
class TracedCallbackProbe<T>

would be the best for this.  I do not remember the reason we had difficulty with
templates when the probes were originally done.  So we have presently a
Uinteger32Probe (to hook to a TracedValue<uint32_t>) and this code is proposing
a Uinteger32TracedCallbackProbe (but calling it a BytesProbe).

So in short, I agree with the functionality but would like a different name, and
would like to check again if templates might work.
Sign in to reply to this message.

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