Skip to content

Commit 5979e77

Browse files
ffangffang
committed
[CXF-9206]Duplicated value of 'cxf.server.requests' metric for JAX RS Server (#3010)
[CXF-9206]avoid creating Endpoint MetricContext for JAXRS endpoint (cherry picked from commit 758f6a5) (cherry picked from commit c0ed1ee)
1 parent a77a1a1 commit 5979e77

File tree

5 files changed

+198
-31
lines changed

5 files changed

+198
-31
lines changed

‎rt/features/metrics/src/main/java/org/apache/cxf/metrics/interceptors/AbstractMetricsInterceptor.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,9 +89,22 @@ private void addEndpointMetrics(ExchangeMetrics ctx, Message m) {
8989
ctx.addContext((MetricsContext)o);
9090
}
9191
}
92+
93+
private boolean isJaxRsEndpoint(Endpoint endpoint) {
94+
if (endpoint == null || endpoint.getEndpointInfo() == null
95+
|| endpoint.getEndpointInfo().getBinding() == null) {
96+
return false;
97+
}
98+
return "http://apache.org/cxf/binding/jaxrs"
99+
.equals(endpoint.getEndpointInfo().getBinding().getBindingId());
100+
}
92101

93102
private Object createEndpointMetrics(Message m) {
94103
final Endpoint ep = m.getExchange().getEndpoint();
104+
if (isJaxRsEndpoint(ep)) {
105+
//shouldn't create endpoint context for JAX-RS endpoints
106+
return null;
107+
}
95108
Object o = ep.get(MetricsContext.class.getName());
96109
if (o == null) {
97110
List<MetricsContext> contexts = new ArrayList<>();
@@ -163,6 +176,8 @@ private synchronized Object createMetricsContextForRestResource(Message message,
163176
if (o != null) {
164177
return o;
165178
}
179+
180+
166181
List<MetricsContext> contexts = new ArrayList<>();
167182
for (MetricsProvider p : getMetricProviders(message.getExchange().getBus())) {
168183
MetricsContext c = p.createResourceContext(message.getExchange().getEndpoint(),

‎systests/jaxrs/pom.xml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -565,6 +565,11 @@
565565
<version>${cxf.netty.version}</version>
566566
<scope>test</scope>
567567
</dependency>
568+
<dependency>
569+
<groupId>io.micrometer</groupId>
570+
<artifactId>micrometer-core</artifactId>
571+
<scope>test</scope>
572+
</dependency>
568573
</dependencies>
569574
<build>
570575
<plugins>

‎systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/JaxRsMetricsClientServerTest.java‎

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,173 @@
1+
/**
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.apache.cxf.systest.jaxrs;
20+
21+
import java.util.Arrays;
22+
import java.util.Collections;
23+
import java.util.List;
24+
25+
import jakarta.ws.rs.GET;
26+
import jakarta.ws.rs.Path;
27+
import jakarta.ws.rs.PathParam;
28+
import jakarta.ws.rs.Produces;
29+
import jakarta.ws.rs.core.MediaType;
30+
import org.apache.cxf.jaxrs.JAXRSServerFactoryBean;
31+
import org.apache.cxf.jaxrs.client.JAXRSClientFactoryBean;
32+
import org.apache.cxf.jaxrs.lifecycle.SingletonResourceProvider;
33+
import org.apache.cxf.metrics.MetricsFeature;
34+
import org.apache.cxf.metrics.micrometer.MicrometerMetricsProperties;
35+
import org.apache.cxf.metrics.micrometer.MicrometerMetricsProvider;
36+
import org.apache.cxf.metrics.micrometer.provider.DefaultExceptionClassProvider;
37+
import org.apache.cxf.metrics.micrometer.provider.DefaultTimedAnnotationProvider;
38+
import org.apache.cxf.metrics.micrometer.provider.StandardTags;
39+
import org.apache.cxf.metrics.micrometer.provider.StandardTagsProvider;
40+
import org.apache.cxf.metrics.micrometer.provider.jaxrs.JaxrsOperationTagsCustomizer;
41+
import org.apache.cxf.metrics.micrometer.provider.jaxrs.JaxrsTags;
42+
import org.apache.cxf.testutil.common.AbstractBusClientServerTestBase;
43+
import org.apache.cxf.testutil.common.AbstractBusTestServerBase;
44+
45+
import io.micrometer.core.instrument.MeterRegistry;
46+
import io.micrometer.core.instrument.Timer;
47+
import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
48+
49+
import org.junit.BeforeClass;
50+
import org.junit.Test;
51+
52+
import static org.junit.Assert.assertEquals;
53+
import static org.junit.Assert.assertNotNull;
54+
import static org.junit.Assert.assertTrue;
55+
56+
/**
57+
* Test for CXF-9206: cxf.server.requests metric must not be
58+
* counted twice for JAX-RS server endpoints when MetricsFeature is used.
59+
*
60+
* Root cause: the CXF-9168 fix (de39a25) made createEndpointContext() eagerly
61+
* return a MicrometerServerMetricsContext for all server-side endpoints so that
62+
* requests failing before a BindingOperationInfo is available are still counted.
63+
* For JAX-RS, createResourceContext() already returns its own MetricsContext per
64+
* request, so ExchangeMetrics ended up incrementing cxf.server.requests twice.
65+
*
66+
* The fix skips creating a server context in createEndpointContext() when the
67+
* endpoint uses the JAX-RS binding (JAXRSBindingFactory.JAXRS_BINDING_ID).
68+
*/
69+
public class JaxRsMetricsClientServerTest extends AbstractBusClientServerTestBase {
70+
71+
private static final MeterRegistry METER_REGISTRY = new SimpleMeterRegistry();
72+
private static final String PORT = allocatePort(JaxRsMetricsClientServerTest.class);
73+
74+
75+
@Path("/hello")
76+
public interface HelloService {
77+
@GET
78+
@Path("/{name}")
79+
@Produces(MediaType.TEXT_PLAIN)
80+
String sayHello(@PathParam("name") String name);
81+
}
82+
83+
public static class HelloServiceImpl implements HelloService {
84+
@Override
85+
public String sayHello(String name) {
86+
return "Hello " + name;
87+
}
88+
}
89+
90+
91+
public static class Server extends AbstractBusTestServerBase {
92+
protected void run() {
93+
var jaxrsTags = new JaxrsTags();
94+
var operationsCustomizer = new JaxrsOperationTagsCustomizer(jaxrsTags);
95+
var tagsProvider = new StandardTagsProvider(
96+
new DefaultExceptionClassProvider(), new StandardTags());
97+
var properties = new MicrometerMetricsProperties();
98+
99+
var provider = new MicrometerMetricsProvider(
100+
METER_REGISTRY,
101+
tagsProvider,
102+
List.of(operationsCustomizer),
103+
new DefaultTimedAnnotationProvider(),
104+
properties
105+
);
106+
107+
JAXRSServerFactoryBean sf = new JAXRSServerFactoryBean();
108+
sf.setResourceClasses(HelloServiceImpl.class);
109+
sf.setResourceProvider(HelloServiceImpl.class,
110+
new SingletonResourceProvider(new HelloServiceImpl()));
111+
sf.setAddress("http://localhost:" + PORT + "/");
112+
sf.setFeatures(Arrays.asList(new MetricsFeature(provider)));
113+
sf.create();
114+
}
115+
}
116+
117+
118+
@BeforeClass
119+
public static void startServers() throws Exception {
120+
createStaticBus();
121+
assertTrue("server did not launch correctly", launchServer(Server.class, true));
122+
}
123+
124+
125+
private HelloService createClient() {
126+
JAXRSClientFactoryBean factory = new JAXRSClientFactoryBean();
127+
factory.setServiceClass(HelloService.class);
128+
factory.setAddress("http://localhost:" + PORT + "/");
129+
factory.setHeaders(Collections.singletonMap("Accept", "text/plain"));
130+
return (HelloService) factory.create();
131+
}
132+
133+
134+
/**
135+
* CXF-9206: a single successful JAX-RS request must produce exactly one
136+
* sample in cxf.server.requests (count == 1), not two.
137+
*/
138+
@Test
139+
public void testSuccessfulRequestIsCountedOnce() {
140+
METER_REGISTRY.clear();
141+
142+
String response = createClient().sayHello("world");
143+
assertEquals("Hello world", response);
144+
145+
Timer timer = METER_REGISTRY.find("cxf.server.requests").timer();
146+
assertNotNull("Expected cxf.server.requests timer to be registered", timer);
147+
assertEquals(
148+
"cxf.server.requests count should be 1 for a single request",
149+
1,
150+
timer.count()
151+
);
152+
}
153+
154+
/**
155+
* Two successive requests must accumulate to count == 2, not 4.
156+
*/
157+
@Test
158+
public void testTwoRequestsAreCountedTwice() {
159+
METER_REGISTRY.clear();
160+
161+
HelloService client = createClient();
162+
client.sayHello("alice");
163+
client.sayHello("bob");
164+
165+
Timer timer = METER_REGISTRY.find("cxf.server.requests").timer();
166+
assertNotNull("Expected cxf.server.requests timer to be registered", timer);
167+
assertEquals(
168+
"cxf.server.requests count should be 2 for two requests",
169+
2,
170+
timer.count()
171+
);
172+
}
173+
}

‎systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/metrics/JAXRSClientMetricsTest.java‎

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -65,17 +65,15 @@ public class JAXRSClientMetricsTest {
6565
private MetricsProvider provider;
6666
private MetricsContext operationContext;
6767
private MetricsContext resourceContext;
68-
private MetricsContext endpointContext;
69-
68+
7069
@Before
7170
public void setUp() {
72-
endpointContext = Mockito.mock(MetricsContext.class);
7371
operationContext = Mockito.mock(MetricsContext.class);
7472
resourceContext = Mockito.mock(MetricsContext.class);
7573

7674
provider = new MetricsProvider() {
7775
public MetricsContext createEndpointContext(Endpoint endpoint, boolean asClient, String cid) {
78-
return endpointContext;
76+
return null;
7977
}
8078

8179
public MetricsContext createOperationContext(Endpoint endpoint, BindingOperationInfo boi,
@@ -109,8 +107,6 @@ public void usingClientProxyStopIsCalledWhenServerReturnsNotFound() throws Excep
109107
} finally {
110108
Mockito.verify(resourceContext, times(1)).start(any(Exchange.class));
111109
Mockito.verify(resourceContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
112-
Mockito.verify(endpointContext, times(1)).start(any(Exchange.class));
113-
Mockito.verify(endpointContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
114110
Mockito.verifyNoInteractions(operationContext);
115111
}
116112
}
@@ -135,8 +131,6 @@ public void usingClientStopIsCalledWhenServerReturnsNotFound() throws Exception
135131
} finally {
136132
Mockito.verify(resourceContext, times(1)).start(any(Exchange.class));
137133
Mockito.verify(resourceContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
138-
Mockito.verify(endpointContext, times(1)).start(any(Exchange.class));
139-
Mockito.verify(endpointContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
140134
Mockito.verifyNoInteractions(operationContext);
141135
}
142136
}
@@ -160,8 +154,6 @@ public void usingClientStopIsCalledWhenConnectionIsRefused() throws Exception {
160154
} finally {
161155
Mockito.verify(resourceContext, times(1)).start(any(Exchange.class));
162156
Mockito.verify(resourceContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
163-
Mockito.verify(endpointContext, times(1)).start(any(Exchange.class));
164-
Mockito.verify(endpointContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
165157
Mockito.verifyNoInteractions(operationContext);
166158
}
167159
}
@@ -189,8 +181,6 @@ public void usingClientStopIsCalledWhenServerReturnSuccessfulResponse() throws E
189181
} finally {
190182
Mockito.verify(resourceContext, times(1)).start(any(Exchange.class));
191183
Mockito.verify(resourceContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
192-
Mockito.verify(endpointContext, times(1)).start(any(Exchange.class));
193-
Mockito.verify(endpointContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
194184
Mockito.verifyNoInteractions(operationContext);
195185
}
196186
}
@@ -210,9 +200,6 @@ public void usingWebClientStopIsCalledWhenServerReturnsNotFound() throws Excepti
210200
} finally {
211201
Mockito.verify(resourceContext, times(1)).start(any(Exchange.class));
212202
Mockito.verify(resourceContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
213-
Mockito.verify(endpointContext, times(1)).start(any(Exchange.class));
214-
Mockito.verify(endpointContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
215-
Mockito.verifyNoInteractions(operationContext);
216203
}
217204
}
218205
}

‎systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/metrics/JAXRSServerMetricsTest.java‎

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,7 @@ public class JAXRSServerMetricsTest extends AbstractClientServerTestBase {
6464
private static MetricsProvider provider;
6565
private static MetricsContext operationContext;
6666
private static MetricsContext resourceContext;
67-
private static MetricsContext endpointContext;
68-
67+
6968
@Rule public ExpectedException expectedException = ExpectedException.none();
7069

7170
public static class BookLibrary implements Library {
@@ -98,18 +97,17 @@ public static void main(String[] args) throws Exception {
9897

9998
@BeforeClass
10099
public static void startServers() throws Exception {
101-
endpointContext = Mockito.mock(MetricsContext.class);
102100
operationContext = Mockito.mock(MetricsContext.class);
103101
resourceContext = Mockito.mock(MetricsContext.class);
104102

105103
provider = new MetricsProvider() {
106104
public MetricsContext createEndpointContext(Endpoint endpoint, boolean asClient, String cid) {
107-
return endpointContext;
105+
return null;
108106
}
109107

110108
public MetricsContext createOperationContext(Endpoint endpoint, BindingOperationInfo boi,
111109
boolean asClient, String cid) {
112-
return operationContext;
110+
return null;
113111
}
114112

115113
public MetricsContext createResourceContext(Endpoint endpoint, String resourceName,
@@ -127,7 +125,6 @@ public MetricsContext createResourceContext(Endpoint endpoint, String resourceNa
127125
public void setUp() {
128126
Mockito.reset(resourceContext);
129127
Mockito.reset(operationContext);
130-
Mockito.reset(endpointContext);
131128
}
132129

133130
@Test
@@ -144,8 +141,6 @@ public void usingClientProxyStopIsCalledWhenServerReturnsNotFound() throws Excep
144141
} finally {
145142
Mockito.verify(resourceContext, times(1)).start(any(Exchange.class));
146143
Mockito.verify(resourceContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
147-
Mockito.verify(endpointContext, times(1)).start(any(Exchange.class));
148-
Mockito.verify(endpointContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
149144
Mockito.verifyNoInteractions(operationContext);
150145
}
151146
}
@@ -165,8 +160,6 @@ public void usingClientStopIsCalledWhenServerReturnsNotFound() throws Exception
165160
} finally {
166161
Mockito.verify(resourceContext, times(1)).start(any(Exchange.class));
167162
Mockito.verify(resourceContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
168-
Mockito.verify(endpointContext, times(1)).start(any(Exchange.class));
169-
Mockito.verify(endpointContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
170163
Mockito.verifyNoInteractions(operationContext);
171164
}
172165
}
@@ -186,8 +179,6 @@ public void usingClientStopIsCalledWhenServerReturnSuccessfulResponse() throws E
186179
} finally {
187180
Mockito.verify(resourceContext, times(1)).start(any(Exchange.class));
188181
Mockito.verify(resourceContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
189-
Mockito.verify(endpointContext, times(1)).start(any(Exchange.class));
190-
Mockito.verify(endpointContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
191182
Mockito.verifyNoInteractions(operationContext);
192183
}
193184
}
@@ -203,8 +194,6 @@ public void usingWebClientStopIsCalledWhenServerReturnsNotFound() throws Excepti
203194
} finally {
204195
Mockito.verify(resourceContext, times(1)).start(any(Exchange.class));
205196
Mockito.verify(resourceContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
206-
Mockito.verify(endpointContext, times(1)).start(any(Exchange.class));
207-
Mockito.verify(endpointContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
208197
Mockito.verifyNoInteractions(operationContext);
209198
}
210199
}
@@ -218,8 +207,6 @@ public void usingWebClientStopIsCalledWhenUrlIsNotServed() throws Exception {
218207
expectedException.expect(ProcessingException.class);
219208
client.get().readEntity(Book.class);
220209
} finally {
221-
Mockito.verify(endpointContext, times(1)).start(any(Exchange.class));
222-
Mockito.verify(endpointContext, times(1)).stop(anyLong(), anyLong(), anyLong(), any(Exchange.class));
223210
Mockito.verifyNoInteractions(resourceContext);
224211
Mockito.verifyNoInteractions(operationContext);
225212
}

0 commit comments

Comments
 (0)