Update webflux to Instrumenter API (and improvements/simplifications)

[trask]

In addition to updating to Instrumenter API:

• Handler span now maps to org.springframework.web.reactive.HandlerAdapter.handle() instead of org.springframework.web.reactive.DispatcherHandler.handle(), which more closely maps to the user handler. This aligns better with other handler spans, and the previous (strange to me) handler instrumentation placement was what motivated What is a "handler span"? #3144. Also, I think this change addresses the flaky problems that required Fix flaky spring webflux tests #3150 previously (without needing the fix from Fix flaky spring webflux tests #3150). @laurit I was not able to get any flakes following your notes to repro the flaky problem (increased request count to 300 for "Multiple GETs to delaying route" test and used spring boot 2.5.0), but would be great if you have time to give it a look/try also.

• Removed spring-webflux.request.predicate experimental attribute because the handler span is not available yet when this is calculated. If anyone wants to keep this, I can either put it on the SERVER span, or propagate it down to where the handler span is started.

[mateuszrzeszutek]

Could we move that code inside instrumenter.start() call?

[anuraaga]

Do you mean Enter? This code seems to be in HandleAdvice.OnMethodExit right now

[trask]

oh no, two different HandleAdvice classes, I fixed the comment 👍

[laurit]

@trask I couldn't reproduce it either, perhaps something else has changed that makes triggering this harder.

[laurit]

Tried it, strict context check still fails. I believe we need to wait for netty request processing to complete before doing context leaks check. I'll create a pr for this.

[laurit]

Although this part wasn't changed in this pr getting a span name from an object is also used in other instrumentations so it would be nice if someone figured out how to do this properly. The problem here is that handler could be anything. For example SpringWebFluxTestApplication when you add

  @Component("greetingHandlerFunction")
  @Scope(value = "prototype", proxyMode = ScopedProxyMode.TARGET_CLASS)
  static class GreetingHandlerFunction implements HandlerFunction<ServerResponse> {
    private final GreetingHandler greetingHandler

    GreetingHandlerFunction(GreetingHandler greetingHandler) {
      this.greetingHandler = greetingHandler
    }

    @Override
    Mono<ServerResponse> handle(ServerRequest request) {
      return greetingHandler.defaultGreet()
    }
  }

and change the start of greetRouterFunction to

  @Bean
  RouterFunction<ServerResponse> greetRouterFunction(GreetingHandler greetingHandler, HandlerFunction<ServerResponse> greetingHandlerFunction) {
    return route(GET("/greet"), greetingHandlerFunction

Then span name is
SpringWebFluxTestApplication$GreetingHandlerFunction$$EnhancerBySpringCGLIB$$d662c0c3.handle
When you change scope annotation to
@Scope(value = "prototype", proxyMode = ScopedProxyMode.INTERFACES)
then span name is
$Proxy119.handle
If we'd assume that handler is a spring bean we could try to unwrap it with something like spring-projects/spring-framework@efe3a35

[trask]

👍 I created issue for this #3813

[laurit]

Handler could be a proxy so name could be SpringWebFluxTestApplication$GreetingHandlerFunction$$EnhancerBySpringCGLIB$$d662c0c3 or $Proxy119

[trask]

@HaloFour have you seen the cancellation issue that was fixed in #3150? I'm wondering if you have any thoughts whether that fix should be applied to the @WithSpan Mono handling? I'm trying to get a sense if it's a general issue that should be handled everywhere Monos are returned (regardless of whether we can reproduce the issue anymore that led to #3150).

[HaloFour]

@trask

The Reactor instrumentation for Mono<T> and Flux<T> do handle cancellation, although using a different approach: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/reactor-3.1/library/src/main/java/io/opentelemetry/instrumentation/reactor/ReactorAsyncOperationEndStrategy.java#L64

I also have another PR up which refactors the Reactor instrumentation to allow for multiple subscriptions and in that implementation I follow a similar approach to the PR you referenced: https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/3751/files#diff-0865720f5bd8562039a4e795c46fd7f745a94a5ba6852ff463ffa0d0424fa236R65

[trask]

putting this PR on hold until #3816