Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove a few ServerSpanNaming usages #4900

Merged
merged 2 commits into from
Dec 21, 2021
Merged

Remove a few ServerSpanNaming usages #4900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a88801b
Remove a few ServerSpanNaming usages
Dec 14, 2021
bec560b
revert vaadin changes
Dec 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...c/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsServerSpanNaming.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ public class GrailsServerSpanNaming {
info.getActionName() != null
? info.getActionName()
: info.getControllerClass().getDefaultAction();
// this is not the actual route/mapping, but it's the best thing that we have access to
return ServletContextPath.prepend(context, "/" + info.getControllerName() + "/" + action);
};

Expand Down
5 changes: 1 addition & 4 deletions ...javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/axis2/Axis2Helper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,10 @@

package io.opentelemetry.javaagent.instrumentation.axis2;

import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTROLLER;
import static io.opentelemetry.javaagent.instrumentation.axis2.Axis2Singletons.instrumenter;

import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
import org.apache.axis2.jaxws.core.MessageContext;

public final class Axis2Helper {
Expand All @@ -24,8 +22,7 @@ public static void start(MessageContext message) {
Context parentContext = Context.current();

Axis2Request request = new Axis2Request(message);
ServerSpanNaming.updateServerSpanName(
parentContext, CONTROLLER, Axis2ServerSpanNaming.SERVER_SPAN_NAME, request);
Axis2ServerSpanNaming.updateServerSpan(parentContext, request);

if (!instrumenter().shouldStart(parentContext, request)) {
return;
Expand Down
38 changes: 22 additions & 16 deletions ...src/main/java/io/opentelemetry/javaagent/instrumentation/axis2/Axis2ServerSpanNaming.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,34 @@

package io.opentelemetry.javaagent.instrumentation.axis2;

import io.opentelemetry.instrumentation.api.servlet.ServerSpanNameSupplier;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.tracer.ServerSpan;
import io.opentelemetry.javaagent.bootstrap.servlet.ServletContextPath;
import javax.servlet.http.HttpServletRequest;
import org.apache.axis2.jaxws.core.MessageContext;

public class Axis2ServerSpanNaming {
public final class Axis2ServerSpanNaming {

public static final ServerSpanNameSupplier<Axis2Request> SERVER_SPAN_NAME =
(context, axis2Request) -> {
String spanName = axis2Request.spanName();
MessageContext message = axis2Request.message();
HttpServletRequest request =
(HttpServletRequest) message.getMEPContext().get("transport.http.servletRequest");
if (request != null) {
String servletPath = request.getServletPath();
if (!servletPath.isEmpty()) {
spanName = servletPath + "/" + spanName;
}
}
public static void updateServerSpan(Context context, Axis2Request axis2Request) {
Span serverSpan = ServerSpan.fromContextOrNull(context);
if (serverSpan == null) {
return;
}

return ServletContextPath.prepend(context, spanName);
};
String spanName = axis2Request.spanName();
MessageContext message = axis2Request.message();
HttpServletRequest request =
(HttpServletRequest) message.getMEPContext().get("transport.http.servletRequest");
if (request != null) {
String servletPath = request.getServletPath();
if (!servletPath.isEmpty()) {
spanName = servletPath + "/" + spanName;
}
}

serverSpan.updateName(ServletContextPath.prepend(context, spanName));
}

private Axis2ServerSpanNaming() {}
}
5 changes: 1 addition & 4 deletions ...3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/cxf/CxfHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,10 @@

package io.opentelemetry.javaagent.instrumentation.cxf;

import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTROLLER;
import static io.opentelemetry.javaagent.instrumentation.cxf.CxfSingletons.instrumenter;

import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
import org.apache.cxf.interceptor.Fault;
import org.apache.cxf.message.Exchange;
import org.apache.cxf.message.Message;
Expand All @@ -31,8 +29,7 @@ public static void start(Message message) {
return;
}

ServerSpanNaming.updateServerSpanName(
parentContext, CONTROLLER, CxfServerSpanNaming.SERVER_SPAN_NAME, request);
CxfServerSpanNaming.updateServerSpanName(parentContext, request);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One problem with not using ServerSpanNaming.updateServerSpanName is that when there is an exception and request is dispatched to exception handling servlet it can overwrite the name set by the original request. ServerSpanNaming avoided it by disallowing name update when new name had the same or lower priority as the name set.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that really happen though? We don't really call updateServerSpanName at the end of processing; there's one invocation in finally block in OpenTelemetryHandlerMappingFilter in spring-webmvc, but it's a web controller framework - it won't run jax-ws code in a controller (probably).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In web.xml you can set error pages where container will dispatch when there is an exception or when request status is 500 or whatever. This error handler can be a servlet and could potentially also try to update server span name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, that's a valid scenario. Hmm, is this behavior that important, should we keep it? Or would it be fine to just ignore that case (since it's just jax-ws; and maybe jax-ws has its own exception handler interface or something)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that for jax-ws it is ok to ignore it. It needs to send errors back as a soap response so it can't use servlet error handling.


if (!instrumenter().shouldStart(parentContext, request)) {
return;
Expand Down
46 changes: 26 additions & 20 deletions ...ent/src/main/java/io/opentelemetry/javaagent/instrumentation/cxf/CxfServerSpanNaming.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,29 +5,35 @@

package io.opentelemetry.javaagent.instrumentation.cxf;

import io.opentelemetry.instrumentation.api.servlet.ServerSpanNameSupplier;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.tracer.ServerSpan;
import io.opentelemetry.javaagent.bootstrap.servlet.ServletContextPath;
import javax.servlet.http.HttpServletRequest;

public class CxfServerSpanNaming {

public static final ServerSpanNameSupplier<CxfRequest> SERVER_SPAN_NAME =
(context, cxfRequest) -> {
String spanName = cxfRequest.spanName();
if (spanName == null) {
return null;
}

HttpServletRequest request = (HttpServletRequest) cxfRequest.message().get("HTTP.REQUEST");
if (request != null) {
String servletPath = request.getServletPath();
if (!servletPath.isEmpty()) {
spanName = servletPath + "/" + spanName;
}
}

return ServletContextPath.prepend(context, spanName);
};
public final class CxfServerSpanNaming {

public static void updateServerSpanName(Context context, CxfRequest cxfRequest) {
String spanName = cxfRequest.spanName();
if (spanName == null) {
return;
}

Span serverSpan = ServerSpan.fromContextOrNull(context);
if (serverSpan == null) {
return;
}

HttpServletRequest request = (HttpServletRequest) cxfRequest.message().get("HTTP.REQUEST");
if (request != null) {
String servletPath = request.getServletPath();
if (!servletPath.isEmpty()) {
spanName = servletPath + "/" + spanName;
}
}

serverSpan.updateName(ServletContextPath.prepend(context, spanName));
}

private CxfServerSpanNaming() {}
}
5 changes: 1 addition & 4 deletions ...javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/metro/MetroHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,12 @@

package io.opentelemetry.javaagent.instrumentation.metro;

import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTROLLER;
import static io.opentelemetry.javaagent.instrumentation.metro.MetroSingletons.instrumenter;

import com.sun.xml.ws.api.message.Packet;
import com.sun.xml.ws.api.server.WSEndpoint;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;

public final class MetroHelper {
private static final String REQUEST_KEY = MetroHelper.class.getName() + ".Request";
Expand All @@ -26,8 +24,7 @@ public static void start(WSEndpoint endpoint, Packet packet) {
Context parentContext = Context.current();

MetroRequest request = new MetroRequest(endpoint, packet);
ServerSpanNaming.updateServerSpanName(
parentContext, CONTROLLER, MetroServerSpanNaming.SERVER_SPAN_NAME, request);
MetroServerSpanNaming.updateServerSpanName(parentContext, request);

if (!instrumenter().shouldStart(parentContext, request)) {
return;
Expand Down
57 changes: 31 additions & 26 deletions ...src/main/java/io/opentelemetry/javaagent/instrumentation/metro/MetroServerSpanNaming.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,40 +6,45 @@
package io.opentelemetry.javaagent.instrumentation.metro;

import com.sun.xml.ws.api.message.Packet;
import io.opentelemetry.instrumentation.api.servlet.ServerSpanNameSupplier;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.tracer.ServerSpan;
import io.opentelemetry.javaagent.bootstrap.servlet.ServletContextPath;
import javax.servlet.http.HttpServletRequest;
import javax.xml.ws.handler.MessageContext;

public class MetroServerSpanNaming {
public final class MetroServerSpanNaming {

public static final ServerSpanNameSupplier<MetroRequest> SERVER_SPAN_NAME =
(context, metroRequest) -> {
String spanName = metroRequest.spanName();
if (spanName == null) {
return null;
}
public static void updateServerSpanName(Context context, MetroRequest metroRequest) {
String spanName = metroRequest.spanName();
if (spanName == null) {
return;
}

Span serverSpan = ServerSpan.fromContextOrNull(context);
if (serverSpan == null) {
return;
}

Packet packet = metroRequest.packet();
HttpServletRequest request =
(HttpServletRequest) packet.get(MessageContext.SERVLET_REQUEST);
if (request != null) {
String servletPath = request.getServletPath();
if (!servletPath.isEmpty()) {
String pathInfo = request.getPathInfo();
if (pathInfo != null) {
spanName = servletPath + "/" + spanName;
} else {
// when pathInfo is null then there is a servlet that is mapped to this exact service
// servletPath already contains the service name
String operationName = packet.getWSDLOperation().getLocalPart();
spanName = servletPath + "/" + operationName;
}
}
Packet packet = metroRequest.packet();
HttpServletRequest request = (HttpServletRequest) packet.get(MessageContext.SERVLET_REQUEST);
if (request != null) {
String servletPath = request.getServletPath();
if (!servletPath.isEmpty()) {
String pathInfo = request.getPathInfo();
if (pathInfo != null) {
spanName = servletPath + "/" + spanName;
} else {
// when pathInfo is null then there is a servlet that is mapped to this exact service
// servletPath already contains the service name
String operationName = packet.getWSDLOperation().getLocalPart();
spanName = servletPath + "/" + operationName;
}
}
}

return ServletContextPath.prepend(context, spanName);
};
serverSpan.updateName(ServletContextPath.prepend(context, spanName));
}

private MetroServerSpanNaming() {}
}
5 changes: 0 additions & 5 deletions ...opentelemetry/javaagent/instrumentation/jaxws/v2_0/WebServiceProviderInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

package io.opentelemetry.javaagent.instrumentation.jaxws.v2_0;

import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTROLLER;
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasClassesNamed;
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.implementsInterface;
import static io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge.currentContext;
Expand All @@ -18,12 +17,10 @@

import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
import io.opentelemetry.javaagent.instrumentation.api.CallDepth;
import io.opentelemetry.javaagent.instrumentation.jaxws.common.JaxWsRequest;
import io.opentelemetry.javaagent.instrumentation.jaxws.common.JaxWsServerSpanNaming;
import javax.xml.ws.Provider;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.type.TypeDescription;
Expand Down Expand Up @@ -66,8 +63,6 @@ public static void startSpan(

Context parentContext = currentContext();
request = new JaxWsRequest(target.getClass(), methodName);
ServerSpanNaming.updateServerSpanName(
parentContext, CONTROLLER, JaxWsServerSpanNaming.SERVER_SPAN_NAME, request);
if (!instrumenter().shouldStart(parentContext, request)) {
return;
}
Expand Down
16 changes: 0 additions & 16 deletions ...n/java/io/opentelemetry/javaagent/instrumentation/jaxws/common/JaxWsServerSpanNaming.java
View file
Open in desktop

This file was deleted.

5 changes: 0 additions & 5 deletions ...opentelemetry/javaagent/instrumentation/jaxws/jws/v1_1/JwsAnnotationsInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

package io.opentelemetry.javaagent.instrumentation.jaxws.jws.v1_1;

import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTROLLER;
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasClassesNamed;
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasSuperMethod;
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.implementsInterface;
Expand All @@ -20,12 +19,10 @@

import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
import io.opentelemetry.javaagent.instrumentation.api.CallDepth;
import io.opentelemetry.javaagent.instrumentation.jaxws.common.JaxWsRequest;
import io.opentelemetry.javaagent.instrumentation.jaxws.common.JaxWsServerSpanNaming;
import javax.jws.WebService;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.type.TypeDescription;
Expand Down Expand Up @@ -77,8 +74,6 @@ public static void startSpan(

Context parentContext = currentContext();
request = new JaxWsRequest(target.getClass(), methodName);
ServerSpanNaming.updateServerSpanName(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would have been relevant only when request is handled by a jax-ws framework that we don't have integration with (or when framework instrumentation is disabled or when there is a bug that prevents it from working properly) because jax-ws framework instrumentation runs before it and already sets the name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep -- and it sets a <class>.<method> span name, so I thought we won't lose that much (if anything) if I remove that.

parentContext, CONTROLLER, JaxWsServerSpanNaming.SERVER_SPAN_NAME, request);
if (!instrumenter().shouldStart(parentContext, request)) {
return;
}
Expand Down
2 changes: 1 addition & 1 deletion ...opentelemetry/javaagent/instrumentation/wicket/RequestHandlerExecutorInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public static void onExit(@Advice.Argument(0) IRequestHandler handler) {
ServerSpanNaming.updateServerSpanName(
context,
CONTROLLER,
WicketServerSpanNameing.SERVER_SPAN_NAME,
WicketServerSpanNaming.SERVER_SPAN_NAME,
(IPageClassRequestHandler) handler);
}
}
Expand Down
4 changes: 2 additions & 2 deletions ...ation/wicket/WicketServerSpanNameing.java → ...tation/wicket/WicketServerSpanNaming.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import org.apache.wicket.core.request.handler.IPageClassRequestHandler;
import org.apache.wicket.request.cycle.RequestCycle;

public class WicketServerSpanNameing {
public final class WicketServerSpanNaming {

public static final ServerSpanNameSupplier<IPageClassRequestHandler> SERVER_SPAN_NAME =
(context, handler) -> {
Expand All @@ -22,5 +22,5 @@ public class WicketServerSpanNameing {
return ServletContextPath.prepend(context, filterPath + "/" + pageName);
};

private WicketServerSpanNameing() {}
private WicketServerSpanNaming() {}
}