elastic / apm-agent-java

https://www.elastic.co/guide/en/apm/agent/java/current/index.html
Apache License 2.0
567 stars 321 forks source link

Add support for Spring 6.x and Spring Boot 3.x #2942

Closed eyalkoren closed 1 year ago

eyalkoren commented 1 year ago

Spring 6 and Spring Boot 3 are GA, requiring us to do some changes in our instrumentations and create tests. Following are major changes in those frameworks and the corresponding required changes in the agent:

This issue is currently blocked on #2936. Once we know how we want our structure to facilitate Java 17 modules, we can unblock and prioritize this one.

Checklist:

JonasKunz commented 1 year ago

Adjust Spring RestTemplate tests to test multiple versions, including Spring 6.x (may require adding proper testing for org.apache.httpcomponents.client5.httpclient5 as well). Tests mostly pass by upgrading to Spring 6 and httpclient to httpclient5

Isn't this already handled by #2920 ? What is missing there?

eyalkoren commented 1 year ago

Isn't this already handled by https://github.com/elastic/apm-agent-java/pull/2920 ? What is missing there?

@JonasKunz it is, thanks, I updated the comment above accordingly. However, what do you think of changing the current version definition to not stop at 7.0.0? Instead, I would add a test for 6.0.0 and another one to fail if needed as soon as an unsupported version is released, for example - [6.0.3,)

JonasKunz commented 1 year ago

However, what do you think of changing the current version definition to not stop at 7.0.0?

Makes sense imo

geiliev commented 1 year ago

Is there a workaround for Spring MVC instrumentation at the moment?

Edit: If anyone is interested I used a quick aspect workaround:

@Component
@Aspect
public class ApmTransactionNameWorkaroundAspect {

    @Pointcut("@annotation(org.springframework.web.bind.annotation.GetMapping) " +
        "|| @annotation(org.springframework.web.bind.annotation.PostMapping) " +
        "|| @annotation(org.springframework.web.bind.annotation.PutMapping) " +
        "|| @annotation(org.springframework.web.bind.annotation.PatchMapping) " +
        "|| @annotation(org.springframework.web.bind.annotation.DeleteMapping)")
    public void allControllerMethods() {
    }

    @Before("allControllerMethods()")
    public void updateTransactionName(JoinPoint jp) {
        String className = jp.getSignature().getDeclaringType().getSimpleName();
        String methodName = jp.getSignature().getName();
        ElasticApm.currentTransaction().setName(String.format("%s#%s", className, methodName));
    }
}
tolsam commented 1 year ago

Hello,

when will this going to be fixed? Spring Boot is the most used framework of all I believe.

JonasKunz commented 1 year ago

We are actively working on it. It won't make it to the next release (which will happen most likely next week), but we are targeting the releaser after that.

shubhu934 commented 1 year ago

Hello @JonasKunz Hope you are doing well. Would you be able to provide the timeline of the fix for this ?

JonasKunz commented 1 year ago

Spring 6 / spring boot 3 support has been released with version 1.38.0 .

jabrena commented 1 year ago

Does exist any docker compose example to test the new version?

geiliev commented 1 year ago

I can confirm that the issue is no longer present. Thank you!

jabrena commented 1 year ago

I found that project: https://github.com/elastic/opbeans-java/tree/main

zt9788 commented 1 year ago

Spring 6 / spring boot 3 support has been released with version 1.38.0 .

Hi, I use the Springboot3+SpringCloud2022, and try to use the 1.38.0 but when use the feign/restTemplate by default httpclient5 then apm can not record. When is it plan to support HttpClient5

SylvainJuge commented 1 year ago

Hi @zt9788 , adding support for httpclient 5.x will be covered in #2980.

Regarding Feign, I really don't know if we need to add another instrumentation to support it, from what I know of it it just delegates to the HTTP client, so in this case adding support for httpclient 5.x would be enough to make it work for you use-case.

JonasKunz commented 1 year ago

Hi, I use the Springboot3+SpringCloud2022, and try to use the 1.38.0 but when use the feign/restTemplate by default httpclient5 then apm can not record. When is it plan to support HttpClient5

Hi @zt9788, could you clarify what exactly is not working for you and maybe provide a sample application reproducing your problems?

Though we haven't thoroughly tested the standalone apache httpclient 5.x (which will be done via #2980 as @SylvainJuge said), we already do have tests using RestTemplate with the apache httpclient 5.x showing that the existing apache httpclient library seems to work correctly.

zt9788 commented 1 year ago

@JonasKunz The issue is that there is no record of calling by feign and Sorry about I really haven't tested the restTemplate yet, only added bean configuration, using the default registered Httpclient by feign. I upgrade my program Springboot2.7.x to Springboot3.1(SpringCloud2022) and change feign-httpclient to feign-hc5 It is can record in Springboot2.7.x(apm 1.37/1.38) befor

         <dependency>
            <groupId>org.springframework.cloud</groupId>
            <artifactId>spring-cloud-starter-openfeign</artifactId>
        </dependency>
       <!-- use httpclient5 -->
        <dependency>
            <groupId>io.github.openfeign</groupId>
            <artifactId>feign-hc5</artifactId>
        </dependency>

bean:

    @Bean
    @LoadBalanced
    public RestTemplate restTemplate(HttpComponentsClientHttpRequestFactory factory) {
        var restTemplate = new RestTemplate(factory);
        restTemplate.getMessageConverters()
                .add(0, new StringHttpMessageConverter(StandardCharsets.UTF_8));
        restTemplate.setInterceptors(Collections.singletonList(new TraceIdInterceptor()));
        return restTemplate;
    }
    //here the Httpclient is org.apache.hc.client5.http.classic.HttpClient; 
    //configruration the bean by feign(use the httpclient5)
    @Bean
    public HttpComponentsClientHttpRequestFactory clientHttpRequestFactory(HttpClient httpClient) {
        var clientHttpRequestFactory = new HttpComponentsClientHttpRequestFactory();
        clientHttpRequestFactory.setHttpClient(httpClient);
        clientHttpRequestFactory.setConnectionRequestTimeout(5000);
        clientHttpRequestFactory.setConnectTimeout(3000);
        return clientHttpRequestFactory;
    }
JonasKunz commented 1 year ago

I've been able to confirm that indeed feign requests with apache http client 5 and default configuration are not captured. Feign uses an org.apache.hc.client5.http.impl.classic.InternalHttpClient which seems to not be covered by our instrumentation.

However, this more part of #2954 than this issue. I'll add it as an acceptance criteria there.