Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#3439 - Handle removal of ClientResponse#rawStatusCode and bump to Spring Boot to 3.2 #3440

Merged
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ Use subheadings with the "=====" level for adding notes for unreleased changes:
===== Features
* Added support for OpenTelemetry annotations - `WithSpan` and `SpanAttribute` - {pull}3406[#3406]
* Only automatically apply redacted exceptions for Corretto JVM 17-20. Outside that, user should use capture_exception_details=false to workaround the JVM race-condition bug if it gets triggered: {pull}3438[#3438]
* Added support for Spring 6.1 / Spring-Boot 3.2 - {pull}3440[#3440]

[[release-notes-1.x]]
=== Java Agent version 1.x
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@
import co.elastic.apm.agent.rabbitmq.components.batch.BatchListenerComponent;
import co.elastic.apm.agent.rabbitmq.config.BatchConfiguration;
import co.elastic.apm.agent.tracer.configuration.MessagingConfiguration;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.jupiter.api.Disabled;
JonasKunz marked this conversation as resolved.
Show resolved Hide resolved
import org.junit.runner.RunWith;
import org.springframework.amqp.rabbit.core.BatchingRabbitTemplate;
import org.springframework.beans.factory.annotation.Autowired;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@
<artifactId>apm-httpclient-core</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>apm-spring-webflux-common</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-web</artifactId>
Expand All @@ -60,6 +65,11 @@
<artifactId>spring-webflux</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-context</artifactId>
<scope>provided</scope>
</dependency>

<dependency>
<groupId>${project.groupId}</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import co.elastic.apm.agent.tracer.Span;
import co.elastic.apm.agent.tracer.Tracer;
import co.elastic.apm.agent.tracer.reference.ReferenceCountedMap;
import co.elastic.apm.agent.webfluxcommon.SpringWebVersionUtils;
import org.reactivestreams.Subscription;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -74,14 +75,14 @@ public void onNext(T t) {
try {
if (span != null && t instanceof ClientResponse) {
ClientResponse clientResponse = (ClientResponse) t;
int statusCode = clientResponse.rawStatusCode();
int statusCode = SpringWebVersionUtils.getClientStatusCode(clientResponse);
span.withOutcome(ResultUtil.getOutcomeByHttpClientStatus(statusCode));
span.getContext().getHttp().withStatusCode(statusCode);
}
subscriber.onNext(t);
} catch (Throwable e) {
thrown = e;
throw e;
throwException(e);
} finally {
doExit(hasActivated, "onNext", span);
discardIf(thrown != null);
Expand Down Expand Up @@ -192,5 +193,19 @@ private void cancelSpan() {
debugTrace(false, "cancelSpan", span);
}
}
@SuppressWarnings("unchecked")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[For Reviewer] This is suddenly required because onNext doesn't declare checked exceptions anymore. For backwards compatibility reasons, I figured it would be best to still continue throwing unwrapped exceptions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be worth adding a comment to make that a bit more explicit and document which version introduced it.

private static <T extends Throwable> void throwException(Throwable exception, Object dummy) throws T
{
throw (T) exception;
}

/**
* Utility method for throwing a checked exception like an unchecked one.
*/
private static void throwException(Throwable exception)
JonasKunz marked this conversation as resolved.
Show resolved Hide resolved
{
throwException(exception, null);
}


}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package co.elastic.apm.agent.springwebclient;

import co.elastic.apm.agent.sdk.internal.PluginClassLoaderRootPackageCustomizer;

import java.util.Arrays;
import java.util.Collection;

public class WebclientPluginRootPackageCustomizer extends PluginClassLoaderRootPackageCustomizer {
@Override
public Collection<String> pluginClassLoaderRootPackages() {
return Arrays.asList(getPluginPackage(), "co.elastic.apm.agent.webfluxcommon");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
co.elastic.apm.agent.springwebclient.WebclientPluginRootPackageCustomizer
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>co.elastic.apm</groupId>
<artifactId>apm-spring-webflux</artifactId>
<version>1.44.1-SNAPSHOT</version>
</parent>

<artifactId>apm-spring-webflux-common-spring5</artifactId>
<name>${project.groupId}:${project.artifactId}</name>

<properties>
<!-- for licence header plugin -->
<apm-agent-parent.base.dir>${project.basedir}/../../..</apm-agent-parent.base.dir>
<animal.sniffer.skip>true</animal.sniffer.skip>
</properties>

<dependencyManagement>
<dependencies>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-dependencies</artifactId>
<version>${version.spring-boot-2}</version>
<type>pom</type>
<scope>import</scope>
</dependency>
</dependencies>
</dependencyManagement>

<dependencies>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-webflux</artifactId>
<scope>provided</scope>
</dependency>
</dependencies>

</project>
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@
* specific language governing permissions and limitations
* under the License.
*/
package co.elastic.apm.agent.springwebflux;
package co.elastic.apm.agent.webfluxcommon;

import org.springframework.http.HttpStatus;
import org.springframework.http.server.reactive.ServerHttpResponse;
import org.springframework.web.reactive.function.client.ClientResponse;

/**
* This class is compiled with spring-web 5.x, relying on the {@link ServerHttpResponse#getStatusCode()}, which changed in 6.0.0.
Expand All @@ -29,8 +30,13 @@
public class SpringWeb5Utils implements SpringWebVersionUtils.ISpringWebVersionUtils {

@Override
public int getStatusCode(Object response) {
public int getServerStatusCode(Object response) {
HttpStatus statusCode = ((ServerHttpResponse) response).getStatusCode();
return statusCode != null ? statusCode.value() : 200;
}

@Override
public int getClientStatusCode(Object response) {
return ((ClientResponse) response).rawStatusCode();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
* specific language governing permissions and limitations
* under the License.
*/
package co.elastic.apm.agent.springwebflux;
package co.elastic.apm.agent.webfluxcommon;

import org.springframework.web.reactive.function.server.ServerResponse;

import javax.annotation.Nullable;

Expand All @@ -27,8 +29,8 @@
*/
public class SpringWebVersionUtils {

private static final String SPRING_WEB_5_UTILS_CLASS_NAME = "co.elastic.apm.agent.springwebflux.SpringWeb5Utils";
private static final String SPRING_WEB_6_UTILS_CLASS_NAME = "co.elastic.apm.agent.springwebflux.SpringWeb6Utils";
private static final String SPRING_WEB_5_UTILS_CLASS_NAME = "co.elastic.apm.agent.webfluxcommon.SpringWeb5Utils";
private static final String SPRING_WEB_6_UTILS_CLASS_NAME = "co.elastic.apm.agent.webfluxcommon.SpringWeb6Utils";

@Nullable
private static ISpringWebVersionUtils instance = null;
Expand All @@ -40,21 +42,16 @@ private static synchronized void initialize() throws Exception {
return;
}
try {
// check if using spring 6.0.0 or higher
Class.forName("org.springframework.http.HttpStatusCode");
try {
// loading class by name so to avoid linkage attempt when spring-web 6 is unavailable
instance = (ISpringWebVersionUtils) Class.forName(SPRING_WEB_6_UTILS_CLASS_NAME).getDeclaredConstructor().newInstance();
} catch (Exception e) {
throw new IllegalStateException("Spring-web 6.x+ identified, but failed to load related utility class", e);
}
} catch (ClassNotFoundException ignored) {
// assuming spring-web < 6.x
try {
// loading class by name so to avoid linkage attempt on spring-web 6, where the getStatusCode API has changed
instance = (ISpringWebVersionUtils) Class.forName(SPRING_WEB_5_UTILS_CLASS_NAME).getDeclaredConstructor().newInstance();
} catch (Exception e) {
throw new IllegalStateException("Spring-web < 6.x identified, but failed to load related utility class", e);
Class<?> statusCodeType = ServerResponse.class.getMethod("statusCode").getReturnType();
switch (statusCodeType.getName()) {
case "org.springframework.http.HttpStatusCode": // spring 6.0.0 or higher
instance = (ISpringWebVersionUtils) Class.forName(SPRING_WEB_6_UTILS_CLASS_NAME).getDeclaredConstructor().newInstance();
break;
case "org.springframework.http.HttpStatus": // spring < 6
instance = (ISpringWebVersionUtils) Class.forName(SPRING_WEB_5_UTILS_CLASS_NAME).getDeclaredConstructor().newInstance();
break;
default:
throw new IllegalStateException("Unexpected type: "+statusCodeType.getName());
}
} finally {
initialized = true;
Expand All @@ -77,10 +74,27 @@ private static ISpringWebVersionUtils getImplementation() throws Exception {
* expected
* @return the status code of the provided response
*/
public static int getStatusCode(Object response) throws Exception {
public static int getServerStatusCode(Object response) throws Exception {
ISpringWebVersionUtils implementation = getImplementation();
if (implementation != null) {
return implementation.getServerStatusCode(response);
}
return 200;
}


/**
* A utility method to get the status code of a {@code org.springframework.web.reactive.function.client.ClientResponse} from any version
* of Spring framework.
*
* @param response must be of type {@code org.springframework.web.reactive.function.client.ClientResponse}, otherwise an Exception is
* expected
* @return the status code of the provided response
*/
public static int getClientStatusCode(Object response) throws Exception {
ISpringWebVersionUtils implementation = getImplementation();
if (implementation != null) {
return implementation.getStatusCode(response);
return implementation.getClientStatusCode(response);
}
return 200;
}
Expand All @@ -93,6 +107,16 @@ public interface ISpringWebVersionUtils {
* @param response must be of type {@code org.springframework.http.server.reactive.ServerHttpResponse}
* @return the corresponding status code
*/
int getStatusCode(Object response);
int getServerStatusCode(Object response);

/**
* A utility method to get the status code of a {@code org.springframework.web.reactive.function.client.ClientResponse} from any version
* of Spring framework.
*
* @param response must be of type {@code org.springframework.web.reactive.function.client.ClientResponse}
* @return the corresponding status code
*/
int getClientStatusCode(Object response);

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,12 @@
* specific language governing permissions and limitations
* under the License.
*/
package co.elastic.apm.agent.springwebflux;
package co.elastic.apm.agent.webfluxcommon;

import org.junit.jupiter.api.Test;
import org.springframework.http.HttpStatus;
import org.springframework.http.server.reactive.ServerHttpResponse;
import org.springframework.web.reactive.function.client.ClientResponse;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
Expand All @@ -30,14 +31,21 @@
class SpringWeb5UtilsTest {

@Test
void testGetStatusCode() throws Exception {
void testGetServerStatusCode() throws Exception {
ServerHttpResponse mockResponse = mock(ServerHttpResponse.class);
doReturn(HttpStatus.IM_USED).when(mockResponse).getStatusCode();
assertThat(SpringWebVersionUtils.getStatusCode(mockResponse)).isEqualTo(226);
assertThat(SpringWebVersionUtils.getServerStatusCode(mockResponse)).isEqualTo(226);
}

@Test
void testWrongResponseType() {
assertThatThrownBy(() -> SpringWebVersionUtils.getStatusCode(new Object())).isInstanceOf(ClassCastException.class);
assertThatThrownBy(() -> SpringWebVersionUtils.getServerStatusCode(new Object())).isInstanceOf(ClassCastException.class);
}

@Test
void testGetClientStatusCode() throws Exception {
ClientResponse mockResponse = mock(ClientResponse.class);
doReturn(226).when(mockResponse).rawStatusCode();
assertThat(SpringWebVersionUtils.getClientStatusCode(mockResponse)).isEqualTo(226);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>co.elastic.apm</groupId>
<artifactId>apm-spring-webflux</artifactId>
<version>1.44.1-SNAPSHOT</version>
</parent>

<artifactId>apm-spring-webflux-common</artifactId>
<name>${project.groupId}:${project.artifactId}</name>

<properties>
<!-- for licence header plugin -->
<apm-agent-parent.base.dir>${project.basedir}/../../..</apm-agent-parent.base.dir>
<animal.sniffer.skip>true</animal.sniffer.skip>
</properties>

<dependencyManagement>
<dependencies>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-dependencies</artifactId>
<version>${version.spring-boot-3}</version>
<type>pom</type>
<scope>import</scope>
</dependency>
</dependencies>
</dependencyManagement>

<dependencies>
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>apm-spring-webflux-common-spring5</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-webflux</artifactId>
<scope>provided</scope>
</dependency>
</dependencies>

</project>
Loading
Loading