jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Philippe Mouawad <philippe.moua...@gmail.com>
Subject Re: Introduce connect times for SampleResult?
Date Sun, 14 Dec 2014 22:28:31 GMT
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