jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrey Pokhilko <a...@ya.ru>
Subject Re: Introduce connect times for SampleResult?
Date Sun, 07 Dec 2014 13:54:42 GMT
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> 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
>>>> 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> 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> wrote:
>>>>>
>>>>>> Nice, will try to review this we.
>>>>>>
>>>>>>
>>>>>> On Friday, December 5, 2014, Rainer Jung <rainer.jung@kippdata.de>
>>>>>> 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>
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.
>>>>
>>>>
>>>>
>>
>


Mime
View raw message