opentelemetry-java-instrumentation icon indicating copy to clipboard operation
opentelemetry-java-instrumentation copied to clipboard

Library instrumentation for java.util.concurrent (metrics)

Open ZawadaZSE opened this issue 3 years ago • 11 comments

Is your feature request related to a problem? Please describe. We're migrating from opentracing and noticed that manual instrumentation for Executors is missing.

Describe the solution you'd like I'd like to know if you plan to implement similar solution to: https://github.com/opentracing-contrib/java-concurrent that would allow manual instrumentation of Executors.

Describe alternatives you've considered I know that it's supported with javaagent instrumentation but we'd rather not use it.

Additional context Add any other context or screenshots about the feature request here.

ZawadaZSE avatar Jun 22 '22 13:06 ZawadaZSE

Hey @ZawadaZSE , Do you need to create spans for every submitted task? Or is it just about context propagation across threads? If it's the latter, there already are facilities to help you with that: you can use the Context.taskWrapping() method to decorate an ExecutorService/Executor with current context propagation.

mateuszrzeszutek avatar Jun 22 '22 13:06 mateuszrzeszutek

Hey @mateuszrzeszutek We are using Context.taskWrapping() method to decorate an ExecutorService. But we are getting errors since we can't use io.opentelemetry.context.CurrentContextExecutorService with Micrometer ExecutorServiceMetrics: i.m.c.i.b.jvm.ExecutorServiceMetrics - Failed to bind as io.opentelemetry.context.CurrentContextExecutorService is unsupported.

reactivejson avatar Jun 30 '22 11:06 reactivejson

Hey @reactivejson , Can you try using micrometer first, and then wrapping with Context.taskWrapping()? E.g.

Context.taskWrapping(
    ExecutorServiceMetrics.monitor(meterRegistry, executorService, "myExecutor"))

mateuszrzeszutek avatar Jun 30 '22 11:06 mateuszrzeszutek

Thanks @mateuszrzeszutek for your reply, I will check your suggestion. One more issue we have is that we cant cast io.opentelemetry.context.CurrentContextExecutorService to ThreadPoolExecutor: java.lang.ClassCastException: class io.opentelemetry.context.CurrentContextExecutorService cannot be cast to class java.util.concurrent.ThreadPoolExecutor (io.opentelemetry.context.CurrentContextExecutorService is in unnamed module of loader 'app'; java.util.concurrent.ThreadPoolExecutor is in module java.base of loader 'bootstrap')

Do you know how to approach it?

reactivejson avatar Jun 30 '22 12:06 reactivejson

Hey @reactivejson , Can you try using micrometer first, and then wrapping with Context.taskWrapping()? E.g.

Context.taskWrapping(
    ExecutorServiceMetrics.monitor(meterRegistry, executorService, "myExecutor"))

We are beanifying it in Spring boot so we don't have much control over it, and it's used by Prometheus, so we can't use ExecutorServiceMetrics.monitor(meterRegistry, executorService, "myExecutor"))

Our code: @Bean ExecutorServiceMetrics example(ExecutorService operationNotificationsExecutorService) { return new ExecutorServiceMetrics(operationNotificationsExecutorService, "operation-notifications", emptyList()); }

reactivejson avatar Jun 30 '22 12:06 reactivejson

One more issue we have is that we cant cast io.opentelemetry.context.CurrentContextExecutorService to ThreadPoolExecutor: java.lang.ClassCastException: class io.opentelemetry.context.CurrentContextExecutorService cannot be cast to class java.util.concurrent.ThreadPoolExecutor (io.opentelemetry.context.CurrentContextExecutorService is in unnamed module of loader 'app'; java.util.concurrent.ThreadPoolExecutor is in module java.base of loader 'bootstrap')

That is expected, because CurrentContextExecutorService is not a ThreadPoolExecutor. In general, I'd recommend using the ExecutorService interface instead of ThreadPoolExecutor where it's possible.

mateuszrzeszutek avatar Jun 30 '22 12:06 mateuszrzeszutek

Hey,

@mateuszrzeszutek problem is that this is internal implementation of micrometer.

We're using ExecutorService interface everywhere in out code. We also use bean post processing to wrap all executor services in CurrentContextExecutorService so that we would not loose context anywhere.

I've checked how it was done in opentracing implementation and they had extended ThreadPoolExecutorService (io.opentracing.contrib.spring.cloud.async.instrument.TracedThreadPoolTaskExecutor).

Can something similar be done for opentelemetry? I feel that whole community could benefit of such change.

ZawadaZSE avatar Jul 04 '22 08:07 ZawadaZSE

@ZawadaZSE @reactivejson Are you using spring-actuator to register the executor service metrics? Or are you doing it manually? For OpenTelemetry, are you using the OpenTelemetry Spring Starter? Spring Cloud Sleuth? Or again, is it a manual setup?

mateuszrzeszutek avatar Jul 06 '22 09:07 mateuszrzeszutek

@mateuszrzeszutek Yes, we're using spring-actuator and we're manually configuring ExecutorServiceMetrics beans like this:

    @Bean
    ExecutorServiceMetrics someExecutorServiceMetrics(ExecutorService someExecutorService) {
        return new ExecutorServiceMetrics(someExecutorService, "some-executor", emptyList());
    }

As for open telemetry, we're using spring starter but that one does not provide auto configuration for executors. We wrote our own BeanPostProcessor which wraps all ExecutorService type beans in Context.taskWrapping(executorService)

ZawadaZSE avatar Jul 06 '22 11:07 ZawadaZSE

You can make use of the ExecutorServiceMetrics.monitor() function, which also returns an ExecutorService, and redefine the someExecutorService bean the following way:

@Bean 
ExecutorService someExecutorService(MeterRegistry meterRegistry) {
  ExecutorService executor = new ThreadPoolExecutor(...);
  return ExecutorServiceMetrics.monitor(meterRegistry, executor, "some-executor");
}

That way your BeanPostProcessor will wrap the already metricized executor.

I agree with you that the OpenTelemetry Spring starter should offer context propagation by default, and that it should play well with the Micrometer metrics installed by the Spring Actuator (that only measure Spring task executors), but I don't think that we can avoid wrapping Executors/ExecutorServices -- in the first place, the OpenTracing lib you mentioned does exactly that, it wraps standard Java executors, and has separate extending wrapper classes for spring types.

mateuszrzeszutek avatar Jul 07 '22 13:07 mateuszrzeszutek