Servlet 5 API, reorganize servlet modules #2609
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
agoallikmaa
requested review from
anuraaga,
iNikem,
jkwatson,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
trask
reviewed
Mar 22, 2021
...a/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2InstrumentationModule.java
Show resolved
Hide resolved
instrumentation/servlet/servlet-2.2/library/servlet-2.2-library.gradle
Outdated
Show resolved
Hide resolved
...ry/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2HttpServerTracer.java
Outdated
Show resolved
Hide resolved
instrumentation/servlet/servlet-3.0/library/servlet-3.0-library.gradle
Outdated
Show resolved
Hide resolved
...ry/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java
Outdated
Show resolved
Hide resolved
...pentelemetry/javaagent/instrumentation/servlet/javax/dispatcher/RequestDispatcherTracer.java
Outdated
Show resolved
Hide resolved
...pentelemetry/javaagent/instrumentation/servlet/dispatcher/RequestDispatcherAdviceHelper.java
Outdated
Show resolved
Hide resolved
...ava/io/opentelemetry/javaagent/instrumentation/servlet/javax/JavaxCommonServletAccessor.java
Outdated
Show resolved
Hide resolved
trask
approved these changes
Mar 22, 2021
...opentelemetry/javaagent/instrumentation/servlet/v5_0/dispatcher/RequestDispatcherTracer.java
Outdated
Show resolved
Hide resolved
...pentelemetry/javaagent/instrumentation/servlet/javax/dispatcher/RequestDispatcherTracer.java
Outdated
Show resolved
Hide resolved
iNikem
reviewed
Mar 22, 2021
...t-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletAccessor.java
Show resolved
Hide resolved
...lemetry/javaagent/instrumentation/servlet/v5_0/response/HttpServletResponseAdviceHelper.java
Outdated
Show resolved
Hide resolved
...etry/javaagent/instrumentation/servlet/v5_0/dispatcher/RequestDispatcherInstrumentation.java
Outdated
Show resolved
Hide resolved
...lemetry/javaagent/instrumentation/servlet/v5_0/dispatcher/RequestDispatcherAdviceHelper.java
Outdated
Show resolved
Hide resolved
...lemetry/javaagent/instrumentation/servlet/v5_0/dispatcher/RequestDispatcherAdviceHelper.java
Outdated
Show resolved
Hide resolved
instrumentation/servlet/servlet-javax-common/javaagent/servlet-javax-common-javaagent.gradle
Show resolved
Hide resolved
...n/java/io/opentelemetry/javaagent/instrumentation/servlet/javax/response/ResponseTracer.java
Show resolved
Hide resolved
iNikem
approved these changes
Mar 22, 2021
anuraaga
approved these changes
Mar 23, 2021
| private String getSpanName( | ||
| Object servletOrFilter, HttpServletRequest request, boolean allowNull) { | ||
| String spanName = getSpanName(servletOrFilter, request); | ||
| if (spanName == null && !allowNull) { |
There was a problem hiding this comment.
I think I only see this called twice, one with allowNull and one without. Can we move the logic for !allowNull to the caller to reduce conditionals?
There was a problem hiding this comment.
It's good to have as a separate method since return keyword is convenient here and the method name still fits. However, made the caller of true call the inner method instead to still get rid of this extra parameter, and renamed the inner method to getSpanNameFromPath.
| pass { | ||
| group = "jakarta.servlet" | ||
| module = 'jakarta.servlet-api' | ||
| versions = "[5.0.0,)" |
There was a problem hiding this comment.
Can we assertInverse? Since package name changed assuming it's safe so good to verify.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The following changes are made in this PR:
servlet-commonis split up intoservlet-commonandservlet-javax-commonwhere the former does not directly use servlet classes, but defines an interfaceServletAccessor<REQUEST, RESPONSE>which is used to access request and response objects, and the latter applies instrumentations previously found inservlet-common.:instrumentation-core:servlet-2.2is migrated to:instrumentation:servlet:servlet-common:libraryservlet-2.2:javaagentandservlet-3.0:javaagentare split intojavaagentandlibrarymodules where the tracer moves to library.TypeInstrumentationclasses moved toservlet-common:javaagentand they take base package (eitherjavax.servletorjakarta.servlet) and advice class name as constructor parametersservlet-jakartawhich applies instrumentations tojakarta.servletclasses (the ones which are done byservlet-javax-commonandservlet-3.0forjavax.servletpackages)servlet-jakartawhich is Servlet API 5.0 equivalent of the test war project. Added Jetty 11 test images which use the new war file. Manually tested that Jetty tests that do not require thejetty-8.0module pass with those images.To get a general overview of the structure of
instrumentation/servletdirectory, read the updated readme in there.Most of the changes to existing servlet modules were made to reduce code duplication, but some code is still duplicated for the following cases:
servlet-3.0andservlet-jakarta. As they modify local variables injected into instrumented methods, extracting common methods would require some additional allocations and complexity.Servlet3HttpServerTracerandJakartaServletHttpServerTracer, as it uses too many different Servlet API classes to reasonably cover with generics.