Convert servlet instrumentation to instrumenter api #4078
Conversation
laurit
requested review from
anuraaga,
iNikem,
jkwatson,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
...ry/src/main/java/io/opentelemetry/instrumentation/servlet/ServletNetAttributesExtractor.java
Outdated
Show resolved
Hide resolved
| @Advice.Local("otelContext") Context context, | ||
| @Advice.Local("otelScope") Scope scope) { | ||
|
|
||
| Context attachedContext = tracer().getServerContext(request); | ||
| Context attachedContext = helper().getServerContext(request); |
There was a problem hiding this comment.
That's something definitely not for this PR, but I wonder if we can simplify this and use the shouldStart() check to verify if there's an ongoing SERVER span. Context should be propagated anyway, we shouldn't have to do that manually.
There was a problem hiding this comment.
This should be jetty specific. We instrument all implementations of org.eclipse.jetty.server.Handler and need to make sure that we start span only in the outermost one. I believe that we should use shouldStart to be consistent with other instrumentations, but I suspect that we can't just replace the existing code with it because shouldStart counts suppressed spans and using it for jetty would blow that number up.
...aagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8Helper.java
Outdated
Show resolved
Hide resolved
...agent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/common/JettyHelper.java
Outdated
Show resolved
Hide resolved
...agent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/common/JettyHelper.java
Outdated
Show resolved
Hide resolved
| String contextPath = accessor.getRequestContextPath(request); | ||
| if (contextPath != null && !contextPath.isEmpty() && !contextPath.equals("/")) { | ||
| return context.with(ServletContextPath.CONTEXT_KEY, contextPath); | ||
| } | ||
| return context; |
There was a problem hiding this comment.
Idea for another PR: this could be encapsulated in ServletContextPath, kind of like ServerSpanNaming does (we should not expose ContextKeys as public vars)
...common/library/src/main/java/io/opentelemetry/instrumentation/servlet/BaseServletHelper.java
Outdated
Show resolved
Hide resolved
| * current request to context if it isn't already added. Servlet instrumentation adds it when it | ||
| * starts server span. | ||
| */ | ||
| public Context updateContext(Context context, REQUEST request) { |
There was a problem hiding this comment.
Could this method be merged with addServletContextPath()? Checking if current context already contains ServletContextPath.CONTEXT_KEY shouldn't probably hurt in startSpan()
There was a problem hiding this comment.
I'll try to do this in a separate pr along with making ServletContextPath. CONTEXT_KEY private.
...let-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHelper.java
Outdated
Show resolved
Hide resolved
...let-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHelper.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Mateusz Rzeszutek <[email protected]>
…n't have any methods, add shouldStart checks
.../src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletRequestContext.java
Show resolved
Hide resolved
...gent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/BaseServletHelper.java
Outdated
Show resolved
Hide resolved
…/io/opentelemetry/javaagent/instrumentation/servlet/ServletRequestContext.java Co-authored-by: Mateusz Rzeszutek <[email protected]>
|
👏 thx @laurit (and @mateuszrzeszutek for reviewing)! I'm going to go ahead and merge to unblock follow-on work, and I'll review in more detail later |
Part of #2713
Converts servlet, jetty and tomcat instrumentation to instrumenter api. Jetty is converted along with servlet to give an idea how server instrumenation that relies on servlet would look. Similarly tomcat is converted along with servlet to give an idea how server instrumenation that does not rely on servlet would look.
This pr deliberately avoids filling more attributes than what tracer api filled to avoid changing test and making this pr event larger. Code to fill more attributes is left commented out.