jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From chaitanya bhatt <bhatt.chaita...@gmail.com>
Subject Re: Introduce connect times for SampleResult?
Date Mon, 15 Dec 2014 17:26:23 GMT
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.
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message