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

build: bump org.springframework.boot:spring-boot-dependencies from 2.7.16 to 3.4.0 #3898

Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ public class IndyBootstrap {
static Method bootstrapLoggingMethod;

private static final CallDepth callDepth = CallDepth.get(IndyBootstrap.class);
private static final CallDepth loggingForNestedCall = CallDepth.get("indy-bootstrap-nested-logging");

private static Logger logger() {
// must not be a static field as it would initialize logging before it's ready
Expand Down Expand Up @@ -430,7 +431,12 @@ private static ConstantCallSite internalBootstrap(MethodHandles.Lookup lookup, S
// avoid re-entrancy and stack overflow errors
// may happen when bootstrapping an instrumentation that also gets triggered during the bootstrap
// for example, adding correlation ids to the thread context when executing logger.debug.
logger().warn("Nested instrumented invokedynamic instruction linkage detected", new Throwable());
if (!loggingForNestedCall.isNestedCallAndIncrement()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This caused the tests to timeout due to not making progress. This only happens due to our test instrumentation where instrument java.lang.System to return custom environment variable values.

// We might be unlucky and cause an infinite recurison through logger()
// for this reason we have the loggingForNestedCall in place to prevent this recursion
logger().warn("Nested instrumented invokedynamic instruction linkage detected", new Throwable());
}
loggingForNestedCall.decrement();
return null;
}
String adviceClassName = (String) args[0];
Expand Down Expand Up @@ -498,7 +504,7 @@ private static ConstantCallSite internalBootstrap(MethodHandles.Lookup lookup, S
// When calling findStatic now, the lookup class will be one that is loaded by the plugin class loader
MethodHandle methodHandle = indyLookup.findStatic(adviceInPluginCL, adviceMethodName, adviceMethodType);
return new ConstantCallSite(methodHandle);
} catch (Exception e) {
} catch (Throwable e) {
logger().error(e.getMessage(), e);
return null;
} finally {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ private CallDepth() {
public static CallDepth get(Class<?> adviceClass) {
// we want to return the same CallDepth instance even if the advice class has been loaded from different class loaders
String key = adviceClass.getName();
return get(key);
}

public static CallDepth get(String key) {
CallDepth callDepth = registry.get(key);
if (callDepth == null) {
registry.putIfAbsent(key, new CallDepth());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-dependencies</artifactId>
<version>3.3.4</version>
<version>3.4.0</version>
<type>pom</type>
<scope>import</scope>
</dependency>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
<!-- Import dependency management from Spring Boot -->
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-dependencies</artifactId>
<version>3.3.4</version> <!-- update IT in case of major upgrade -->
<version>3.4.0</version> <!-- update IT in case of major upgrade -->
<type>pom</type>
<scope>import</scope>
</dependency>
Expand Down Expand Up @@ -52,6 +52,7 @@
<dependency>
<groupId>com.squareup.okhttp3</groupId>
<artifactId>okhttp</artifactId>
<version>${version.okhttp}</version>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,13 @@
*/
package co.elastic.apm.agent.springwebflux;

import co.elastic.apm.agent.sdk.weakconcurrent.WeakKeySoftValueLoadingCache;
import co.elastic.apm.agent.tracer.Transaction;
import co.elastic.apm.agent.sdk.logging.Logger;
import co.elastic.apm.agent.sdk.logging.LoggerFactory;
import co.elastic.apm.agent.sdk.weakconcurrent.WeakKeySoftValueLoadingCache;
import co.elastic.apm.agent.tracer.Transaction;
import org.springframework.http.server.reactive.AbstractServerHttpRequest;
import org.springframework.http.server.reactive.ServerHttpRequest;
import org.springframework.http.server.reactive.ServerHttpRequestDecorator;
import org.springframework.web.server.ServerWebExchange;

import javax.annotation.Nullable;
Expand Down Expand Up @@ -60,19 +61,23 @@ public static Transaction<?> getServletTransaction(ServerWebExchange exchange) {

// While the active transaction is the one created by Servlet, it would rely on the fact that we are on the
// same thread as the one that created the transaction, which is an implementation detail.

Transaction<?> transaction = null;

ServerHttpRequest exchangeRequest = exchange.getRequest();
return getTransactionFromRequest(exchangeRequest);
}

@Nullable
private static Transaction<?> getTransactionFromRequest(ServerHttpRequest exchangeRequest) {
if (exchangeRequest instanceof AbstractServerHttpRequest) {
Object nativeRequest = ((AbstractServerHttpRequest) exchangeRequest).getNativeRequest();

// note: attribute name is defined in Servlet plugin and should be kept in sync
transaction = (Transaction<?>) getServletAttribute(nativeRequest, "co.elastic.apm.agent.servlet.ServletApiAdvice.transaction");
return (Transaction<?>) getServletAttribute(nativeRequest, "co.elastic.apm.agent.servlet.ServletApiAdvice.transaction");

} else if (exchangeRequest instanceof ServerHttpRequestDecorator) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this was a case we simply missed before. Starting with spring 3.4.0 the requests seem to be wrapped by some firewall by default.

ServerHttpRequestDecorator decorator = (ServerHttpRequestDecorator) exchangeRequest;
return getTransactionFromRequest(decorator.getDelegate());
}

return transaction;
return null;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,16 @@
package co.elastic.apm.agent.springwebflux;

import co.elastic.apm.agent.AbstractInstrumentationTest;
import co.elastic.apm.agent.common.util.WildcardMatcher;
import co.elastic.apm.agent.configuration.CoreConfigurationImpl;
import co.elastic.apm.agent.impl.context.RequestImpl;
import co.elastic.apm.agent.impl.context.UrlImpl;
import co.elastic.apm.agent.impl.transaction.TransactionImpl;
import co.elastic.apm.agent.tracer.configuration.WebConfiguration;
import co.elastic.apm.agent.impl.error.ErrorCaptureImpl;
import co.elastic.apm.agent.common.util.WildcardMatcher;
import co.elastic.apm.agent.impl.transaction.TransactionImpl;
import co.elastic.apm.agent.springwebflux.testapp.GreetingWebClient;
import co.elastic.apm.agent.springwebflux.testapp.WebFluxApplication;
import co.elastic.apm.agent.testutils.DisabledOnAppleSilicon;
import co.elastic.apm.agent.tracer.configuration.WebConfiguration;
import co.elastic.apm.agent.tracer.metadata.PotentiallyMultiValuedMap;
import org.assertj.core.data.Offset;
import org.junit.jupiter.api.AfterAll;
Expand Down Expand Up @@ -202,7 +202,7 @@ private Predicate<Throwable> expectClientError(int expectedStatus) {
}

@ParameterizedTest
@CsvSource({"GET", "POST", "PUT", "DELETE", "PATCH", "HEAD", "OPTIONS", "TRACE"})
@CsvSource({"GET", "POST", "PUT", "DELETE", "PATCH", "HEAD", "OPTIONS"})
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like in spring 3.4.0 the TRACE method is now rejected by default with 400 BAD_REQUEST. TBH I've so far never seen the TRACE method used in the wild, so imo it is okay to simply remove it from the tests.

void methodMapping(String method) {
client.setHeader("Authorization", BASIC_AUTH_HEADER_VALUE);

Expand Down
2 changes: 1 addition & 1 deletion apm-agent-plugins/apm-spring-webflux/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

<!-- spring boot version to use for dependency management & testing -->
<version.spring-boot-2>2.7.16</version.spring-boot-2>
<version.spring-boot-3>3.3.4</version.spring-boot-3>
<version.spring-boot-3>3.4.0</version.spring-boot-3>
</properties>

<modules>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
<!-- Import dependency management from Spring Boot -->
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-dependencies</artifactId>
<version>3.3.4</version> <!-- must be 3.X to correspond to spring 6 -->
<version>3.4.0</version> <!-- must be 3.X to correspond to spring 6 -->
<type>pom</type>
<scope>import</scope>
</dependency>
Expand Down
2 changes: 1 addition & 1 deletion apm-agent-plugins/apm-spring-webmvc/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
<!-- Import dependency management from Spring Boot -->
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-dependencies</artifactId>
<version>3.3.4</version> <!-- must be 3.X to correspond to spring 6 -->
<version>3.4.0</version> <!-- must be 3.X to correspond to spring 6 -->
<type>pom</type>
<scope>import</scope>
</dependency>
Expand Down
2 changes: 1 addition & 1 deletion integration-tests/spring-boot-3/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-dependencies</artifactId>
<version>3.3.4</version>
<version>3.4.0</version>
<type>pom</type>
<scope>import</scope>
</dependency>
Expand Down
Loading