Allow for simpler creation of start-only and end-only SpanProcessors. #5923
Conversation
Codecov ReportAttention:
... and 10 files with indirect coverage changes 📢 Thoughts on this report? Let us know!. |
|
I like this idea. Couple of thoughts:
|
Sure, what's the mechanism for doing that?
I sketched that as well. I think I concluded that I disliked allowing implementations of Maybe I'm being a little too rigid there, but it just seems to give a lot of room for users to build weird, ill-fitting implementations. However, I might could be talked into it.... |
Move it to an internal package or, preferably,
That's a good point. I guess the thing that gives me pause about the approach in this PR is the use of Consumer / BiConsumer instead of having dedicated functional interfaces. What if we added dedicated |
Ok, I don't think it hurts to add these, so I'm doing that...but I also think that if the |
|
|
||
| @FunctionalInterface | ||
| public interface OnEnd { | ||
| void apply(ReadableSpan span); |
There was a problem hiding this comment.
I used apply for both OnStart and OnEnd. I read it as a functional interface, so an implementation or lambda instance of OnEnd is the function and the function gets applied. At least that's how my brain thinks of it.
I think the name isn't hugely important here, but OnEnd.onEnd() seems a little silly. I think one could also make the case that it's a consumer, so it should be called accept(), but then OnStart has two args.
I guess I think apply is the most straightforward.
There was a problem hiding this comment.
is this too silly?
@FunctionalInterface
public interface OnEndSpanProcessor extends SpanProcessor {
// this is the kind of silly part...
static SpanProcessor create(OnEndSpanProcessor onEnd) {
return onEnd;
}
@Override
default boolean isEndRequired() {
return true;
}
@Override
default void onStart(Context parentContext, ReadWriteSpan span) {
// nop
}
@Override
default boolean isStartRequired() {
return false;
}
}
There was a problem hiding this comment.
is this too silly?
It's not, but it does allow people to still do silly things with it. :)
|
Will merge this tomorrow if there are no further comments. |
The current design of SpanProcessor is such that it allows implementations to designate whether they operate at start time, end time, or both. A number of implementations only do one small thing at start time or one small thing at end time, and not both.
This results in a fair amount of boilerplate that gets in the way of readability.
Because SpanProcessor is stable, we can't alter the existing api surface, so this just adds a couple of helper classes. If this idea is interesting, you could also peek at the first commit, which used static factory methods on the interface to return anonymous classes.