From: "Adrian Crum" <adrian.c...@sandglass-software.com>
> I'm going to try one more time to ask nicely, and if that doesn't work,
> then I'm prepared to veto your commit.
>
> You added redundant methods to the existing code that do nothing more
> than duplicate existing functionality.
"duplicate existing functionality. " Not true for 2 reasons
1) recordServiceRate() provides only the serviceRate variable which is then
used to feed the corresponding column in the webtools form.
I need another column to store the request events durations for the chained
requests. Hence a new requestRate variable is used in recordRequestRate(). I
could have called it EventRequestRate to make it more clear, because it really
measures the event duration which for chained requests is the only duration
which makes sense.
2) As says the javadoc, recordServiceRate() implements as default a smoothed
moving average. I wanted something simpler: a simple average, that's what
recordRequestRate() does.
>You are not taking the time to
> understand how to use the feature properly.
I did. You are not taking the time to understand my need. I want to measure
chained request events durations, and I want to use a simple average not a
smoothed moving average.
>Instead, you are being
> argumentative and supporting your view with imaginary users with
> imaginary requirements.
I'm an user and I'm not imaginary. So what I want I believe some other users
would like.
I thought about putting it in the original Stats form which lacks a way to
measure chained requests events duration.
But I found Metrics form easier to read, Metrics implementation easy to
understand and using only memory a plus
> Please revert your commit, then we can discuss this further.
If I revert I will lose the functionallity I want (chained request events
durations measured with a simple average) and with the current code it can't be
implemented.
I understand that you want to keep the Metrics classes as they are now but I
did not change it, just added what I needed
What I could to if you want is below but I'd then lose the simple average
calculation and things would get blurred rather than simplified.
Index: base/src/org/ofbiz/base/metrics/Metrics.java
===================================================================
--- base/src/org/ofbiz/base/metrics/Metrics.java (revision 1484069)
+++ base/src/org/ofbiz/base/metrics/Metrics.java (working copy)
@@ -76,12 +76,11 @@
void recordServiceRate(int numEvents, long time);
/**
- * Records the request time for <code>numEvents</code> taking
- * <code>time</code> milliseconds to be processed.
- */
- void recordRequestRate(int numEvents, long time);
+ * Records in varName the time for <code>numEvents</code> taking
+ * <code>time</code> milliseconds to be processed.
+ */
+ void recordServiceRate(int numEvents, long time, String varName);
/** Resets all metrics. */
void reset();
-
}
\ No newline at end of file
Index: base/src/org/ofbiz/base/metrics/MetricsFactory.java
===================================================================
--- base/src/org/ofbiz/base/metrics/MetricsFactory.java (revision 1484069)
+++ base/src/org/ofbiz/base/metrics/MetricsFactory.java (working copy)
@@ -209,6 +209,10 @@
@Override
public synchronized void recordServiceRate(int numEvents, long time) {
+ recordServiceRate(numEvents, time, "serviceRate");
+ }
+
+ public synchronized void recordServiceRate(int numEvents, long time,
String varName) {
totalEvents += numEvents;
cumulativeEvents += numEvents;
totalServiceTime += time;
@@ -219,20 +223,18 @@
totalEvents = 1;
}
double rate = totalServiceTime / totalEvents;
- serviceRate = (rate * smoothing) + (serviceRate * (1.0 -
smoothing));
+ rate = (rate * smoothing) + (serviceRate * (1.0 - smoothing));
+ if ("serviceRate".equals(varName)) {
+ serviceRate = rate;
+ } else if ("requestRate".equals(varName)) {
+ requestRate = rate;
+ }
count = 0;
lastTime = curTime;
totalEvents = totalServiceTime = 0;
}
}
- public synchronized void recordRequestRate(int numEvents, long time) {
- totalEvents += numEvents;
- cumulativeEvents += numEvents;
- totalServiceTime += time;
- requestRate = totalServiceTime / cumulativeEvents;
- }
-
@Override
public synchronized void reset() {
serviceRate = 0.0;
@@ -276,11 +278,11 @@
}
@Override
- public void recordServiceRate(int numEvents, long time) {
+ public void recordServiceRate(int numEvents, long time, String
varName) {
}
@Override
- public void recordRequestRate(int numEvents, long time) {
+ public void recordServiceRate(int numEvents, long time) {
}
@Override
Index: webapp/src/org/ofbiz/webapp/control/RequestHandler.java
===================================================================
--- webapp/src/org/ofbiz/webapp/control/RequestHandler.java (revision 1484069)
+++ webapp/src/org/ofbiz/webapp/control/RequestHandler.java (working copy)
@@ -423,7 +423,7 @@
System.currentTimeMillis() - eventStartTime,
userLogin);
}
if (requestMap.metrics != null) {
- requestMap.metrics.recordRequestRate(numEvent,
System.currentTimeMillis() - eventStartTime);
+ requestMap.metrics.recordServiceRate(numEvent,
System.currentTimeMillis() - eventStartTime, "requetRate");
numEvent = 0;
}
Also I'd need to introduce a metricsSmoothingFactor properties in
serverstats.properties to avoid the burden of putting smoothing="1" everywhere
I'd need in controllers.
Anyway event with that I'd not have simple average calculation or would need to
have also a metricsEstimationSize properties in serverstats.properties (set to
a very high value in my case)
I think we can discuss changes without reverting all, can't we?
Jacques
>
> -Adrian
>
> On 5/17/2013 6:18 PM, Jacques Le Roux wrote:
>> From: "Adrian Crum" <adrian.c...@sandglass-software.com>
>>> Agreed. You added something that is unnecessary. Please remove it.
>> I don't agree it's unnecessary, it does measure the request events durations
>> for chained request and that's quite useful.
>> Without this new column the measure for chained requests does not correspond
>> to the reality
>> You just need to add the average of chained requests measures of a request
>> to be persuaded.
>> So this new functionality completes the measures for chained requests.
>> It does not use moving average, that could be discussed, but is not
>> necessary IMO.
>>
>> Jacques
>>
>>> Metrics.java does not need to be changed. If you want to change what is
>>> being measured, then apply Metrics java in a different way, but do not
>>> change it.
>>>
>>> -Adrian
>>>
>>> On 5/17/2013 5:45 PM, Jacques Le Roux wrote:
>>>> From: "Adrian Crum" <adrian.c...@sandglass-software.com>
>>>>> What you believe users want and what the feature was designed to do seem
>>>>> to be entirely different.
>>>>>
>>>>> The metric is intended to measure the entire request. If you want
>>>>> something different, then feel free to modify your local copy.
>>>> I believe some users will be interested by what I added. I did not remove
>>>> anything!
>>>>
>>>> Jacques
>>>>
>>>>> -Adrian
>>>>>
>>>>> On 5/17/2013 5:25 PM, Jacques Le Roux wrote:
>>>>>> From: "Adrian Crum" <adrian.c...@sandglass-software.com>
>>>>>>> The real request duration is meaningless. If a garbage collection occurs
>>>>>>> during the request, the measurement will be artificially high. That's
>>>>>>> the whole point of the smoothing - it factors out transient performance
>>>>>>> differences.
>>>>>>>
>>>>>>> Please don't break the design. If you don't like the way it works, then
>>>>>>> create your own version by extending it.
>>>>>> To keep things clear. The curent design is good for services but not for
>>>>>> requests: it does not measure the time passed by the event but the whole
>>>>>> request. I believe much users would be interested by the 1st.
>>>>>>
>>>>>> Jacques
>>>>>>
>>>>>>> -Adrian
>>>>>>>
>>>>>>> On 5/17/2013 4:41 PM, Jacques Le Roux wrote:
>>>>>>>> What I wanted to add by
>>>>>>>>
>>>>>>>>> Just thinking I did not use the
>>>>>>>> was that I did not use the moving average (I forgot to remove this
>>>>>>>> part after).
>>>>>>>> Actually I had no needs for it and it's already covered by
>>>>>>>> recordServiceRate() anyway.
>>>>>>>>
>>>>>>>> The pb with current position of recordServiceRate() in RequestHandler
>>>>>>>> is it does not measure the real request event duration. That's what I
>>>>>>>> was interested by, and I believe much users. Having a moving average
>>>>>>>> is secondary for me.
>>>>>>>>
>>>>>>>> Jacques
>>>>>>>>
>>>>>>>> From: "Jacques Le Roux" <jacques.le.r...@les7arts.com>
>>>>>>>>> Hi Adrian,
>>>>>>>>>
>>>>>>>>> At r1483822 I have improved it a bit for requests. I was more
>>>>>>>>> interested in chained ones, it works for all. It's a new colums in
>>>>>>>>> Metrics page which measure the request event duration. Just thinking
>>>>>>>>> I did not use the
>>>>>>>>> I wonder if the use of recordServiceRate() in doRequest() makes
>>>>>>>>> sense. Let me know if you think we should adapt it for the threshold
>>>>>>>>> functionnality using the event duration of it's it fine as is.
>>>>>>>>>
>>>>>>>>> Also let me know if you want me to create the wiki page when we will
>>>>>>>>> be done.
>>>>>>>>>
>>>>>>>>> Cheers
>>>>>>>>>
>>>>>>>>> Jacques
>>>>>>>>>
>>>>>>>>> From: "Adrian Crum" <adrian.c...@sandglass-software.com>
>>>>>>>>>> Anything you can do to improve the implementation will be
>>>>>>>>>> appreciated.
>>>>>>>>>> Sorry I haven't had more time to document it.
>>>>>>>>>>
>>>>>>>>>> -Adrian
>>>>>>>>>>
>>>>>>>>>> On 4/19/2013 12:01 PM, Jacques Le Roux wrote:
>>>>>>>>>>> OK, I guess the smoothing factor explains the tiny differences for
>>>>>>>>>>> services
>>>>>>>>>>> But for chained requests it makes no sense.
>>>>>>>>>>> The metrics report ten more than in log.
>>>>>>>>>>> I will have a look because it's a fine tool for chained requests
>>>>>>>>>>> duration which are not availabe in the stats feature
>>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>>>
>>>>>>>>>>> Jacques
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Adrian Crum wrote:
>>>>>>>>>>>> Keep in mind the metrics provide a moving average. Also, there is a
>>>>>>>>>>>> smoothing factor that can affect the value.
>>>>>>>>>>>>
>>>>>>>>>>>> The smoothing factor is important. Let's say you are using a
>>>>>>>>>>>> monitoring
>>>>>>>>>>>> tool to monitor a particular request. If that request has a very
>>>>>>>>>>>> brief
>>>>>>>>>>>> increase in response time, you don't want to be alerted. Instead,
>>>>>>>>>>>> you
>>>>>>>>>>>> want to know when the increase in response time is sustained over a
>>>>>>>>>>>> period of time. So, the smoothing factor helps prevent "false
>>>>>>>>>>>> positives"
>>>>>>>>>>>> in a way.
>>>>>>>>>>>>
>>>>>>>>>>>> Another scenario: You have an eCommerce site and you put a metric
>>>>>>>>>>>> on the
>>>>>>>>>>>> store's landing page and you configure it with a threshold value
>>>>>>>>>>>> and an
>>>>>>>>>>>> alternate view - one that doesn't include a shopping cart.
>>>>>>>>>>>> Normally,
>>>>>>>>>>>> customers land on the normal page that includes a shopping cart.
>>>>>>>>>>>> But a
>>>>>>>>>>>> new promotion causes customers to swamp the server and the shopping
>>>>>>>>>>>> experience degrades. The metric will smooth the response time
>>>>>>>>>>>> values so
>>>>>>>>>>>> the alternate (non-shopping cart) landing page is not offered too
>>>>>>>>>>>> soon.
>>>>>>>>>>>> But if the server is swamped for an extended period, then new
>>>>>>>>>>>> visitors
>>>>>>>>>>>> will get the alternate page and be able to browse the site without
>>>>>>>>>>>> being
>>>>>>>>>>>> able to order anything. As the request load decreases, the metric
>>>>>>>>>>>> will
>>>>>>>>>>>> smooth the response times so the alternate landing page is not
>>>>>>>>>>>> removed
>>>>>>>>>>>> too soon. When the landing page response time drops below the
>>>>>>>>>>>> configured
>>>>>>>>>>>> threshold, all visitors will receive the normal shopping
>>>>>>>>>>>> experience.
>>>>>>>>>>>>
>>>>>>>>>>>> -Adrian
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 4/19/2013 10:53 AM, Jacques Le Roux wrote:
>>>>>>>>>>>>> Actually r1361725 was also needed for services metrics to work.
>>>>>>>>>>>>> For services I get slightly higher results than in log.
>>>>>>>>>>>>> It's more consistent than for chained requests which are plain
>>>>>>>>>>>>> false.
>>>>>>>>>>>>> I did not get a chance to look at code yet...
>>>>>>>>>>>>>
>>>>>>>>>>>>> Jacques
>>>>>>>>>>>>>
>>>>>>>>>>>>> Jacques Le Roux wrote:
>>>>>>>>>>>>>> From: "Jacques Le Roux" <jacques.le.r...@les7arts.com>
>>>>>>>>>>>>>>> Thanks Adrian,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Yes I saw the threshold idea in Java comments
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Looks like an interesting tool
>>>>>>>>>>>>>>> BTW for the other statistics (I never used it), do you know if
>>>>>>>>>>>>>>> chained requests are taken into account?
>>>>>>>>>>>>>> Hi Adrian,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I can answer now that chained requests are not taken into
>>>>>>>>>>>>>> account by the Statistics feature.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I also noticed something weird in Metrics feature. The total
>>>>>>>>>>>>>> duration of the chained requests and the duration of the initial
>>>>>>>>>>>>>> calling request don't match And when I compare durations in log
>>>>>>>>>>>>>> with those in Metrics I find those in metrics much higher in
>>>>>>>>>>>>>> some cases, and always higher.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Do I misinterpret them? Did you use/check this feature for
>>>>>>>>>>>>>> chained requests?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Maybe I should wait your wiki page?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Jacques
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Still, can you confirm only if 1361722 and 1361724 the only
>>>>>>>>>>>>>>> commits? I guess there is no Jira?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Jacques
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> From: "Adrian Crum" <adrian.c...@sandglass-software.com>
>>>>>>>>>>>>>>>> I will create a Wiki page this weekend.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> The metric name can be anything - the example has a URL:
>>>>>>>>>>>>>>>> prefix to help
>>>>>>>>>>>>>>>> distinguish request metrics from service metrics. You can put
>>>>>>>>>>>>>>>> anything
>>>>>>>>>>>>>>>> you want there.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> The request-map can have an additional <response> element -
>>>>>>>>>>>>>>>> which will
>>>>>>>>>>>>>>>> direct the servlet to an alternate view if the metric crosses a
>>>>>>>>>>>>>>>> threshold. I think the JavaDocs explain that.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> The basic idea is to put metrics in places where you
>>>>>>>>>>>>>>>> anticipate heavy
>>>>>>>>>>>>>>>> loads or bottlenecks, then use a third-party
>>>>>>>>>>>>>>>> reporting/monitoring tool
>>>>>>>>>>>>>>>> to read the metrics.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> -Adrian
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 4/11/2013 8:36 AM, Jacques Le Roux wrote:
>>>>>>>>>>>>>>>>> Thanks Atul,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> So you mean you simply need to put a token (like webapp name)
>>>>>>>>>>>>>>>>> before the request name in this metric name? Did you try it
>>>>>>>>>>>>>>>>> with another request?
>>>>>>>>>>>>>>>>> I see it works on trunk demo and OOTB locally. But I can't
>>>>>>>>>>>>>>>>> get it to work on a custom app I have patched, so maybe I miss
>>>>>>>>>>>>>>>>> something I will wait Adrian's answer about that
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Jacques
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> From: "Atul Vani" <atul.v...@hotwaxmedia.com>
>>>>>>>>>>>>>>>>>> I think that's just the name, to recognize it among list of
>>>>>>>>>>>>>>>>>> several
>>>>>>>>>>>>>>>>>> others, when there will be lots of them. A certain
>>>>>>>>>>>>>>>>>> convention is used to
>>>>>>>>>>>>>>>>>> specify that it is for URL or any service. Then again same
>>>>>>>>>>>>>>>>>> names request
>>>>>>>>>>>>>>>>>> mappings can exist in separate webapps, so a leading
>>>>>>>>>>>>>>>>>> appname, in this case
>>>>>>>>>>>>>>>>>> 'webtools'. It can be changed to anything.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On Thu, 11 Apr 2013 00:47:26 +0530, Jacques Le Roux
>>>>>>>>>>>>>>>>>> <jacques.le.r...@les7arts.com> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Hi Adrian,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Let me know if you will, else I might do it...
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> BTW, are 1361722 and 1361724 the only commit? Is there a
>>>>>>>>>>>>>>>>>>> Jira?
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Could you explain just a bit more the request syntax, sorry
>>>>>>>>>>>>>>>>>>> but the OOTB
>>>>>>>>>>>>>>>>>>> example is a bit confusing.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> + <request-map uri="ViewMetrics">
>>>>>>>>>>>>>>>>>>> + <security https="true" auth="true"/>
>>>>>>>>>>>>>>>>>>> + <metric name="URL: webtools/ViewMetrics" /><!--
>>>>>>>>>>>>>>>>>>> Here for
>>>>>>>>>>>>>>>>>>> demonstration -->
>>>>>>>>>>>>>>>>>>> + <response name="success" type="view"
>>>>>>>>>>>>>>>>>>> value="ViewMetrics"/>
>>>>>>>>>>>>>>>>>>> + </request-map>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> What would it be for another than ViewMetrics?
>>>>>>>>>>>>>>>>>>> I mean why "URL: webtools/ViewMetrics"?
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Jacques
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> From: "Jacques Le Roux" <jacques.le.r...@les7arts.com>
>>>>>>>>>>>>>>>>>>>> Great, thanks Adrian!
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Maybe a wiki page using
>>>>>>>>>>>>>>>>>>>> http://markmail.org/message/x4lzvda66ju6gdg5
>>>>>>>>>>>>>>>>>>>> would help to remember the commands?
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Jacques
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> From: "Adrian Crum" <adrian.c...@sandglass-software.com>
>>>>>>>>>>>>>>>>>>>>> On 4/7/2013 6:38 PM, Adrian Crum wrote:
>>>>>>>>>>>>>>>>>>>>>> On 4/7/2013 12:11 PM, Jacques Le Roux wrote:
>>>>>>>>>>>>>>>>>>>>>>> Jacques Le Roux wrote:
>>>>>>>>>>>>>>>>>>>>>>>> From: "Adrian Crum"
>>>>>>>>>>>>>>>>>>>>>>>> <adrian.c...@sandglass-software.com>
>>>>>>>>>>>>>>>>>>>>>>>>> Have you considered using the metrics feature?
>>>>>>>>>>>>>>>>>>>>>>>> Ha forgot to ask about it, now I see and remember this
>>>>>>>>>>>>>>>>>>>>>>>> thread and I
>>>>>>>>>>>>>>>>>>>>>>>> will digg in
>>>>>>>>>>>>>>>>>>>>>>>> http://markmail.org/message/x4lzvda66ju6gdg5
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Of course, I'd not be against a brief briefing, or a
>>>>>>>>>>>>>>>>>>>>>>>> link to
>>>>>>>>>>>>>>>>>>>>>>>> explain.
>>>>>>>>>>>>>>>>>>>>>>> What I mostly miss is where are the metrics in webtools?
>>>>>>>>>>>>>>>>>>>>>> Oops, I forgot to commit that part. I will take care of
>>>>>>>>>>>>>>>>>>>>>> it.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Actually, I did commit it:
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> https://localhost:8443/webtools/control/ViewMetrics
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> -Adrian
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>>>>> Using Opera's revolutionary email client:
>>>>>>>>>>>>>>>>>> http://www.opera.com/mail/
>
