[jbossws-dev] Fwd: EndpointMetrics Refactor
Alessio Soldano
asoldano at redhat.com
Tue Jul 8 06:45:28 EDT 2014
Hi Jim,
here is the feedback on the metrics stuff; please note it's a general
review on the topic, including both stuff you just implemented and stuff
being already there in cxf: I'm just trying to evaluate what's there atm
and see if it fits our needs.
ResponseTime(*)Interceptor
--------------------------------------
* The AbstractResponseTimeInterceptor#getServiceCounterName method will
be using the counter name we set in the EndpointAssociationInterceptor,
good. However, from a CXF point of view, the name could be computed each
time, so the code doing that needs to be decently optimized. In
particular, the StringBuilder should be properly used there; if there's
a StringBuilder being used, you should really avoid string
concatenations to add stuff in the builder. The JDK optimizer will
replace the string concatenations with additional StringBuilder
instances, which is a waste of resources. So basically use multiple
append() invocations instead of String concatenations (unless the String
constants only are being concatenated).
* same reasoning for
AbstractResponseTimeInterceptor#getOperationCounterName, plus please use
StringBuilder instead of StringBuffer, as that's not accessed concurrently.
* In all ResponseTimeMessage(*)Interceptor, the check on "forceDisabled"
should come before the isServiceCounterEnabled() invocation, as that's
expensive. So basically do "if !forceDisabled &&
(isServiceCounterEnabled(ex))" instead of "if
(isServiceCounterEnabled(ex) && !forceDisabled)"
ResponseTimeCounter
-------------------------------
* Generally speaking I have some concerns on the thread safety of this
class, in particular on visibility. Shouldn't we have final or volatile
attributes?
* At line 87, did you really want to use "|" or "||" ?
* Is it possible to end up with handlingTime = 0 for oneway messages and
hence end up setting min handling time to 0 ? Is that reasonable ?
(probably no...)
* the update of min and max handling time in not performed in a thread
safe way; please have a look at how that should possible be done in our
former EndpointMetricImpl (see updateMin / updateMax in [1])
* it is possible to get unreliable values for the average response time
in highly concurrent environments; we might decide to accept this or
solve as we did in our EndpointMetricsImpl (see the usage or read/write
lock and related comment in [1])
EndpointAssociationInterceptor
-------------------------------------------
* We should probably set the service counter name in the exchange only
when the statistics are enabled.
EndpointMetricsCXFAdapterImpl
--------------------------------------------
* counterRepo and counter members should be final, while there's no need
for keeping a reference to the objectName
Thanks
Alessio
[1]
http://anonsvn.jboss.org/repos/jbossws/common/tags/jbossws-common-2.3.0.Final/src/main/java/org/jboss/ws/common/management/EndpointMetricsImpl.java
On 07/07/14 10:31, Jim Ma wrote:
> I've checked in all things related this change for your review :
>
> https://svn.jboss.org/repos/jbossws/spi/branches/metrics
> https://svn.jboss.org/repos/jbossws/common/branches/metrics
> https://svn.jboss.org/repos/jbossws/stack/cxf/branches/metrics
> https://github.com/jimma/wildfly/tree/metric
> All the cxf change are merged : in
> https://git-wip-us.apache.org/repos/asf/cxf.git
>
> If you have any concerns and comments, please let me know.
>
> Thanks,
> Jim
>
> On 03/07/14 11:35, Jim Ma wrote:
>> Thanks for all your input, Alessio. I've already created prototype code,
>> will create some workspace and check in later.
>>
>> On 07/02/2014 10:52 PM, Alessio Soldano wrote:
>>> Hi Jim,
>>>
>>>> At the beginning, I'd like to quickly list the cons of the current
>>>> EndpointMetrics
>>>> implementation.
>>>> - Because the statistics is fulfilled in RequestHandlerImpl , after
>>>> the
>>>> response is successfully
>>>> returned to receiver. The response related metric value
>>>> could not
>>>> be updated in time, more
>>>> details pleas see JBWS-3808
>>>> - It doesn't count the one way fault correctly.
>>> yep, we need more consistent values to be returned by metric queries.
>>>
>>>> - It doesn't expose all the attribute in jmx.
>>> This is not necessarily a requirement, but a nice to have.
>>>
>>>> - If the cxf's management is enabled, user will get two different
>>>> statistics value. One is
>>>> calculated with jbossws-cxf's endpointmetrics which happens in
>>>> RequestHandler(servlet),
>>>> and another one is calculated with
>>>> ResponseTimeInterceptor(cxf's
>>>> interceptor).
>>> right, and this is possibly confusing users, while we could rely on the
>>> existing cxf mechanism...
>>>
>>>
>>>> The pros of current implementation :
>>>> - It gives more accurate start time for each request.
>>>> - It is simple and doesn't need to add more interceptor/extra
>>>> code in
>>>> the runtime to collect
>>>> the data.
>>> In any case, regardless of which solution we eventually end up relying
>>> on, we need to make the data collection as light as possible and, more
>>> important, we need to make sure you can disable statistics and get no
>>> performance degradation at all.
>>>
>>>> >From the above analysis, one important thing we need to do is make
>>>> the
>>>> EndpintMetrics seamlessly
>>>> work with cxf's management intercetptor and get statistics from one
>>>> place instead of generate two
>>>> copies from different implementation.
>>> +1
>>>
>>>> There is CounterRepostiry extension in cxf which responsible for
>>>> creating ResponseTimeCounter
>>>> for each endpoint, It can be the only source we get statistics for
>>>> each
>>>> endpoint.
>>>> To get metric value from this class, we need to change following
>>>> things:
>>>> 1. Each ResponseTImeCounter(something similar to EndpointMetrics) is
>>>> lazily created in
>>>> message interceptor and put in CounterReposiory with a long
>>>> ObjectName at the moment. We
>>>> need to change it and add the ResponseTimeCounter for each
>>>> jbossws
>>>> endpoint , then put it in
>>>> CounterRepostiry with key name of endpoint name.
>>>> 2. Change EndpointMetricsFactory abstract class to create with
>>>> EndpointMetric with endpoint
>>>> parameter so the endpoint name can be used to create
>>>> ResponseTimeCounter.
>>>> 3. Create new EndpointMetricImpl in jbossws-cxf and get metric value
>>>> from ResponseCounter
>>>> which is created for each endpoint.
>>>> 4. Change the ResponseTimeInterceptor and allow for define the counter
>>>> object name.
>>>>
>>>> Besides this , I also need to refactor the ResponseTimeInterceptor to
>>>> make it log the process time
>>>> more accurate(by move ResponseTimeInInterceptor before several
>>>> interceptors ?). And also refactor
>>>> the ResponseTimeInterceptor a bit to handle the fault message
>>>> correctly.
>>> The plan might work afaics here. The details are probably the core of
>>> the issue ;-) I think you can provide a prototype and we can have a
>>> look
>>> together. Just always keep in mind my comment above regarding
>>> performance and being able to disable the stats at runtime to have full
>>> perf.
>>>
>>> Cheers
>>> Alessio
>>>
>> _______________________________________________
>> jbossws-dev mailing list
>> jbossws-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/jbossws-dev
>
--
Alessio Soldano
Web Service Lead, JBoss
More information about the jbossws-dev
mailing list