You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "ammachado (via GitHub)" <gi...@apache.org> on 2024/09/11 05:26:12 UTC
[PR] Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
ammachado opened a new pull request, #15496:
URL: https://github.com/apache/camel/pull/15496
# Description
Add a ThreadPoolFactory to propagate OpenTelemetry contexts
# Target
- [x] I checked that the commit is targeting the correct branch (note that Camel 3 uses `camel-3.x`, whereas Camel 4 uses the `main` branch)
# Tracking
- [x] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it).
# Apache Camel coding standards and style
- [x] I checked that each commit in the pull request has a meaningful subject line and body.
- [x] I have run `mvn clean install -DskipTests` locally from root folder and I have committed all auto-generated changes.
<!--
You can run the aforementioned command in your module so that the build auto-formats your code. This will also be verified as part of the checks and your PR may be rejected if if there are uncommited changes after running `mvn clean install -DskipTests`.
You can learn more about the contribution guidelines at https://github.com/apache/camel/blob/main/CONTRIBUTING.md
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2355249836
yes thread locals and MDC is poor in a modern multi threaded world. However its fine if there is a few situations like multicast + parallel its not work (just document this). We just need it to work good for 99% use-cases.
And we can then see if we can add some special hook in mutlicast parallel mode to make it work better for this use-case. otel is more important than legacy MDC logging
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "ammachado (via GitHub)" <gi...@apache.org>.
ammachado commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2343739678
> Do you have any links/docs about otel and wrapping these threads is a best practice ? The threads are reused by other Camel Exchanges and as such I wonder how otel manages that.
https://opentelemetry.io/docs/languages/java/instrumentation/#context-propagation-between-threads
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2343348549
Do you have any links/docs about otel and wrapping these threads is a best practice ?
The threads are reused by other Camel Exchanges and as such I wonder how otel manages that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "ammachado (via GitHub)" <gi...@apache.org>.
ammachado commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2345243826
I replaced the implementation with wrapping `ExecutorService`s. Should be fine now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "ammachado (via GitHub)" <gi...@apache.org>.
ammachado commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2346720829
Yes, @davsclaus, you are correct. I understand the issue, but this PR is handling only thread pools that are created by Camel itself. I don't have enough time/resources to test all components, but I'll be happy to help as much as possible.
Regarding the exchanges and it's span ids, this is already / should be handled by `org.apache.camel.opentelemetry.OpenTelemetryTracer`, and `org.apache.camel.tracing.SpanDecorator` component-specific implementations. New spans are created by default for all components which have a `SpanDecorator` implementation, except for `LogComponent`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2354693362
Be mindful that before/after process is executed for every step (EIP i.e. Processor) in the route.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "johnpoth (via GitHub)" <gi...@apache.org>.
johnpoth commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2355151296
Having to use this ThreadLocal stuff seems like a tough requirement IMO so I'm investigating if we can remove it [1] [2]
[1] https://github.com/johnpoth/camel/commits/opentelemetry-rewrite/
[2] https://github.com/open-telemetry/opentelemetry-java/discussions/6584
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2354690625
> @davsclaus are we guaranteed to be called on the same Thread though in the UnitOfWork hooks ? Unfortunately OpenTelemetry requires it [1]
>
> [1] https://github.com/open-telemetry/opentelemetry-java/blob/main/context/src/main/java/io/opentelemetry/context/ThreadLocalContextStorage.java#L48
Yes but only when you use before/afterProcess callbacks and turn this on (like MDC does)
https://github.com/apache/camel/blob/b4b66d5b9dc9ce44a5ed497076fd837c73b70b0c/core/camel-base-engine/src/main/java/org/apache/camel/impl/engine/SharedCamelInternalProcessor.java#L161
I think this was implemented for the same reason that MDC also needs callback for before/after on the same thread.
PS: It may be that Camel in transacted mode does not execute before/after processor (we can add this if needed) but transacted is forced executing synchronously anyway by Camel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2373028006
We can expose beforeExecute/afterExecute from `ThreadPoolExecutor` that should make this easier. And its also more correct as these callbacks are invoked just before the runnable is being executed by the current thread.
Then we can find a nicer way to do this without having to wrap every thread pool executor implementation in Camel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2373355499
This PR
https://github.com/apache/camel/pull/15679
Makes it possible to build a hook into `newThread` so all threads can be wrapped. However `newThread` is only used in a few places in Camel for auxilliary threads such as a JMX remote listener port, and for camel-main to trigger shutdown if duration was hit. So its not really in use for regular standard Camel apps. But this allows us to cover 100%, and also a hook for future use.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "johnpoth (via GitHub)" <gi...@apache.org>.
johnpoth commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2348408770
@davsclaus IMO we would need a new SPI for `ThreadPoolFactory` in Camel to allow us to grab the current Exchange the Thread is running on. This would allow camel-opentelemetry to do something like this before every thread:
```
@Override
public Future<?> submit(Runnable task, Exchange exchange) {
Context context = exchange.getProperty(ExchangePropertyKey.ACTIVE_SPAN, io.opentelemetry.context.Context.Context.class)
return delegate().submit(context.wrap(task));
}
```
That way, other libraries in the Thread know what context to grab. Most libraries do a Context.current() which relies on ThreadLocal storage...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "ammachado (via GitHub)" <gi...@apache.org>.
ammachado commented on code in PR #15496:
URL: https://github.com/apache/camel/pull/15496#discussion_r1775788049
##########
components/camel-opentelemetry/src/main/java/org/apache/camel/opentelemetry/OpenTelemetryInstrumentedThreadPoolFactory.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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 org.apache.camel.opentelemetry;
+
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.RejectedExecutionHandler;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.TimeUnit;
+
+import io.opentelemetry.context.Context;
+import org.apache.camel.opentelemetry.internal.CurrentContextScheduledExecutorService;
+import org.apache.camel.spi.ThreadPoolFactory;
+import org.apache.camel.spi.ThreadPoolProfile;
+import org.apache.camel.spi.annotations.JdkService;
+import org.apache.camel.support.DefaultThreadPoolFactory;
+
+@JdkService(ThreadPoolFactory.FACTORY)
+public class OpenTelemetryInstrumentedThreadPoolFactory extends DefaultThreadPoolFactory implements ThreadPoolFactory {
+
+ @Override
+ public ExecutorService newCachedThreadPool(ThreadFactory threadFactory) {
+ return Context.taskWrapping(super.newCachedThreadPool(threadFactory));
+ }
+
+ @Override
+ public ExecutorService newThreadPool(
+ int corePoolSize,
+ int maxPoolSize,
+ long keepAliveTime,
+ TimeUnit timeUnit,
+ int maxQueueSize,
+ boolean allowCoreThreadTimeOut,
+ RejectedExecutionHandler rejectedExecutionHandler,
+ ThreadFactory threadFactory)
+ throws IllegalArgumentException {
+
+ ExecutorService executorService = super.newThreadPool(
+ corePoolSize,
+ maxPoolSize,
+ keepAliveTime,
+ timeUnit,
+ maxQueueSize,
+ allowCoreThreadTimeOut,
+ rejectedExecutionHandler,
+ threadFactory);
+
+ return Context.taskWrapping(executorService);
+ }
+
+ @Override
+ public ScheduledExecutorService newScheduledThreadPool(ThreadPoolProfile profile, ThreadFactory threadFactory) {
+ return new CurrentContextScheduledExecutorService(super.newScheduledThreadPool(profile, threadFactory));
Review Comment:
I'll try. There's a PR open on OpenTelemetry to move this implementation there. https://github.com/open-telemetry/opentelemetry-java/pull/6712
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "ammachado (via GitHub)" <gi...@apache.org>.
ammachado commented on code in PR #15496:
URL: https://github.com/apache/camel/pull/15496#discussion_r1775788049
##########
components/camel-opentelemetry/src/main/java/org/apache/camel/opentelemetry/OpenTelemetryInstrumentedThreadPoolFactory.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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 org.apache.camel.opentelemetry;
+
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.RejectedExecutionHandler;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.TimeUnit;
+
+import io.opentelemetry.context.Context;
+import org.apache.camel.opentelemetry.internal.CurrentContextScheduledExecutorService;
+import org.apache.camel.spi.ThreadPoolFactory;
+import org.apache.camel.spi.ThreadPoolProfile;
+import org.apache.camel.spi.annotations.JdkService;
+import org.apache.camel.support.DefaultThreadPoolFactory;
+
+@JdkService(ThreadPoolFactory.FACTORY)
+public class OpenTelemetryInstrumentedThreadPoolFactory extends DefaultThreadPoolFactory implements ThreadPoolFactory {
+
+ @Override
+ public ExecutorService newCachedThreadPool(ThreadFactory threadFactory) {
+ return Context.taskWrapping(super.newCachedThreadPool(threadFactory));
+ }
+
+ @Override
+ public ExecutorService newThreadPool(
+ int corePoolSize,
+ int maxPoolSize,
+ long keepAliveTime,
+ TimeUnit timeUnit,
+ int maxQueueSize,
+ boolean allowCoreThreadTimeOut,
+ RejectedExecutionHandler rejectedExecutionHandler,
+ ThreadFactory threadFactory)
+ throws IllegalArgumentException {
+
+ ExecutorService executorService = super.newThreadPool(
+ corePoolSize,
+ maxPoolSize,
+ keepAliveTime,
+ timeUnit,
+ maxQueueSize,
+ allowCoreThreadTimeOut,
+ rejectedExecutionHandler,
+ threadFactory);
+
+ return Context.taskWrapping(executorService);
+ }
+
+ @Override
+ public ScheduledExecutorService newScheduledThreadPool(ThreadPoolProfile profile, ThreadFactory threadFactory) {
+ return new CurrentContextScheduledExecutorService(super.newScheduledThreadPool(profile, threadFactory));
Review Comment:
There is no wrapper for `ScheduledExecutorService` yet.
There's a PR open on OpenTelemetry to move this implementation there. https://github.com/open-telemetry/opentelemetry-java/pull/6712
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2342652425
:star2: Thank you for your contribution to the Apache Camel project! :star2:
:robot: CI automation will test this PR automatically.
:camel: Apache Camel Committers, please review the following items:
* First-time contributors **require MANUAL approval** for the GitHub Actions to run
* You can use the command `/component-test (camel-)component-name1 (camel-)component-name2..` to request a test from the test bot.
* You can label PRs using `build-all`, `build-dependents`, `skip-tests` and `test-dependents` to fine-tune the checks executed by this PR.
* Build and test logs are available in the Summary page. **Only** [Apache Camel committers](https://camel.apache.org/community/team/#committers) have access to the summary.
* :warning: Be careful when sharing logs. Review their contents before sharing them publicly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "johnpoth (via GitHub)" <gi...@apache.org>.
johnpoth commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2355138476
Hi @davsclaus thanks for your insights ! After some testing I don't think this is enough... Ideally OpenTelemetry wants a hook when a _thread_ starts some work and another hook when the work is done ( everything in the same thread):
```
Context context = exchange.getProperty(ExchangePropertyKey.ACTIVE_SPAN, io.opentelemetry.context.Context.Context.class);
Scope scope = context.makeCurrent();
<!---- Thread starts work ---->
<!---- Thread ends work ---->
scope.close();
```
Naively using the before/after process hooks in the UnitOfWorkAPI I get some errors like :
```
Thread [Camel (camel-6) thread #36 - seda://c] opened scope, but thread [Camel (camel-6) thread #44 - Delay] closed it
closed here: at org.apache.camel.opentelemetry.OpenTelemetryUnitOfWork.afterProcess(OpenTelemetryUnitOfWork.java:98) ~[classes/:?]
opened here: at org.apache.camel.opentelemetry.OpenTelemetryUnitOfWork.beforeProcess(OpenTelemetryUnitOfWork.java:86) ~[classes/:?]
```
For MDC to achieve this when in Multicast with parallel processing, it seems some more code was added in camel-core [1]
[1] https://issues.apache.org/jira/browse/CAMEL-16378
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2348494053
Hmm I think this is somewhat wrong with these thread pool.
If the purpose is to ensure the otel context is correct for the given exchange and to store/get that via thread locals, then look at MDC and the `MDCUnitOfWork` it is doing this kind of stuff.
It has callbacks for before/after and coupled with the Exchange processing by Camel which allows better to setup/cleanup otel context accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "johnpoth (via GitHub)" <gi...@apache.org>.
johnpoth commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2353284724
@davsclaus are we guaranteed to be called on the same Thread though in the UnitOfWork hooks ? Unfortunately OpenTelemetry requires it [1]
[1] https://github.com/open-telemetry/opentelemetry-java/blob/main/context/src/main/java/io/opentelemetry/context/ThreadLocalContextStorage.java#L48
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on code in PR #15496:
URL: https://github.com/apache/camel/pull/15496#discussion_r1774733303
##########
components/camel-opentelemetry/src/main/java/org/apache/camel/opentelemetry/OpenTelemetryInstrumentedThreadPoolFactory.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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 org.apache.camel.opentelemetry;
+
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.RejectedExecutionHandler;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.TimeUnit;
+
+import io.opentelemetry.context.Context;
+import org.apache.camel.opentelemetry.internal.CurrentContextScheduledExecutorService;
+import org.apache.camel.spi.ThreadPoolFactory;
+import org.apache.camel.spi.ThreadPoolProfile;
+import org.apache.camel.spi.annotations.JdkService;
+import org.apache.camel.support.DefaultThreadPoolFactory;
+
+@JdkService(ThreadPoolFactory.FACTORY)
+public class OpenTelemetryInstrumentedThreadPoolFactory extends DefaultThreadPoolFactory implements ThreadPoolFactory {
+
+ @Override
+ public ExecutorService newCachedThreadPool(ThreadFactory threadFactory) {
+ return Context.taskWrapping(super.newCachedThreadPool(threadFactory));
+ }
+
+ @Override
+ public ExecutorService newThreadPool(
+ int corePoolSize,
+ int maxPoolSize,
+ long keepAliveTime,
+ TimeUnit timeUnit,
+ int maxQueueSize,
+ boolean allowCoreThreadTimeOut,
+ RejectedExecutionHandler rejectedExecutionHandler,
+ ThreadFactory threadFactory)
+ throws IllegalArgumentException {
+
+ ExecutorService executorService = super.newThreadPool(
+ corePoolSize,
+ maxPoolSize,
+ keepAliveTime,
+ timeUnit,
+ maxQueueSize,
+ allowCoreThreadTimeOut,
+ rejectedExecutionHandler,
+ threadFactory);
+
+ return Context.taskWrapping(executorService);
+ }
+
+ @Override
+ public ScheduledExecutorService newScheduledThreadPool(ThreadPoolProfile profile, ThreadFactory threadFactory) {
+ return new CurrentContextScheduledExecutorService(super.newScheduledThreadPool(profile, threadFactory));
Review Comment:
Can you use
`return Context.wrap(super.newScheduledThreadPool(profile, threadFactory));`
As opentelemtry has a wrap for ScheduledExecutorService
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2373310128
> We can expose beforeExecute/afterExecute from `ThreadPoolExecutor` that should make this easier. And its also more correct as these callbacks are invoked just before the runnable is being executed by the current thread.
>
> Then we can find a nicer way to do this without having to wrap every thread pool executor implementation in Camel.
One caveat with that is the before and after is executed when the task is being processed, and the original thread that added the task may be doing something else, and the need for this context propagation may be too late. However this would only be in cases where the thread pool is busy and tasks and filled up in the backlog queue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2346510909
> I replaced the implementation with wrapping `ExecutorService`s. Should be fine now.
I fail to understand that, because that only works for a task/runnable that are isolated.
You do this generally for all of Camel's thread pool, and threads such as a JMS listener will then use the same task/runnable but to process all the incoming JMS messages, and each of those JMS messages is a different Exchange and _span id_ in that otel world.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2343751230
> > Do you have any links/docs about otel and wrapping these threads is a best practice ? The threads are reused by other Camel Exchanges and as such I wonder how otel manages that.
>
> https://opentelemetry.io/docs/languages/java/instrumentation/#context-propagation-between-threads
Yes but would that not be wrong as the thread is reused by Camel later to process other exchanges and then the thread has stale data from the previous exchanges.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "johnpoth (via GitHub)" <gi...@apache.org>.
johnpoth commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2348685863
Ah yeah the camel-opentelemetry is doing that through CamelEvents but perhaps the UnitOfWork API is more suited, will check it out
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-21202 - Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on code in PR #15496:
URL: https://github.com/apache/camel/pull/15496#discussion_r1778015194
##########
components/camel-opentelemetry/src/main/java/org/apache/camel/opentelemetry/OpenTelemetryInstrumentedThreadPoolFactory.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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 org.apache.camel.opentelemetry;
+
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.RejectedExecutionHandler;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.TimeUnit;
+
+import io.opentelemetry.context.Context;
+import org.apache.camel.opentelemetry.internal.CurrentContextScheduledExecutorService;
+import org.apache.camel.spi.ThreadPoolFactory;
+import org.apache.camel.spi.ThreadPoolProfile;
+import org.apache.camel.spi.annotations.JdkService;
+import org.apache.camel.support.DefaultThreadPoolFactory;
+
+@JdkService(ThreadPoolFactory.FACTORY)
+public class OpenTelemetryInstrumentedThreadPoolFactory extends DefaultThreadPoolFactory implements ThreadPoolFactory {
+
+ @Override
+ public ExecutorService newCachedThreadPool(ThreadFactory threadFactory) {
+ return Context.taskWrapping(super.newCachedThreadPool(threadFactory));
+ }
+
+ @Override
+ public ExecutorService newThreadPool(
+ int corePoolSize,
+ int maxPoolSize,
+ long keepAliveTime,
+ TimeUnit timeUnit,
+ int maxQueueSize,
+ boolean allowCoreThreadTimeOut,
+ RejectedExecutionHandler rejectedExecutionHandler,
+ ThreadFactory threadFactory)
+ throws IllegalArgumentException {
+
+ ExecutorService executorService = super.newThreadPool(
+ corePoolSize,
+ maxPoolSize,
+ keepAliveTime,
+ timeUnit,
+ maxQueueSize,
+ allowCoreThreadTimeOut,
+ rejectedExecutionHandler,
+ threadFactory);
+
+ return Context.taskWrapping(executorService);
+ }
+
+ @Override
+ public ScheduledExecutorService newScheduledThreadPool(ThreadPoolProfile profile, ThreadFactory threadFactory) {
+ return new CurrentContextScheduledExecutorService(super.newScheduledThreadPool(profile, threadFactory));
Review Comment:
can we add a TODO in the camel code to refer to this so we wont forget to change it when otel comes with this out of the box.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a ThreadPoolFactory to propagate OpenTelemetry contexts [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #15496:
URL: https://github.com/apache/camel/pull/15496#issuecomment-2342803172
Do you mind create a JIRA for these kind of PRs and link to the JIRA in the commit message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org