HTTP Traffic Generator

The implementation of HTTP traffic generator composes mainly three parts:
1. Client
2. Server
3. Runtime Variable
4. Distribution Generation (Notification to Statistics Research, Bell Labs, Lucent Technologies and The University of North Carolina at Chapel Hill)

When the traffic generator starts, the http client initialize connections to http server, after the connection has established, the client calls the distribution generation module and generates empirical parameters involved with http operation. For example, request size, response size, request gap time, server delay time, total objects per page, total pages per web session. All those parameters will be saved in runtime variable model, which can be accessed by both the http client and server. Based on the parameters saved in runtime variable, the client and server will generate http traffic which can represent real-world http traffic.
This traffic generator is capable of generating both HTTP 1.0 traffic as well as HTTP 1.1 traffic.
The detailed description can be found in in docs folder.

[Tom Henderson, 2012-03-02 02:04:08]

There seems to be a lot of good work here, providing what appears to be a
carefully worked on HTTP generator, and heavy use of the ns-3 attribute system. 
 However, I can't sign off on merging this until some things are addressed.

- it is poorly documented outside of the attribute system, in terms of Sphinx
and Doxygen. This must have originated from published work; where is the
reference?  There is no model library (.rst) file and only light overview
documentation in http.h.  The models themselves would raise a ton of doxygen
errors if I ran Doxygen on them (I did not test, just read the code).  It seems
to me that too many headers are exposed, particularly the random variable
headers and runtime variable headers-- making them private will reduce your
doxygen load.

- the tests are trivial.  If I just started going in and changing random
constants, your tests would continue to pass.  I would venture to say that this
appears to be untested code.  Was any validation performed (even if by hand)? 
If so, please document it.

- the use of random variables is a bit unorthodox; I do not see why it couldn't
be more in line with the ones defined in src/core.  It will be important to line
this up so that this model can benefit from the forthcoming adjustments to the
random variables to make deterministic stream assignments possible.

- outputs are dumped into a text file; this bypasses use of tracing system. 
Authors readily admit this.  However, I think this will annoy future
database-driven users of the model.  Suggest to export data as trace source and
have a file writer write out your text file.

- the example is daunting/bewildering for a new user.  Remember that most users
will want to copy paste code from the example into their scripts.  Your examples
should focus on the most common use cases (i.e. how simple can you make an
example, using two wired nodes)?  Then, create a few specialized examples to
exemplify the utility of (and how to) do something different than the base
example, such as a really good use case of the manual runtime variable (if you
decide to keep that in).  Please think of providing simple atomic examples for
the most common use cases.

- by the way, it was very unclear to me the purpose of exposing RuntimeVariable
to the user

I think it needs another review after the above are addressed.

http://codereview.appspot.com/4940041/diff/1/src/http/docs/http.h
File src/http/docs/http.h (right):

http://codereview.appspot.com/4940041/diff/1/src/http/docs/http.h#newcode59
src/http/docs/http.h:59: * model to generate HTTP traffic.
I do not see why there is such a manual mode.  the example doesn't make clear
use of this.  It seems like this will be beyond the needs/capabilities of most
users.

http://codereview.appspot.com/4940041/diff/1/src/http/docs/http.h#newcode62
src/http/docs/http.h:62: * to make sure that it does not mess up with object
from different page.
I did not understand this comment "mess up with object.."

http://codereview.appspot.com/4940041/diff/1/src/http/examples/http.cc
File src/http/examples/http.cc (right):

http://codereview.appspot.com/4940041/diff/1/src/http/examples/http.cc#newcode77
src/http/examples/http.cc:77: std::string transportProtocol ("TCP");
I did not really understand the need for the above two variables.

http://codereview.appspot.com/4940041/diff/1/src/http/examples/http.cc#newcod...
src/http/examples/http.cc:132: Config::SetDefault
("ns3::OnOffApplication::DataRate", StringValue (rate));
why on-off?

http://codereview.appspot.com/4940041/diff/1/src/http/examples/http.cc#newcod...
src/http/examples/http.cc:134: // Fix non-unicast data rate to be the same as
that of unicast
this comment doesn't match the code

