You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by Andrey Pokhilko <ap...@ya.ru> on 2014/12/03 15:15:43 UTC
Re: Introduce connect times for SampleResult?
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.
Andrey Pokhilko
On 11/29/2014 04:06 PM, sebb wrote:
> On 29 November 2014 at 12:14, Andrey Pokhilko <ap...@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
>>
Re: Introduce connect times for SampleResult?
Posted by Philippe Mouawad <ph...@gmail.com>.
Hi,
There is an issue in code in the following case:
- Use Concurrent Download
You get a NPE due to the fact that connManager is not setup.
This is due to the fact that connManager is initialized in the same block
as HttpClient init , but as httpClient is then cached in a ThreadLocal map
(HTTPCLIENTS) you may end up having HttpClient != null while connManager is.
To reproduce, just check concurrent download , you will get NPE
2014/12/06 15:48:25 WARN - jmeter.protocol.http.sampler.HTTPSamplerBase:
Execution issue when fetching embedded resources
java.util.concurrent.ExecutionException: java.lang.NullPointerException
at java.util.concurrent.FutureTask$Sync.innerGet(FutureTask.java:232)
at java.util.concurrent.FutureTask.get(FutureTask.java:91)
at
org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase.downloadPageResources(HTTPSamplerBase.java:1317)
at
org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase.resultProcessing(HTTPSamplerBase.java:1580)
at
org.apache.jmeter.protocol.http.sampler.HTTPAbstractImpl.resultProcessing(HTTPAbstractImpl.java:338)
at
org.apache.jmeter.protocol.http.sampler.HTTPHC4Impl.sample(HTTPHC4Impl.java:409)
at
org.apache.jmeter.protocol.http.sampler.HTTPSamplerProxy.sample(HTTPSamplerProxy.java:74)
at
org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase.sample(HTTPSamplerBase.java:1141)
at
org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase.sample(HTTPSamplerBase.java:1130)
at
org.apache.jmeter.threads.JMeterThread.process_sampler(JMeterThread.java:431)
at org.apache.jmeter.threads.JMeterThread.run(JMeterThread.java:258)
at java.lang.Thread.run(Thread.java:695)
Caused by: java.lang.NullPointerException
at
org.apache.jmeter.protocol.http.sampler.HTTPHC4Impl.sample(HTTPHC4Impl.java:280)
at
org.apache.jmeter.protocol.http.sampler.HTTPSamplerProxy.sample(HTTPSamplerProxy.java:74)
at
org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase$ASyncSample.call(HTTPSamplerBase.java:1905)
at
org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase$ASyncSample.call(HTTPSamplerBase.java:1)
at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303)
at java.util.concurrent.FutureTask.run(FutureTask.java:138)
at
java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:895)
at
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:918)
at
org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase$1$1.run(HTTPSamplerBase.java:1294)
... 1 more
Regards
Philippe M.
@philmdot
On Sat, Dec 6, 2014 at 3:34 PM, Philippe Mouawad <philippe.mouawad@gmail.com
> 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 <ra...@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 <ap...@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.
>>
>>
>>
>
>
> --
> Cordialement.
> Philippe Mouawad.
>
>
>
--
Cordialement.
Philippe Mouawad.
Re: Introduce connect times for SampleResult?
Posted by sebb <se...@gmail.com>.
On 5 January 2015 at 15:35, Andrey Pokhilko <ap...@ya.ru> wrote:
> What to do with test failure messages like this:
> [java] Loading file testfiles/AssertionTestPlan.jmx and saving it
> back changes its size from 6214 to 6306.
> [java] Number of lines changes from 121 to 123
>
> Should I modify the JMXes in tests?
Depends why the jmx has changed.
It may indicate a code bug, e.g. a new property does not have the
correct default so is being saved to the JMX.
>
> And this:
> [java] 1)
> checkI18n(org.apache.jmeter.resources.PackageTest)junit.framework.AssertionFailedError:
> 1 missing labels, labels missing:Missing labels in
> bundle:org/apache/jmeter/resources/messages_fr.properties
> [java] save_connecttime=Save Connect Time
> [java] table_visualizer_connect=Connect
> [java] view_results_connect_time=Connect Time:
>
> I don't know French, sorry :( And I wonder why it requires it...
If the properties are used anywhere then French translations are
needed (unless they are just numbers and symbols like 99%).
Milamber or Philippe can provide the translation if needed.
> 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 <ap...@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 <ap...@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.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>
>
Re: Introduce connect times for SampleResult?
Posted by Andrey Pokhilko <ap...@ya.ru>.
What to do with test failure messages like this:
[java] Loading file testfiles/AssertionTestPlan.jmx and saving it
back changes its size from 6214 to 6306.
[java] Number of lines changes from 121 to 123
Should I modify the JMXes in tests?
And this:
[java] 1)
checkI18n(org.apache.jmeter.resources.PackageTest)junit.framework.AssertionFailedError:
1 missing labels, labels missing:Missing labels in
bundle:org/apache/jmeter/resources/messages_fr.properties
[java] save_connecttime=Save Connect Time
[java] table_visualizer_connect=Connect
[java] view_results_connect_time=Connect Time:
I don't know French, sorry :( And I wonder why it requires it...
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 <ap...@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 <ap...@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.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>
Re: Introduce connect times for SampleResult?
Posted by Milamber <mi...@apache.org>.
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 <ap...@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 <ap...@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.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>
Re: Introduce connect times for SampleResult?
Posted by Andrey Pokhilko <ap...@ya.ru>.
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 <ap...@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 <ap...@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.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>
Re: Introduce connect times for SampleResult?
Posted by sebb <se...@gmail.com>.
On 15 December 2014 at 17:22, Philippe Mouawad
<ph...@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.
Ideally, but if you forget some files, it's OK to add these in another
commit, so long as the correct Bug id is used, and the Bugzilla issue
is updated.
The intention is to be able to easily find the changes that were made
as a result of a Bugzilla issue.
It's important that a single commit only includes directly related changes.
Unrelated changes should be done in separate commits.
If a commit contains multiple unrelated changes, it's much harder to
review, and is much harder to revert if that becomes necessary.
> 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 <ap...@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 <ap...@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.
Re: Introduce connect times for SampleResult?
Posted by chaitanya bhatt <bh...@gmail.com>.
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 <ap...@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 <ap...@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.
>
Re: Introduce connect times for SampleResult?
Posted by Philippe Mouawad <ph...@gmail.com>.
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 <ap...@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 <ap...@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.
Re: Introduce connect times for SampleResult?
Posted by Andrey Pokhilko <ap...@ya.ru>.
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 <ap...@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.
>>>>>>
>>>>>>
>>>>>>
>>
Re: Introduce connect times for SampleResult?
Posted by Philippe Mouawad <ph...@gmail.com>.
Hi,
Sounds ok for commit for me.
Regards
On Sunday, December 7, 2014, Andrey Pokhilko <ap...@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.
Re: Introduce connect times for SampleResult?
Posted by Andrey Pokhilko <ap...@ya.ru>.
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 <ap...@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 <ra...@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 <ap...@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.
>>>>
>>>>
>>>>
>>
>
Re: Introduce connect times for SampleResult?
Posted by Philippe Mouawad <ph...@gmail.com>.
On Sun, Dec 7, 2014 at 1:38 PM, Andrey Pokhilko <ap...@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 <ra...@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 <ap...@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.
> >>
> >>
> >>
> >
>
>
--
Cordialement.
Philippe Mouawad.
Re: Introduce connect times for SampleResult?
Posted by Andrey Pokhilko <ap...@ya.ru>.
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 <ra...@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 <ap...@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.
>>
>>
>>
>
Re: Introduce connect times for SampleResult?
Posted by Philippe Mouawad <ph...@gmail.com>.
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 <ra...@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 <ap...@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.
>
>
>
--
Cordialement.
Philippe Mouawad.
Re: Introduce connect times for SampleResult?
Posted by Philippe Mouawad <ph...@gmail.com>.
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 <ra...@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 <ap...@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.
Re: Introduce connect times for SampleResult?
Posted by Philippe Mouawad <ph...@gmail.com>.
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 <ra...@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 <ap...@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.
Re: Introduce connect times for SampleResult?
Posted by Philippe Mouawad <ph...@gmail.com>.
Nice, will try to review this we.
On Friday, December 5, 2014, Rainer Jung <ra...@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 <ap...@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.
Re: Introduce connect times for SampleResult?
Posted by Rainer Jung <ra...@kippdata.de>.
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 <ap...@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
Re: Introduce connect times for SampleResult?
Posted by Andrey Pokhilko <ap...@ya.ru>.
No, it is not committed into SVN, you'll need to build it from the sources.
Andrey Pokhilko
On 12/04/2014 07:07 PM, Shmuel Krakower wrote:
> Is it part of any nightly yet?
> I'll give it a try...
>
> www.beatsoo.org - free application performance monitoring from world wide
> locations.
> On Dec 3, 2014 4:17 PM, "Andrey Pokhilko" <ap...@ya.ru> wrote:
>
>> 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.
>>
>> Andrey Pokhilko
>>
>> On 11/29/2014 04:06 PM, sebb wrote:
>>> On 29 November 2014 at 12:14, Andrey Pokhilko <ap...@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
>>>>
>>
Re: Introduce connect times for SampleResult?
Posted by Shmuel Krakower <sh...@gmail.com>.
Is it part of any nightly yet?
I'll give it a try...
www.beatsoo.org - free application performance monitoring from world wide
locations.
On Dec 3, 2014 4:17 PM, "Andrey Pokhilko" <ap...@ya.ru> wrote:
> 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.
>
> Andrey Pokhilko
>
> On 11/29/2014 04:06 PM, sebb wrote:
> > On 29 November 2014 at 12:14, Andrey Pokhilko <ap...@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
> >>
>
>