[CXF-8213] Add Micrometer metric support for JAX-WS

[shark300]

No description provided.

[jhswedeveloper]

How would adding support to JAX-RS look like? Could we reuse parts of the code?

[shark300]

How would adding support to JAX-RS look like? Could we reuse parts of the code?

In theoretically yes you can. But I have a lack of knowledge about JAX-RS. Due to this I don't want to add any code related to that.

[reta]

@junhuhdev The idea is to shape this PR so the foundation is ready for JAX-RS (very likely without introducing JAX-RS support directly as per discussion with @shark300 on the ticket). I should be back on it in a few days to finish up the review.

[jhswedeveloper]

Ok thanks looks promising. Looking forward to testing this implementation.

Btw is support JAX-RS on the roadmap?

[reta]

@junhuhdev certainly, https://issues.apache.org/jira/browse/CXF-8252

[reta]

@shark300 thanks a lot for working on that, it looks great, I just have a few comments, looks very good otherwise.

[shark300]

@reta Thank you, and I'm sorry because fixing review comments takes much time.

[reta]

@shark300 not a problem at all, this is really nice feature, your contribution is greatly appreciated.

[shark300]

@reta I'm not familiar with this project, but I have some questions: who is responsible for documentation update and squashing commits?

[reta]

@shark300 maintainers usually squash commits when merging to master (and sometimes cherry-pick to maintenance branches), the documentation is also updated by maintainers.

[reta]

@shark300 I have no outstanding comments or concerns left, I just need to take some time to verify / play with this feature in different scenarios before merging it in, sounds good? Thanks again for your work.

[shark300]

@reta Thank you for your work too :)

[reta]

Sorry, a few issues I have run into with this method.

1. The computeIfAbsent is known to introduce unnecessary locking on JDK8 in case when entry already exists (see please https://bugs.openjdk.java.net/browse/JDK-8161372). Since we are going to call getTimedAnnotations for each invocation, it is better to avoid these kind if issues if possible.
2. We allocate the empty sets every time for each method that does not have @Timed annotation, we could use Collections.emptySet() singleton instead.

The suggested modifications are below, hope they make sense (the same would apply to DefaultTimedAnnotationProvider)

    public Set<Timed> getTimedAnnotations(Exchange ex) {
        HandlerMethod handlerMethod = new HandlerMethod(ex);

        final Set<Timed> exists = timedAnnotationCache.get(handlerMethod);
        if (exists  != null) {
            return exists;
        }

       return timedAnnotationCache.computeIfAbsent(handlerMethod, method -> {
            Set<Timed> timed = findTimedAnnotations(method.getMethod());
            if (timed.isEmpty()) {
                timed = findTimedAnnotations(method.getBeanType());
            }
            return timed.isEmpty() ? Collections.emptySet() : timed;;
        });
    }

PS: I am fine doing these changes myself after merge as well, please let me know your preferences.

[shark300]

Fixed.

[reta]

@shark300 may I ask you please to:

1. Merge with latest master (seems like some checks are failing, CodeQL / Analyze (java))
2. Run mvn clean verify -Dmaven.test.skip=true -DskipITs=true on this PR, there are some checkstyle violations (it should be covered by Jenkins job but it seems like it was not run for some reasons).

We should be able to merge when checks + checkstyle are "green". I also created the documentation template (https://cwiki.apache.org/confluence/display/CXF20DOC/Micrometer) and would ask your help please to review once ready (if you don't mind).

[shark300]

@reta
PMD and Checkstyle is OK locally.