http://codereview.appspot.com/4940041/diff/1/src/http/examples/http.cc#newcod...
src/http/examples/http.cc:164: Ptr<http::RuntimeVariable> runtimeVariable =
CreateObject<http::RuntimeVariable> ();
what is the point of creating a (default) runtimeVariable and passing it in?

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-random-varia...
File src/http/model/http-random-variable.cc (right):

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-random-varia...
src/http/model/http-random-variable.cc:123: UniformVariable uniform;
Can you remove these temporary random variables?  Why not draw values from
m_uniform?

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-random-varia...
src/http/model/http-random-variable.cc:143: UniformVariable uniform;
use m_uniform if possible

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-random-varia...
src/http/model/http-random-variable.cc:185: UniformVariable uniform;
m_uniform if possible

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-random-varia...
src/http/model/http-random-variable.cc:220: #define repeat for (;; )
remove this macro; impairs readability

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-random-varia...
src/http/model/http-random-variable.cc:311: UniformVariable uniform;
use m_uniform if possible

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-random-varia...
src/http/model/http-random-variable.cc:449: #define Min(a, b)      ((a) < (b) ?
(a) : (b))
unnecessary macro

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-random-varia...
src/http/model/http-random-variable.cc:1316: return 0;
is this unimplemented?

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-random-varia...
File src/http/model/http-random-variable.h (right):

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-random-varia...
src/http/model/http-random-variable.h:60: * \ingroup core
not in core module, rather http

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-random-varia...
src/http/model/http-random-variable.h:94: class HTTPRandomVariableBase : public
Object
ns-3 naming style is 'HttpRandomVariableBase'.  I am not sure why it doesnt
inherit from RandomVariable

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-random-varia...
src/http/model/http-random-variable.h:118: double qweibull (double p, double
shape, double scale)
capitalize method names.  do not include implementation in header file unless
inlined.   Doxygen missing.

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-random-varia...
src/http/model/http-random-variable.h:129: RngStream* m_generator;  //
underlying generator being wrapped
this variable appears to be unused

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-random-varia...
src/http/model/http-random-variable.h:163: * \brief Constructor.
doxygen error.

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-random-varia...
src/http/model/http-random-variable.h:177: double* ratep ()
we typically forbid this in public API (exposing raw pointer)

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-random-varia...
src/http/model/http-random-variable.h:271: */
doxygen error

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-runtime-vari...
File src/http/model/http-runtime-variable.h (right):

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-runtime-vari...
src/http/model/http-runtime-variable.h:131: uint32_t GetPageTimeout ();
see aodv code for example of how to doxygenate in a compact manner

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-server.h
File src/http/model/http-server.h (right):

http://codereview.appspot.com/4940041/diff/1/src/http/model/http-server.h#new...
src/http/model/http-server.h:111: Ipv4Address           m_server;              
// The ip address of server
seems a shame to limit this to IPv4-- could you make it IPv6-capable?  (Note, I
realize IPv6 transport was not available when this was posted)

http://codereview.appspot.com/4940041/diff/1/src/http/wscript
File src/http/wscript (right):

http://codereview.appspot.com/4940041/diff/1/src/http/wscript#newcode4
src/http/wscript:4: module = bld.create_ns3_module('http', ['internet',
'config-store', 'tools', 'applications'])
why does it depend on config-store?  Just for an example?

[Nicola Baldo, 2012-07-25 09:00:43]

I agree with Tom that this is a potentially very good contributions, but it
needs more work before it meets the quality standard required for merging with
ns-3-dev.

I reviewed Patch Set 2, I think that many points raised by Tom are still open:

- a lot of doxygen is missing

- there are still many coding style issues (e.g., correct
capitalization/CamelCase)

