Mailing-List: contact dev-help@jmeter.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@jmeter.apache.org
Received-SPF: pass (nike.apache.org: domain of bhatt.chaitanya@gmail.com
 designates 209.85.216.172 as permitted sender)
MIME-Version: 1.0
In-Reply-To: 
 <CAH9fUpYy_fxXiY3YTUfFO_3J6Eqyg4oRA5qmjAqAuQbCkywPOQ@mail.gmail.com>
References: <5479B8B8.9010301@ya.ru>
	<CAOGo0VbiP2EgeD6RD-LeNta3R8Df7U6ciGSE2-VsGWFsCws8gg@mail.gmail.com>
	<547F1B0F.5060701@ya.ru>
	<548119B8.7080402@kippdata.de>
	<CAH9fUpact3f9U3KD_-YKSXBLUqX_X5jfrFP=uzs=NEfNrYg2hw@mail.gmail.com>
	<CAH9fUpaH5P4ZVwP_V4Wu2v=xttVaj4VqE7v-xbbsqbmfDdgz3g@mail.gmail.com>
	<CAH9fUpbWLoFAzzjsL9776pxkg2fwQ-qWn-__vv5ca==oCV65ZA@mail.gmail.com>
	<CAH9fUpZvO2BoEN-bDacOQ6-67XJcERR+LPYiz8RqEo_9b234cw@mail.gmail.com>
	<54844A58.6030809@ya.ru>
	<CAH9fUpY_WdG4EziQVxMm9niLN2EeE+j=z+UUxbRUO=TzFmDcVw@mail.gmail.com>
	<54845C22.9080702@ya.ru>
	<CAH9fUpajjW6E_gW5b1zB_tD4SgEDoQ+Kso=gMtkt2NmbC+XHfw@mail.gmail.com>
	<548E9971.5000908@ya.ru>
	<CAH9fUpYy_fxXiY3YTUfFO_3J6Eqyg4oRA5qmjAqAuQbCkywPOQ@mail.gmail.com>
Date: Mon, 15 Dec 2014 09:26:23 -0800
Message-ID: 
 <CAHSHCF7F3=ZC5aShNp02zzew-9HwAzpuz4LO=s9ds-GR5JpCxg@mail.gmail.com>
Subject: Re: Introduce connect times for SampleResult?
From: chaitanya bhatt <bhatt.chaitanya@gmail.com>
To: dev@jmeter.apache.org
Content-Type: multipart/alternative; boundary=001a11331da69e484d050a448a4d

--001a11331da69e484d050a448a4d
Content-Type: text/plain; charset=UTF-8

Andrey,

Kindly let us know what needs to be tested for your feature in the nightly.

Thanks
Chaitanya M Bhatt

