jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Milamber <milam...@apache.org>
Subject Re: Introduce connect times for SampleResult?
Date Mon, 05 Jan 2015 21:20:02 GMT
Hello,

Thanks for your first commit.

Done for the French missing translation. Andrey don't hesitate to ask
the French translation to me or Philippe (we are 2 French guys :-))
before commit (or just after to avoid some errors with jenkins builds
(the ant distribution task)

Milamber


On 05/01/2015 20:59, Andrey Pokhilko wrote:
> Ok, I commited it without French. But I have no mail after the commit...
>
> Andrey Pokhilko
>
> On 12/15/2014 08:22 PM, Philippe Mouawad 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.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>


Mime
View raw message