[improvement] Add methods to set span operation when wrapping with a new trace #59
Conversation
| @@ -237,7 +240,7 @@ public void testWrapCallableWithNewTrace_traceStateInsideCallableHasSpan() throw | |||
|
|
|||
| OpenSpan span = spans.get(0); | |||
|
|
|||
| assertThat(span.getOperation()).isEqualTo("root"); | |||
There was a problem hiding this comment.
we should retain test cases for the default as well
|
It would be helpful to configure our executors to wrap operations based on the executor name. This looks sane to me, thoughts @uschi2000? |
| * wrap a ScheduledExecutorService that has already been {@link #wrap(ExecutorService) wrapped}. If this is | ||
| * done, a new trace will be generated for each execution, effectively bypassing the intent of the previous | ||
| * wrapping. | ||
| */ | ||
| public static ExecutorService wrapWithNewTrace(ExecutorService executorService) { | ||
| public static ExecutorService wrapWithNewTrace(String operation, ExecutorService executorService) { |
There was a problem hiding this comment.
This will appear unredacted in Telemetry, so what do you think about another beloved @CompileTimeConstant to enforce that people don't accidentally exfil usernames or something?
There was a problem hiding this comment.
This would make it impossible to use thread pool name patterns as the operation when created a wrapped executor.
There was a problem hiding this comment.
I don't think this is the operation we'd use for thread pools, we'd need to use the DeferredTracer + overload Tracers.wrap(ExecutorService) which can be implemented in a separate PR
There was a problem hiding this comment.
We use this method when building scheduled executor services with new traces.
| ExecutorService wrappedService = | ||
| Tracers.wrapWithNewTrace(Executors.newSingleThreadExecutor()); | ||
| Tracers.wrapWithNewTrace(someOperartion, Executors.newSingleThreadExecutor()); |
|
Does this mean that all of these traces will no longer have any |
|
They won't have a span named Related to this PR, do we think we should add a span for every runnable executed on an executor? We copy the trace and allow spans to be created, but we don't provide any data to track when the task began/completed, or on which executor. |
|
This looks reasonable to me 👍 |
|
I've opened #63 |
Before this PR
When calling any of the
wrapWithNewTracemethods, the operation of the initial span would be set to "root". This makes it difficult to determine the origin of traces ifwrapWithNewTraceis used in more than one place.After this PR
Tracersnow provides variants of thewrapWithNewTracemethods that allow callers to explicitly set the initial span operation.Follow up to #49.