Add limit to trace scope depth #1177
Conversation
When limit is exceeded, a NoopScope is returned. Allow custom ScopeManager to be provided, with the plan to remove `ScopeContext` customization in the future.
| @@ -510,7 +516,9 @@ public boolean addTraceInterceptor(final TraceInterceptor interceptor) { | |||
|
|
|||
| @Override | |||
| public void addScopeListener(final ScopeListener listener) { | |||
| scopeManager.addScopeListener(listener); | |||
| if (scopeManager instanceof ContextualScopeManager) { | |||
There was a problem hiding this comment.
Why do we need to cast?
I assuming because the Nop variant doesn't have addScopeListener.
I think it would be cleaner to addScopeListener to the Nop variant.
There was a problem hiding this comment.
no, this is because ScopeListener is purely something we added, not part of OpenTracing. If we accept any OpenTracing ScopeManager, we can't support this.
There was a problem hiding this comment.
That makes sense. Although, we could address that by having our own Nop with addScopeListener.
But for now, I think it is fine. We can address it after we split from OpenTracing.
| if (active instanceof DDScope) { | ||
| final int currentDepth = ((DDScope) active).depth(); | ||
| if (depthLimit <= currentDepth) { | ||
| log.debug("Scope depth limit exceeded ({}). Returning NoopScope.", currentDepth); |
There was a problem hiding this comment.
Should this be debug or error?
There was a problem hiding this comment.
It would also be nice to have a health metric, but we haven't made getting to the statsd object easy yet.
There was a problem hiding this comment.
Ideally it would be a rate limited warning, but this can be very high throughput, so I kept it at debug.
There was a problem hiding this comment.
Yeah I agree a health metric would make sense. Should that work be in this PR or a separate one?
There was a problem hiding this comment.
It can wait for now. I need to finish my PR where I make health metrics more accessible, but other things took priority.
When limit is exceeded, a NoopScope is returned.
Allow custom ScopeManager to be provided, with the plan to remove
ScopeContextcustomization in the future.