[Ns-developers] [ns3] Statistical framework (draft)

[Joseph Kopena]

On Wed, May 14, 2008 at 11:56 AM, Mathieu Lacage
<mathieu.lacage at sophia.inria.fr> wrote:
>  - PlainTextStatFramework uses multiple inheritance: we have worked
> pretty hard to never do this in ns-3 so, it would be nice to keep on
> doing this. A simple way to avoid this would be to use a
> Callback<void,std::string,uint32_t> instead of the StatOutputCallback

Hi Mathieu,

Yes, poor form on my part---temporary lazyness.  No objections.

>  - most of your class member variables seem to be protected by default.
> private would be a better default for most of them and protected should
> be used when you really cannot do otherwise.

For the most part I have trouble justifying the runtime hit & extra
code associated w/ private simple members that subclasses will make
frequent use of and will need accessors, but don't have strong
feelings on this.

> The main use-case you seem to be concerned with is providing a way to
> asbtract the storage of the data/statistics to allow the user to choose
> between multiple storage formats: text, xml, database. Am I right ?
>
> So, if I look at your system, you have:
>  - StatFramework: abtract the storage system. Subclasses implement
> serialization to the right storage format.
>  - StatCalculator: abstract the type of statistics being calculated.
> Subclasses implement the actual statistics calculation.
>
> If I understand the code correctly, you could presumably implement a
> DataStatCalculator which would not perform and statistics calculation
> and would just dump in the framework the raw data, right ?

Agreed, though I think the emphasis is on providing a simple mechanism
to collect data, rather than focusing on supporting multiple output
formats.

> It seems to me that it is also conceivable to implement something like a
> GnuplotStatFramework, right ?

Yes, though I think that's a slightly different issue.  Most plots
will be based on data from multiple runs, which the code there is
currently not explicitly supporting.  Doing that well is something we
have to think about a bit.

>  - a better name for StatFramework might be DataStorage or something
> similar to outline its relationship to data storage format.

No objection.

The more interesting question here then is what is the relationship of
this component in this module to the existing trace logging, e.g. pcap
and ascii?  Is there any?  Both are doing data storage...

>  - I would tend to move out Counter creation out of the StatFramework
> base class and make the StatCalculator constructor take a pointer to its
> associated StatFramework instead: allows you to add new subclasses to
> StatCalculator without patching StatFramework

This I think is not so straightforward.  The rationale for the factory
pattern is to be able to easily switch between stat framework
implementations.  For example, to go from plain text to DB I just
switch which framework is created, and all of the actual
instrumentation code remains the same.  It's conceivable to me that
there could be StatCalculators which depend on the type of the
underlying framework, e.g. a DataStatCalculator you mentioned earlier
could look very different in plain text vs DB form.  If sim writers
create those calculator objects directly, they need to change several
things in order to switch output formats.  I have no opposition to
adding functionality to StatFramework to enable manual addition of
directly created StatCalculator objects, which would allow you to use
new StatCalculator subclasses without patching all StatFramework
implementations.  However, I think those changes once the module is
mature are less likely than people switching between output formats,
so I'm hesitant to push that hassle onto the sim writers.

>  - the primitive data type you manipulate is a uint32_t
> (CounterStat::Update and StatOutputCallback::OutputSingleton): I think
> that it is important to support other types such a double. Are there
> other types beyond uint32_t and double that we want to support ?
> uint64_t ? Time ? We need to have a list.

Agreed; uint32_t is the only one listed right now merely because it's
all I got around to so far.  A list should be made up, most likely as
people denote need; double and Time are definites.

> I understand that it is probably a matter of taste, but moving your
> headers to src/stats would be more in line with the "ns-3 way" and would
> make it easier for me to review the code (because I don't have to cd
> constantly to move from headers to implementations).

Again, no huge issues.

Thx

-- 
- joe kopena
right here and now