On Mon, Dec 15, 2014 at 9:22 AM, Philippe Mouawad <
philippe.mouawad@gmail.com> wrote:
>
> Hi,
> You need to update docs and component_reference.xml or any doc that
> currently mentions other reported indicators, don't forget screenshots if
> any are deprecated.
> Also fill in changes.xml with bug id at the right place, and when commiting
> put in comment the bug ID + the bugzilla full title.
>
> YOu need to commit files all in 1.
> Once done, take the mail your receive with commit and copy the line
> starting after author and just before commit files changes details, and put
> this in the bugzilla that you close as resolved.
>
> Regards
>
>
>
>
>
> On Mon, Dec 15, 2014 at 9:18 AM, Andrey Pokhilko <apc4@ya.ru> wrote:
> >
> > Is there anything else that I should add to commit? Changelog records
> > etc? Is there a document for commit requirements?
> >
> > Andrey Pokhilko
> >
> > On 12/15/2014 12:28 AM, Philippe Mouawad wrote:
> > > Hi,
> > > Sounds ok for commit for me.
> > >
> > > Regards
> > >
> > > On Sunday, December 7, 2014, Andrey Pokhilko <apc4@ya.ru> wrote:
> > >
> > >> Note that I made classes private static, because they seems have no
> use
> > >> outside connection manager. Is it OK?
> > >>
> > >> Andrey Pokhilko
> > >>
> > >> On 12/07/2014 03:37 PM, Philippe Mouawad wrote:
> > >>> On Sun, Dec 7, 2014 at 1:38 PM, Andrey Pokhilko <apc4@ya.ru
> > >> <javascript:;>> wrote:
> > >>>> Hi Philippe,
> > >>>>
> > >>>> This is a great help to have your code review, thanks! I tried to
> fix
> > >>>> following items, please verify:
> > >>>>
> > >>>>   * MeasuringConnectionManager#dnsResolver removed
> > >>>>   * Added Connect time into View Results in Table
> > >>>>   * ATT_CONNECT_TIME changed to ct
> > >>>>   * connectedTime and startedTime are removed, you are right about
> > their
> > >>>>     origin
> > >>>>   * added License header and javadocs to MeasuringConnectionManager
> > >>>>   * fixed NPE source by removing the connManager field
> > >>>>
> > >>>>
> > >>>> The items that I did not fix, but can fix if you will insist on it:
> > >>>>
> > >>>>   * I don't agree that CSV_CONNECT_TIME should be changed to
> > >>>>     "Connection", for me "connect" is short of "connect time", and
> > >>>>     "connection" is about "connection state" or "connection object"
> > >>>>
> > >>> Ok just a matter of opinion :-)
> > >>>
> > >>>>   * I don't think the 'PoolingClientConnectionManagerAdapter' makes
> > our
> > >>>>     code easier to read. The approach to make Java class names more
> > and
> > >>>>     more long has its limits. Finally, ask yourself, what is more
> > >>>>     obvious in this situation - MeasuringConnectionManager or
> > >>>>     PoolingClientConnectionManagerAdapter ?
> > >>>>
> > >>> Ok just a matter of opinion :-)   , I named them after pattern, you
> > name
> > >>> them after their use , ok for me.
> > >>>
> > >>>
> > >>>>   * I did not understand what is wrong with
> MeasuringConnectionRequest
> > >>>>     and MeasuringConnectionRequest classes being inner. What do you
> > >>>>     offer instead?
> > >>>>
> > >>> I am asking to make the public static class instead of public class.
> > >> There
> > >>> is no benefit of making them public class and you hold uselessly a
> > >>> reference to MeasuringConnectionManager instance
> > >>>
> > >>>   * The SampleResult instance variable is the only way to have the
> same
> > >>>>     style as for latency, when you just call "connectEnd". Otherwise
> > we
> > >>>>     need to get connectedTime and startedTime back and use simple
> > >>>>     getter. The only change that might make sense is to use
> > >>>>     WeakReference to shorten the lifetime of SampleResult. As I see
> in
> > >>>>     the code there is no ways currently to share the same HTTPClient
> > >>>>     between threads.
> > >>>>
> > >>> Ok, as I said in the following mail there was no issue except the
> > NPE.  I
> > >>> will check new PR.
> > >>>
> > >>>> Let's discuss, maybe with more opinions from other contributors,
> > what's
> > >>>> should be finally done with these items.
> > >>>>
> > >>>> NPE will be discussed in a different branch of emails.
> > >>>>
> > >>>> Andrey Pokhilko
> > >>>>
> > >>>> On 12/06/2014 04:34 PM, Philippe Mouawad wrote:
> > >>>>> Hi,
> > >>>>> I checked , it seems OK regarding multi-threading.
> > >>>>>
> > >>>>> Few more notes:
> > >>>>> MeasuringConnectionManager#dnsResolver is not used
> > >>>>>
> > >>>>> Shouldn't connectTime be added to View Results in Table ?
> > >>>>>
> > >>>>> Regards
> > >>>>>
> > >>>>>
> > >>>>> On Sat, Dec 6, 2014 at 2:51 PM, Philippe Mouawad <
> > >>>> philippe.mouawad@gmail.com <javascript:;>
> > >>>>>> wrote:
> > >>>>>> Hi,
> > >>>>>> Note there is currently a bugzilla and patch (but partly outdated
> > due
> > >> to
> > >>>>>> HttpSampler having been drastically reworked since that time) for
> > >> this:
> > >>>>>>    - https://issues.apache.org/bugzilla/show_bug.cgi?id=48799
> > >>>>>>
> > >>>>>>
> > >>>>>> Regards
> > >>>>>>
> > >>>>>> On Sat, Dec 6, 2014 at 2:48 PM, Philippe Mouawad <
> > >>>>>> philippe.mouawad@gmail.com <javascript:;>> wrote:
> > >>>>>>
> > >>>>>>> Hi,
> > >>>>>>> My notes:
> > >>>>>>> Naming:
> > >>>>>>> - CSV_CONNECT_TIME , Connect should maybe be named Connection
> > >>>>>>> - ATT_CONNECT_TIME should maybe be ct (for ConnectTime) instead
> of
> > cn
> > >>>>>>> - MeasuringConnectionManager should be
> > >>>>>>> PoolingClientConnectionManagerAdapter ?
> > >>>>>>> - MeasuringConnectionRequest should be
> > >> ClientConnectionRequestAdapter ?
> > >>>>>>> - MeasuredConnection should be
> > >>>>>>> MeasuringConnectionManager is missing Apache License header and
> > >>>> javadocs
> > >>>>>>> :-)
> > >>>>>>>
> > >>>>>>> public class MeasuringConnectionRequest should not be public
> static
> > >>>> class
> > >>>>>>> MeasuringConnectionRequest ?
> > >>>>>>> Same for MeasuredConnection ?
> > >>>>>>>
> > >>>>>>> MeasuredConnection :
> > >>>>>>> connectedTime and startedTime are not used , I suppose it was a
> > work
> > >> in
> > >>>>>>> progress code that was not deleted afterwards.
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> More in depth remarks:
> > >>>>>>> - Are you sure about having SampleResult instance variable of
> > >>>>>>> MeasuringConnectionManager ?
> > >>>>>>> This should be checked as I am afraid you may end up sharing
> > >>>> SampleResult
> > >>>>>>> between different samples and threads.
> > >>>>>>> I need more time to check this.
> > >>>>>>>
> > >>>>>>> Regards
> > >>>>>>> Philippe M.
> > >>>>>>> @philmdot
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> On Fri, Dec 5, 2014 at 9:55 PM, Philippe Mouawad <
> > >>>>>>> philippe.mouawad@gmail.com <javascript:;>> wrote:
> > >>>>>>>
> > >>>>>>>> Nice, will try to review this we.
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> On Friday, December 5, 2014, Rainer Jung <
> rainer.jung@kippdata.de
> > >> <javascript:;>>
> > >>>>>>>> wrote:
> > >>>>>>>>
> > >>>>>>>>> Am 03.12.2014 um 15:15 schrieb Andrey Pokhilko:
> > >>>>>>>>>
> > >>>>>>>>>> Happened to be not so much work:
> > >>>>>>>>>> https://github.com/apache/jmeter/pull/11/files
> > >>>>>>>>>>
> > >>>>>>>>>> Please, review it and point me at any changes needed.
> > >>>>>>>>>>
> > >>>>>>>>> I didn't review the patch but I patched a 2.12 I'm currently
> > using
> > >> to
> > >>>>>>>>> probe a service and it seems to run well here.
> > >>>>>>>>>
> > >>>>>>>>> Regards,
> > >>>>>>>>>
> > >>>>>>>>> Rainer
> > >>>>>>>>>
> > >>>>>>>>>  On 11/29/2014 04:06 PM, sebb wrote:
> > >>>>>>>>>>> On 29 November 2014 at 12:14, Andrey Pokhilko <apc4@ya.ru
> > >> <javascript:;>> wrote:
> > >>>>>>>>>>>> Hi,
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> Many times I see a sence to have connect times measured
> > >>>> separately,
> > >>>>>>>>>>>> in
> > >>>>>>>>>>>> addition to latency that we have in SampleResult. It is
> > >> important
> > >>>>>>>>>>>> when
> > >>>>>>>>>>>> measuring a time for SSL handshake and DNS resolving, when
> > users
> > >>>>>>>>>>>> want to
> > >>>>>>>>>>>> see it separate share in total Response Time.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> Connect time is available as separate metric in Grinder and
> > >>>>>>>>>>>> Yandex.Tank.
> > >>>>>>>>>>>> The latter has following details on response time pars:
> > connect,
> > >>>>>>>>>>>> send,
> > >>>>>>>>>>>> latency, receive. Sometimes some parts are zero, but at
> least
> > >>>> there
> > >>>>>>>>>>>> is a
> > >>>>>>>>>>>> technical possibility to see when it is non-zero. It should
> be
> > >>>> noted
> > >>>>>>>>>>>> that full breakdown would be: dns, connect, send, latency,
> > >>>> receive.
> > >>>>>>>>>>>> Send and receive times are not of great importance, IMO.
> And I
> > >>>> would
> > >>>>>>>>>>>> cope with connect time including DNS resolve time. But
> having
> > >>>> connect
> > >>>>>>>>>>>> time would add interesting aspect on results.
> > >>>>>>>>>>>>
> > >>>>>>>>>>> [I expect DNS resolve time might be very tricky to measure in
> > >> Java]
> > >>>>>>>>>>>  For implementation it will require adding one more property
> > with
> > >>>>>>>>>>>> getters
> > >>>>>>>>>>>> and setters to SampleResult, modifying
> SampleSaveConfiguration
> > >>>> and UI
> > >>>>>>>>>>>> settings to configure saving, using this new field in HTTP
> > >>>> sampler,
> > >>>>>>>>>>>> TCP
> > >>>>>>>>>>>> sampler, maybe there are other samplers that can respect
> this
> > >>>> field.
> > >>>>>>>>>>> The docs would need to be updated to state whether a sampler
> > >>>> supports
> > >>>>>>>>>>> the metric or not.
> > >>>>>>>>>>>
> > >>>>>>>>>>>  As separate question I would raise if latency should not
> > include
> > >>>>>>>>>>>> connect
> > >>>>>>>>>>>> time, for me it sounds logical, but changes existing
> behavior.
> > >>>>>>>>>>>>
> > >>>>>>>>>>> Connect time is currently included in both latency and
> elapsed.
> > >>>>>>>>>>>
> > >>>>>>>>>>> The simplest would be to just add connect as a separate time,
> > but
> > >>>> not
> > >>>>>>>>>>> subtract it from latency or elapsed.
> > >>>>>>>>>>> This would allow further analysis without changing behaviour.
> > >>>>>>>>>>> Maybe add an option to perform the subtraction.
> > >>>>>>>>>>> I don't think we should change the default behaviour.
> > >>>>>>>>>>>
> > >>>>>>>>>>>  Any opinions?
> > >>>>>>>>>>> I can see its use and am not against it, but it needs quite a
> > lot
> > >>>> of
> > >>>>>>>>>>> work to implement fully.
> > >>>>>>>>>>>
> > >>>>>>>>>>>  --
> > >>>>>>>>>>>> Andrey Pokhilko
> > >>>>>>>>>>>>
> > >>>>>>>> --
> > >>>>>>>> Cordialement.
> > >>>>>>>> Philippe Mouawad.
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>> --
> > >>>>>>> Cordialement.
> > >>>>>>> Philippe Mouawad.
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>> --
> > >>>>>> Cordialement.
> > >>>>>> Philippe Mouawad.
> > >>>>>>
> > >>>>>>
> > >>>>>>
> > >>
> >
> >
>
> --
> Cordialement.
> Philippe Mouawad.
>

--001a11331da69e484d050a448a4d--