Add methods to create wrapped executor with new span #71
Conversation
|
What's the concern around making this the default behavior if it is generally useful? Are there any potential concerns? |
|
Making it the default behavior would require changing the API to also take an operation. I would prefer to deprecate the old methods. I just wanted to get some agreement that this is the desired outcome. |
|
@dansanduleac how do you feel about deprecating the old methods? |
|
An alternative is to keep the old methods but use the default "root" span operation. However this probably isn't very useful since seeing a "root" span operation can be confusing. |
|
I would actually challenge that. I would suggest making the operation not configurable, and simply something like "executor-wrapped-task". If someone wants to have an operation name, they can always create a new span themselves. In the end, what we are trying to get from this feature is the knowledge of when the task actually run, so the operation name does not really matter and it is better to everyone to have the better behaviour by default. Does that make sense? |
|
I disagree. I think having a default span name that will be used for all executors will quickly become confusing when you have more than one executor. Ideally users will want to identify these spans. Requiring them to wrap a task twice just to get useful names (with no way to remove the unhelpful default name) does not seems like the right solution here. |
|
Why do you need to identify the executor that wrapped a task? Can you give me an explicit workflow in which a constant operation is not enough? All that said, if you really feel strongly, we could allow the operation name to be configurable, but allowing for a default constant one in case you did not opt-in for overriding it seems much more beneficial than giving you no span at all until you sign in for it. |
|
I don't mind making the operation name configurable. I guess it can be useful if you want to search for spans of a specific wrap. |
| this(Optional.empty()); | ||
| } | ||
|
|
||
| public DeferredTracer(@CompileTimeConstant String operation) { |
There was a problem hiding this comment.
Please remove CompileTimeConstant
There was a problem hiding this comment.
(because it's reasonable for this to not be a compiletimeconstant, and it is overly onerous for downstream developers)
|
Thoughts on adding a default span operation for the existing executor methods? It seems like a reasonable approach to me I just want to make sure we're willing to make that behavior change. |
|
I would welcome that |
|
👍 |
## Before this PR In #71, all wrapped executor tasks had spans added by default. This was pretty noisy for tasks that were already doing their own wrapping. ## After this PR If wrapping is not explicitly requested, it is not done
Fixes #63
This PR preserves the existing executor wrapping methods and their behavior. So users must explicitly opt-in to the new span-creating behavior.This is no longer true. The existing executor methods will now create a new span for tasks they run.