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 12:38:48 GMT
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"
  * 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 ?
  * I did not understand what is wrong with MeasuringConnectionRequest
    and MeasuringConnectionRequest classes being inner. What do you
    offer instead?
  * 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.

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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message