Refactor servlet instrumentation and disable async on response. #405
Conversation
Separate out common advice between instrumentation.
Add context to servlet2 test. Clean up some declared tracers that mess up the classpath.
tylerbenson
force-pushed
the
tyler/servlet-async
branch
from
e08dd6d
to
1e36343
Compare
| @@ -62,137 +44,21 @@ public ElementMatcher typeMatcher() { | |||
| return new String[] { | |||
| "datadog.trace.instrumentation.jetty8.HttpServletRequestExtractAdapter", | |||
| "datadog.trace.instrumentation.jetty8.HttpServletRequestExtractAdapter$MultivaluedMapFlatIterator", | |||
| HandlerInstrumentationAdvice.class.getName() + "$TagSettingAsyncListener" | |||
| "datadog.trace.instrumentation.jetty8.TagSettingAsyncListener" | |||
There was a problem hiding this comment.
You could use package name of the current class here instead of hardcoding it as a string - this would make it easier if things move.
There was a problem hiding this comment.
I considered it, but there are many other places that could also be done. Seems like a better case for doing consistently in a single PR.
| this.tags.remove(tag); | ||
| } | ||
| } else { | ||
| if (this.tags.isEmpty()) { |
There was a problem hiding this comment.
This whole idea with immutable empty hash for 'no tags' case seems like a premature optimization that just complicates code likely with not much of a real performance benefit if any. Maybe it is worth considering removing that?
There was a problem hiding this comment.
It's already in place (and has been since the prototype). I'd rather not mess with that here. Perhaps a separate PR?
Separate out common advice between instrumentation.