Add library instrumentation for servlet #15473
Conversation
| @@ -0,0 +1,43 @@ | |||
| plugins { | |||
There was a problem hiding this comment.
I renamed testing to javaagent-testing for some reason this file didn't register as being moved :(
|
|
||
| /** Sets the status extractor for server spans. */ | ||
| @CanIgnoreReturnValue | ||
| public ServletTelemetryBuilder setStatusExtractor( |
There was a problem hiding this comment.
Currently ServletRequestContext is in internal package we have to either move it out of the internal or alternatively we could change the signature to UnaryOperator<SpanStatusExtractor<HttpServletRequest, HttpServletResponse>>. The wrappers are there mostly so we could share code between servlet 3 & 5 instrumentations. ServletResponseContext has some fields but ServletRequestContext doesn't have any and is probably there only for symmetry. If we don't use the wrappers in the signature then the downside is that conversion will be needed
builder.setStatusExtractor(
inputExtractor -> {
SpanStatusExtractor<HttpServletRequest, HttpServletResponse> outputExtractor =
statusExtractor.apply(
(spanStatusBuilder, request, response, error) ->
inputExtractor.extract(
spanStatusBuilder,
new ServletRequestContext<>(request),
response == null ? null : new ServletResponseContext<>(response),
error));
return (spanStatusBuilder, requestContext, responseContext, error) ->
outputExtractor.extract(
spanStatusBuilder,
requestContext.request(),
responseContext == null ? null : responseContext.response(),
error);
});@trask which way would you prefer?
There was a problem hiding this comment.
removed wrapper types from the ServletTelemetryBuilder public api to try it out
| jvmArgs("-XX:+IgnoreUnrecognizedVMOptions") | ||
| } | ||
|
|
||
| if (findProperty("testLatestDeps") as Boolean) { |
There was a problem hiding this comment.
not using otelJava because that would apply to the main source too which would break the instrumentation for libraries that only work with java 8 when tests are run with -PtestLatestDeps=true. The same approach could be used to get rid of the javaagent-testing module and move the tests back to javaagent.
There was a problem hiding this comment.
can you add this as a code comment?
...ain/java/io/opentelemetry/instrumentation/servlet/v3_0/internal/Servlet3TelemetryFilter.java
Outdated
Show resolved
Hide resolved
| otelRequest.asyncException = error; | ||
| } | ||
| } else { | ||
| instrumenter.end(context, requestContext, responseContext, error); |
There was a problem hiding this comment.
Are there any options to enable customizing the metric attributes for the metrics generated by this instrumenter?
There was a problem hiding this comment.
We pass all the attributes produced by the AttributesExtractors to the metrics. Users can choose which of the attributes are used by configuring a metrics view.
| } | ||
|
|
||
| /** Returns a new {@link Filter} for producing telemetry. */ | ||
| public Filter newFilter() { |
There was a problem hiding this comment.
I think it would be helpful for consumers of this library to have a no-arg-constructor public implementation of Filter that can be added directly to web.xml, but it would not be difficult for them to implement it themselves so I don't see this as a blocker.
There was a problem hiding this comment.
Providing a filter like that is problematic because it raises the questions where does the OpenTelemetry instance come from and how is that filter configured. With the current api user can have their own filter which creates the telemetry filter and delegates to it. This way users are responsible for providing OpenTelemetry instance and the configuration is handled in the builder class as for our other library instrumentations.
If you believe that a dedicated filter would be useful you are welcome to work on this in a subsequent PR.
There was a problem hiding this comment.
I understand. It is just a mild suggestion that I wanted to mention. I'll consider contributing something with default config values and GlobalOpenTelemetry later on.
tylerbenson
left a comment
tylerbenson
left a comment
There was a problem hiding this comment.
I appreciate you taking the time to implement this. From what I can tell, this is a reasonable implementation and works as I expect. I look forward to this being released.
Thanks!
There was a problem hiding this comment.
Pull request overview
This PR adds library instrumentation for Java Servlet 3.0+ by extracting servlet instrumentation code from javaagent-only modules into shared library modules. This enables users to manually instrument their servlet applications without requiring the Java agent.
Key changes:
- Creates new library modules for servlet instrumentation (servlet-common:library, servlet-javax-common:library, servlet-3.0:library)
- Refactors servlet classes from javaagent packages to library internal packages
- Consolidates servlet test classes into shared testing modules with parameterized tests
- Updates dependent modules to use the new library modules instead of javaagent modules
Reviewed changes
Copilot reviewed 126 out of 134 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| settings.gradle.kts | Adds new library and testing module declarations |
| instrumentation/servlet/servlet-3.0/library/* | New library instrumentation module with public API (ServletTelemetry, ServletTelemetryBuilder) |
| instrumentation/servlet/servlet-common/library/* | Shared servlet library code moved from javaagent with internal packages |
| instrumentation/servlet/servlet-3.0/testing/* | Refactored test modules into shared base classes |
| instrumentation/servlet/servlet-3.0/javaagent-testing/* | New javaagent-specific test module |
| instrumentation/servlet/servlet-javax-common/javaagent/build.gradle.kts | Deleted - functionality moved to library module |
| Multiple build.gradle.kts files | Updated dependencies from servlet-javax-common:javaagent to library modules |
| instrumentation/tomcat/, instrumentation/jetty/, etc. | Updated imports to use new servlet internal package locations |
| testing-common/.../AbstractHttpServerTest.java | Changed setupOptions visibility from package-private to protected |
| docs/supported-libraries.md | Updated servlet documentation with library module link |
|
🎉 Thanks for reviewing @tylerbenson! |
3 participants
Supersedes #15188