-
Notifications
You must be signed in to change notification settings - Fork 274
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
Port otel/1090 #1852
Port otel/1090 #1852
Conversation
1b52061
to
d1df54f
Compare
@@ -1,77 +0,0 @@ | |||
package datadog.trace.instrumentation.java.concurrent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcculls looks like you an I might have some conflict here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcculls deleting this file seems to be the same thing you accomplished in your PR right? Removing the weakmap access?
@@ -12,9 +11,6 @@ | |||
@Slf4j | |||
public class ExecutorInstrumentationUtils { | |||
|
|||
private static final WeakMap<Executor, Boolean> EXECUTORS_DISABLED_FOR_WRAPPED_TASKS = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aww I hate seeing these disappear :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget... without this, how is ScheduledExecutorService
handled?
fb76613
to
281abc3
Compare
Ports open-telemetry/opentelemetry-java-instrumentation#1090 from @trask which since I also believe this is not needed.
TODO add some tests with java 8 and lambdas to ensure this isn't breaking things subtly and add some tests around executors that are on our classpath that have their own queue implementations