- the .rst doc section about testing are deceptive: you make a lot of claims
about testing and validation, but when one actually goes and looks into the
testing code it emerges that only a handful of tests are really meaningful, most
are useless (e.g., what's the purpose of testing the value of a constant?) and
overall there is minimal functional test coverage and no validation.

- the statistical model should be described in detail: the code is full of const
values for the various distribution, but there is no hint on where they come
from.

http://codereview.appspot.com/4940041/diff/5021/src/http/docs/http.rst
File src/http/docs/http.rst (right):

http://codereview.appspot.com/4940041/diff/5021/src/http/docs/http.rst#newcode45
src/http/docs/http.rst:45: 
Any detailed info on the statistical model that is implemented in the generator?
This is the most important piece of information that one would expect to appear
here.

http://codereview.appspot.com/4940041/diff/5021/src/http/docs/http.rst#newcode70
src/http/docs/http.rst:70: To have a node run HTTP, the easiest way would be to
use the ClickInternetStackHelper
ClickInternetStackHelper???

http://codereview.appspot.com/4940041/diff/5021/src/http/docs/http.rst#newcode87
src/http/docs/http.rst:87: Validation
I would call this just "Testing".

http://codereview.appspot.com/4940041/diff/5021/src/http/docs/http.rst#newcode92
src/http/docs/http.rst:92: * Unit tests have been written to verify the
internals of DSR. This can be found in ``src/http/test/http-test-suite.cc``.
These tests verify whether the methods inside DSR module which deal with packet
buffer, headers work correctly.
DSR?

http://codereview.appspot.com/4940041/diff/5021/src/http/docs/http.rst#newcode93
src/http/docs/http.rst:93: * Simulation cases similar to [1] have been tested
and have comparable results.
"have comparable results" with what? [1] seems to describe the same model that
is being reviewed here, and I see no comparison with other data there.

http://codereview.appspot.com/4940041/diff/5021/src/http/docs/http.rst#newcode94
src/http/docs/http.rst:94: * Random variable validation cases have been
conducted to fit the proposed distribution function for each http parameter
this sentence is not clear, and what you do is not evident from the test
program. Can you please explain better?

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
File src/http/model/http-distribution.h (right):

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
src/http/model/http-distribution.h:58: namespace http {
do you really need a separate namespace? most of ns3 code just sits in the ns3
namespace.

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
src/http/model/http-distribution.h:77: inline double GetRate ()
from the ns-3 coding style: "Do not include inline implementations in header
files; put all implementation in a .cc file (unless implementation in the header
file brings demonstrable and significant performance improvement)."

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
src/http/model/http-distribution.h:81: // Get the request size
please use doxygen style for these comments. Applies to all the other methods
below.

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
src/http/model/http-distribution.h:82: uint32_t GetReqsize ();
coding style: should be GetReqSize, maybe better GetRequestSize

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
src/http/model/http-distribution.h:84: uint32_t GetRspsize ();
coding style: GetResponseSize

note from the coding style doc: "A long descriptive name which requires a lot of
typing is always better than a short name which is hard to decipher. Do not use
abbreviations in names unless the abbreviation is really unambiguous and obvious
to everyone (e.g., use "size" over "sz")"

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
src/http/model/http-distribution.h:101: double GetReqgap (uint32_t page,
uint32_t obj);
GetReqestGap

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
src/http/model/http-distribution.h:103: uint32_t AdjustPersistRspsz ();
AdjustPersistResponseSize

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
src/http/model/http-distribution.h:105: void ResetPersistRspsz ();
ResetPersistResponseSize

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
src/http/model/http-distribution.h:115: HTTPFlowArriveRandomVariableImpl*
m_flowarriveRV;
coding style: 

HttpFlowArriveRandomVariableImpl m_flowArriveRandomVariable;

or just

HttpFlowArriveRandomVariableImpl m_flowArrive;

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
File src/http/model/http-random-variable.h (right):

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:68: struct arima_params
ArimaParams

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:73: uint32_t pAR, qMA;
doxygen?

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:81: class HTTPRandomVariableBase : public
RandomVariable
HttpRandomVariableBase

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:95: double logit (double m_x);
please use camel case for this and below methods
and add doxygen for each method

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:114:
/************************************************************************************************************************/
?

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:115: class FX
any more meaningful name?

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:139: class ARFIMAImpl : public
HTTPRandomVariableBase
ArfimaImpl

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:182: class
HTTPFlowArriveRandomVariableImpl : public HTTPRandomVariableBase
doxygen?

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:216: ///\name Fields
kill dead code

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:265: ///\name Fields
kill dead code

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:342: static struct arima_params
filesize_arima_params;
camel case

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:344: static FX fsize_invcdf[2][2];
camel case

http://codereview.appspot.com/4940041/diff/5021/src/http/test/http-test-suite.cc
File src/http/test/http-test-suite.cc (right):

http://codereview.appspot.com/4940041/diff/5021/src/http/test/http-test-suite...
src/http/test/http-test-suite.cc:205: NS_TEST_EXPECT_MSG_EQ
(http::HTTPFileSizeRandomVariableImpl::FSIZE1_PROB_A, 0.5, "trivial");
testing const values??? Come on, this is absolutely useless!!!

http://codereview.appspot.com/4940041/diff/5021/src/http/test/http-test-suite...
src/http/test/http-test-suite.cc:326: NS_TEST_EXPECT_MSG_EQ (rt.m_reqSize,  500,
"trivial");
are you testing that the assignment operator works properly?