Distinguish between the various servlet tracers for easier debugging. #1979
Conversation
jkwatson
requested review from
anuraaga,
iNikem,
mateuszrzeszutek,
trask and
tylerbenson
as code owners
|
hey @jkwatson, I was assuming that the instrumentation library name is supposed to be the same for all instrumentation produced by a given instrumentation library, which would rule out the for adding the version number to the library name, we used to have that, but discussed and removed it in #1260. definitely open to revisiting that discussion tho. is this a case where you were debugging without access to the javaagent debug log? |
I'd be fine with that, although I don't think it matters too much, one way or the other.
Yes, all we had were a couple of sample spans in the final observability system. Being able to detect what servlet version was being instrumented would have been very useful. |
can you create an issue to revisit the decision made in #1260? I'd like to give others a chance to weigh in, and then apply the change consistently across all instrumentation |
| @@ -16,7 +16,7 @@ public static Servlet2HttpServerTracer tracer() { | |||
|
|
|||
| @Override | |||
| protected String getInstrumentationName() { | |||
| return "io.opentelemetry.javaagent.servlet"; | |||
There was a problem hiding this comment.
These actually seem like a no-brainer to me - our artifact names are also e.g., opentelemetry-javaagent-servlet-2.2, the name of the library. And these are supposed to reflect the name of the library.
I understand the potential for confusion due to the lack of something like a + but I think it's just the nature of how instrumentation libraries are named.
| @@ -16,6 +16,6 @@ public static RequestDispatcherTracer tracer() { | |||
|
|
|||
| @Override | |||
| protected String getInstrumentationName() { | |||
| return "io.opentelemetry.javaagent.servlet"; | |||
| return "io.opentelemetry.javaagent.servlet.request"; | |||
There was a problem hiding this comment.
I don't know if I like it but I guess for consistency and to reflect "name of instrumentation library", these should use .common?
There was a problem hiding this comment.
yeah, I wasn't really sure what these should say. .common seems to accurately and uniquely identify the instrumentation, though, and that's definitely the most important thing.
|
...n/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2HttpServerTracer.java
Outdated
Show resolved
Hide resolved
...n/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java
Outdated
Show resolved
Hide resolved
...a/io/opentelemetry/javaagent/instrumentation/servlet/dispatcher/RequestDispatcherTracer.java
Outdated
Show resolved
Hide resolved
.../java/io/opentelemetry/javaagent/instrumentation/servlet/http/HttpServletResponseTracer.java
Outdated
Show resolved
Hide resolved
…/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2HttpServerTracer.java Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
…/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
…/io/opentelemetry/javaagent/instrumentation/servlet/dispatcher/RequestDispatcherTracer.java Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
…/io/opentelemetry/javaagent/instrumentation/servlet/http/HttpServletResponseTracer.java Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
|
Anybody wants to chime in on this before we merge? I don't :) |
While trying to track down an issue with a customer, I ran into the fact that I couldn't figure out which servlet instrumentation was generating the spans. This will help with that.