Consolidate span suppression logic into the BaseTracer #2106
Conversation
77c9544
to
645af1e
Compare
| @@ -83,6 +80,25 @@ public Span getCurrentSpan() { | |||
| return Span.current(); | |||
| } | |||
|
|
|||
| protected final boolean shouldNotSuppressSpan(Kind proposedKind, Context context) { | |||
There was a problem hiding this comment.
Can we name this method shouldStartSpan? Current name is too close to double negation IMO
There was a problem hiding this comment.
I don't like the double negative either. But,I also want something that references suppression. Unless we want everything to always call this method before starting a span. We could reverse the logic if we're ok with all the current callers negating it.
There was a problem hiding this comment.
Well, in theory as of now all instrumentations should/could make a check before starting a span. Either to prevent nested SERVER/CLIENT spans or to check CallDepth for INTERNAL spans. This may mean that "we want everything to always call this method before starting a span". But nested client spans may become allowed...
There was a problem hiding this comment.
right. the main reason for this change was to make it so that we could have one place to handle the supression logic, especially if the logic was going to change. Happy to rename to shouldStartSpan if folks think that's not too generic.
There was a problem hiding this comment.
I also just realized that this method is now unit-testable. I'll do the rename and add some unit tests.
Provide the appropriate methods to get access.
31fde4e
to
26c681b
Compare
Depends on #2105 (will rebase after that is merged)