Server span naming for servlet filters #2887
Conversation
laurit
requested review from
anuraaga,
iNikem,
jkwatson,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
| public final class JakartaServletFilterConfigHolder { | ||
| private static final Cache<Filter, FilterConfig> filterConfigs = | ||
| Cache.newBuilder().setWeakKeys().build(); |
There was a problem hiding this comment.
can we use InstrumentationContext instead? or if not can you add comment here to explain why?
There was a problem hiding this comment.
rewrote it to use InstrumentationContext
...ntation/servlet/servlet-5.0/javaagent/src/test/groovy/TomcatServlet5FilterMappingTest.groovy
Outdated
Show resolved
Hide resolved
| public final class Servlet3FilterConfigHolder { | ||
| private static final Cache<Filter, FilterConfig> filterConfigs = | ||
| Cache.newBuilder().setWeakKeys().build(); |
| protected MappingProvider<ServletContext> getMappingProvider(Object servletOrFilter) { | ||
| if (servletOrFilter instanceof Servlet) { | ||
| return getServletMappingProvider((Servlet) servletOrFilter); | ||
| } else if (servletOrFilter instanceof Filter) { | ||
| return getServletFilterMappingProvider((Filter) servletOrFilter); | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
currently this instantiates a new MappingProvider on each call, can we cache the MappingProvider for each servlet/filter?
or maybe cache MappingResolver in the servlet/filter, and then only need to instantiate MappingProvider the first time when MappingResolver is not available and need to instantiate it? and maybe then MappingProvider can be MappingResolverFactory or MappingResolverProvider? I think that would help reduce the number of concepts (even if not the number of classes)
I also may not be making sense, let me know
There was a problem hiding this comment.
now MappingResolver is bound to InstrumentationContext via MappingResolverFactory
...ntation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java
Outdated
Show resolved
Hide resolved
…TomcatServlet5FilterMappingTest.groovy Co-authored-by: Trask Stalnaker <[email protected]>
...entation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/MappingResolver.java
Outdated
Show resolved
Hide resolved
...entelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3FilterMappingResolverFactory.java
Outdated
Show resolved
Hide resolved
...ntation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3FilterMappingTest.groovy
Outdated
Show resolved
Hide resolved
| import spock.lang.IgnoreIf | ||
|
|
||
| @IgnoreIf({ !jvm.java11Compatible }) | ||
| class JettyServlet5MappingTest extends AbstractServlet5MappingTest<Object, Object> { |
There was a problem hiding this comment.
Why not
| class JettyServlet5MappingTest extends AbstractServlet5MappingTest<Object, Object> { | |
| class JettyServlet5MappingTest extends AbstractServlet5MappingTest<Server, ServletContextHandler > { |
?
There was a problem hiding this comment.
Jetty 11 classes are compiled with java 11 so need to disable test on java 8. As class is poked with reflection before IgnoreIf applies can't use java 11 classes in method signatures because of UnsupportedClassVersionError.
…TomcatServlet3FilterMappingTest.groovy Co-authored-by: Mateusz Rzeszutek <[email protected]>
| import javax.servlet.ServletRegistration; | ||
|
|
||
| public class Servlet3MappingResolverFactory extends ServletMappingResolverFactory | ||
| implements ContextStore.Factory<MappingResolver> { |
There was a problem hiding this comment.
nice, I hadn't noticed ContextStore.Factory before!
Set server span name when request passes through a servlet filter. Names given based on servlet filters have a lower precedence than names from servlets and controllers. If request reaches a servlet or controller then the name given from filter will be replaced.