From ace4a48881fa21b8745fac93dc3aeb6d58cdfb6f Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Wed, 13 Apr 2022 14:54:12 +0200 Subject: [PATCH 1/2] Do not set the http.route attribute in JSF instrumentations by default --- .../api/config/ExperimentalConfig.java | 5 ++++ .../jsf/JsfServerSpanNaming.java | 29 ++++++++++++++++++- .../src/main/groovy/BaseJsfTest.groovy | 21 ++++++++++++-- .../RestoreViewPhaseInstrumentation.java | 5 +--- .../RestoreViewExecutorInstrumentation.java | 5 +--- .../test/asserts/AttributesAssert.groovy | 2 +- 6 files changed, 55 insertions(+), 12 deletions(-) diff --git a/instrumentation-api-semconv/src/main/java/io/opentelemetry/instrumentation/api/config/ExperimentalConfig.java b/instrumentation-api-semconv/src/main/java/io/opentelemetry/instrumentation/api/config/ExperimentalConfig.java index e869ab3acb2e..a27e284edd84 100644 --- a/instrumentation-api-semconv/src/main/java/io/opentelemetry/instrumentation/api/config/ExperimentalConfig.java +++ b/instrumentation-api-semconv/src/main/java/io/opentelemetry/instrumentation/api/config/ExperimentalConfig.java @@ -38,6 +38,11 @@ public boolean viewTelemetryEnabled() { "otel.instrumentation.common.experimental.view-telemetry.enabled", !suppressViewSpans); } + public boolean useViewNameAsHttpRoute() { + return config.getBoolean( + "otel.instrumentation.common.experimental.view-name-as-http-route.enabled", false); + } + public boolean messagingReceiveInstrumentationEnabled() { // TODO: remove that `suppress...` flag after 1.13 release boolean receiveSpansSuppressed = diff --git a/instrumentation/jsf/jsf-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jsf/JsfServerSpanNaming.java b/instrumentation/jsf/jsf-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jsf/JsfServerSpanNaming.java index 19968bb99b0a..6f62b84fd6b0 100644 --- a/instrumentation/jsf/jsf-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jsf/JsfServerSpanNaming.java +++ b/instrumentation/jsf/jsf-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jsf/JsfServerSpanNaming.java @@ -5,14 +5,24 @@ package io.opentelemetry.javaagent.instrumentation.jsf; +import static io.opentelemetry.instrumentation.api.instrumenter.http.HttpRouteSource.CONTROLLER; + +import io.opentelemetry.api.trace.Span; +import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.api.config.ExperimentalConfig; import io.opentelemetry.instrumentation.api.instrumenter.http.HttpRouteGetter; +import io.opentelemetry.instrumentation.api.instrumenter.http.HttpRouteHolder; +import io.opentelemetry.instrumentation.api.server.ServerSpan; import io.opentelemetry.javaagent.bootstrap.servlet.ServletContextPath; import javax.faces.component.UIViewRoot; import javax.faces.context.FacesContext; public class JsfServerSpanNaming { - public static final HttpRouteGetter SERVER_SPAN_NAME = + private static final boolean USE_VIEW_NAME_AS_HTTP_ROUTE = + ExperimentalConfig.get().useViewNameAsHttpRoute(); + + private static final HttpRouteGetter SERVER_SPAN_NAME = (context, facesContext) -> { UIViewRoot uiViewRoot = facesContext.getViewRoot(); if (uiViewRoot == null) { @@ -26,5 +36,22 @@ public class JsfServerSpanNaming { return ServletContextPath.prepend(context, viewId); }; + public static void updateViewName(Context context, FacesContext facesContext) { + if (USE_VIEW_NAME_AS_HTTP_ROUTE) { + HttpRouteHolder.updateHttpRoute( + context, CONTROLLER, JsfServerSpanNaming.SERVER_SPAN_NAME, facesContext); + } else { + // just update the server span name, without touching the http.route + Span serverSpan = ServerSpan.fromContextOrNull(context); + if (serverSpan == null) { + return; + } + String name = SERVER_SPAN_NAME.get(context, facesContext); + if (name != null) { + serverSpan.updateName(name); + } + } + } + private JsfServerSpanNaming() {} } diff --git a/instrumentation/jsf/jsf-common/testing/src/main/groovy/BaseJsfTest.groovy b/instrumentation/jsf/jsf-common/testing/src/main/groovy/BaseJsfTest.groovy index 55c982ac4c17..836785c27d10 100644 --- a/instrumentation/jsf/jsf-common/testing/src/main/groovy/BaseJsfTest.groovy +++ b/instrumentation/jsf/jsf-common/testing/src/main/groovy/BaseJsfTest.groovy @@ -8,6 +8,7 @@ import io.opentelemetry.instrumentation.test.AgentInstrumentationSpecification import io.opentelemetry.instrumentation.test.asserts.TraceAssert import io.opentelemetry.instrumentation.test.base.HttpServerTestTrait import io.opentelemetry.sdk.trace.data.SpanData +import io.opentelemetry.semconv.trace.attributes.SemanticAttributes import io.opentelemetry.testing.internal.armeria.common.AggregatedHttpRequest import io.opentelemetry.testing.internal.armeria.common.AggregatedHttpResponse import io.opentelemetry.testing.internal.armeria.common.HttpData @@ -82,7 +83,7 @@ abstract class BaseJsfTest extends AgentInstrumentationSpecification implements @Unroll def "test #path"() { setup: - AggregatedHttpResponse response = client.get(address.resolve("hello.jsf").toString()).aggregate().join() + AggregatedHttpResponse response = client.get(address.resolve(path).toString()).aggregate().join() expect: response.status().code() == 200 @@ -95,12 +96,28 @@ abstract class BaseJsfTest extends AgentInstrumentationSpecification implements name getContextPath() + "/hello.xhtml" kind SpanKind.SERVER hasNoParent() + attributes { + "$SemanticAttributes.NET_TRANSPORT" SemanticAttributes.NetTransportValues.IP_TCP + "$SemanticAttributes.NET_PEER_PORT" Long + "$SemanticAttributes.NET_PEER_IP" "127.0.0.1" + "$SemanticAttributes.HTTP_METHOD" "GET" + "$SemanticAttributes.HTTP_SCHEME" "http" + "$SemanticAttributes.HTTP_HOST" { it == "localhost" || it == "localhost:$port" } + "$SemanticAttributes.HTTP_TARGET" "/jetty-context/" + path + "$SemanticAttributes.HTTP_USER_AGENT" TEST_USER_AGENT + "$SemanticAttributes.HTTP_FLAVOR" SemanticAttributes.HttpFlavorValues.HTTP_1_1 + "$SemanticAttributes.HTTP_STATUS_CODE" 200 + "$SemanticAttributes.HTTP_ROUTE" "/jetty-context/" + route + "$SemanticAttributes.HTTP_CLIENT_IP" { it == null || it == TEST_CLIENT_IP } + } } } } where: - path << ['hello.jsf', 'faces/hello.xhtml'] + path | route + "hello.jsf" | "*.jsf" + "faces/hello.xhtml" | "faces/*" } def "test greeting"() { diff --git a/instrumentation/jsf/jsf-mojarra-1.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/mojarra/RestoreViewPhaseInstrumentation.java b/instrumentation/jsf/jsf-mojarra-1.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/mojarra/RestoreViewPhaseInstrumentation.java index 0a7eff416cb3..278b4f00cd7b 100644 --- a/instrumentation/jsf/jsf-mojarra-1.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/mojarra/RestoreViewPhaseInstrumentation.java +++ b/instrumentation/jsf/jsf-mojarra-1.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/mojarra/RestoreViewPhaseInstrumentation.java @@ -5,12 +5,10 @@ package io.opentelemetry.javaagent.instrumentation.mojarra; -import static io.opentelemetry.instrumentation.api.instrumenter.http.HttpRouteSource.CONTROLLER; import static io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge.currentContext; import static net.bytebuddy.matcher.ElementMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; -import io.opentelemetry.instrumentation.api.instrumenter.http.HttpRouteHolder; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import io.opentelemetry.javaagent.instrumentation.jsf.JsfServerSpanNaming; @@ -38,8 +36,7 @@ public static class ExecuteAdvice { @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void onExit(@Advice.Argument(0) FacesContext facesContext) { - HttpRouteHolder.updateHttpRoute( - currentContext(), CONTROLLER, JsfServerSpanNaming.SERVER_SPAN_NAME, facesContext); + JsfServerSpanNaming.updateViewName(currentContext(), facesContext); } } } diff --git a/instrumentation/jsf/jsf-myfaces-1.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/myfaces/RestoreViewExecutorInstrumentation.java b/instrumentation/jsf/jsf-myfaces-1.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/myfaces/RestoreViewExecutorInstrumentation.java index bb1bbf6a8322..721609151f5e 100644 --- a/instrumentation/jsf/jsf-myfaces-1.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/myfaces/RestoreViewExecutorInstrumentation.java +++ b/instrumentation/jsf/jsf-myfaces-1.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/myfaces/RestoreViewExecutorInstrumentation.java @@ -5,12 +5,10 @@ package io.opentelemetry.javaagent.instrumentation.myfaces; -import static io.opentelemetry.instrumentation.api.instrumenter.http.HttpRouteSource.CONTROLLER; import static io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge.currentContext; import static net.bytebuddy.matcher.ElementMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; -import io.opentelemetry.instrumentation.api.instrumenter.http.HttpRouteHolder; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import io.opentelemetry.javaagent.instrumentation.jsf.JsfServerSpanNaming; @@ -38,8 +36,7 @@ public static class ExecuteAdvice { @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void onExit(@Advice.Argument(0) FacesContext facesContext) { - HttpRouteHolder.updateHttpRoute( - currentContext(), CONTROLLER, JsfServerSpanNaming.SERVER_SPAN_NAME, facesContext); + JsfServerSpanNaming.updateViewName(currentContext(), facesContext); } } } diff --git a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/asserts/AttributesAssert.groovy b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/asserts/AttributesAssert.groovy index c9c270ec4d1a..fb5f27f70df2 100644 --- a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/asserts/AttributesAssert.groovy +++ b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/asserts/AttributesAssert.groovy @@ -48,7 +48,7 @@ class AttributesAssert { def methodMissing(String name, args) { if (args.length == 0) { - throw new IllegalArgumentException(args.toString()) + throw new IllegalArgumentException("Attribute $name needs to be provided expected value; zero args were passed: $args") } attribute(name, args[0]) } From daf2fd45d395f840a53fd0d4cf461428f0d340b6 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Thu, 14 Apr 2022 11:57:36 +0200 Subject: [PATCH 2/2] code review comments --- .../api/config/ExperimentalConfig.java | 5 -- .../jsf/JsfServerSpanNaming.java | 53 ++++++------------- 2 files changed, 17 insertions(+), 41 deletions(-) diff --git a/instrumentation-api-semconv/src/main/java/io/opentelemetry/instrumentation/api/config/ExperimentalConfig.java b/instrumentation-api-semconv/src/main/java/io/opentelemetry/instrumentation/api/config/ExperimentalConfig.java index a27e284edd84..e869ab3acb2e 100644 --- a/instrumentation-api-semconv/src/main/java/io/opentelemetry/instrumentation/api/config/ExperimentalConfig.java +++ b/instrumentation-api-semconv/src/main/java/io/opentelemetry/instrumentation/api/config/ExperimentalConfig.java @@ -38,11 +38,6 @@ public boolean viewTelemetryEnabled() { "otel.instrumentation.common.experimental.view-telemetry.enabled", !suppressViewSpans); } - public boolean useViewNameAsHttpRoute() { - return config.getBoolean( - "otel.instrumentation.common.experimental.view-name-as-http-route.enabled", false); - } - public boolean messagingReceiveInstrumentationEnabled() { // TODO: remove that `suppress...` flag after 1.13 release boolean receiveSpansSuppressed = diff --git a/instrumentation/jsf/jsf-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jsf/JsfServerSpanNaming.java b/instrumentation/jsf/jsf-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jsf/JsfServerSpanNaming.java index 6f62b84fd6b0..a4f01eb0fccc 100644 --- a/instrumentation/jsf/jsf-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jsf/JsfServerSpanNaming.java +++ b/instrumentation/jsf/jsf-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jsf/JsfServerSpanNaming.java @@ -5,52 +5,33 @@ package io.opentelemetry.javaagent.instrumentation.jsf; -import static io.opentelemetry.instrumentation.api.instrumenter.http.HttpRouteSource.CONTROLLER; - import io.opentelemetry.api.trace.Span; import io.opentelemetry.context.Context; -import io.opentelemetry.instrumentation.api.config.ExperimentalConfig; -import io.opentelemetry.instrumentation.api.instrumenter.http.HttpRouteGetter; -import io.opentelemetry.instrumentation.api.instrumenter.http.HttpRouteHolder; import io.opentelemetry.instrumentation.api.server.ServerSpan; import io.opentelemetry.javaagent.bootstrap.servlet.ServletContextPath; import javax.faces.component.UIViewRoot; import javax.faces.context.FacesContext; -public class JsfServerSpanNaming { - - private static final boolean USE_VIEW_NAME_AS_HTTP_ROUTE = - ExperimentalConfig.get().useViewNameAsHttpRoute(); - - private static final HttpRouteGetter SERVER_SPAN_NAME = - (context, facesContext) -> { - UIViewRoot uiViewRoot = facesContext.getViewRoot(); - if (uiViewRoot == null) { - return null; - } - - // JSF spec 7.6.2 - // view id is a context relative path to the web application resource that produces the - // view, such as a JSP page or a Facelets page. - String viewId = uiViewRoot.getViewId(); - return ServletContextPath.prepend(context, viewId); - }; +public final class JsfServerSpanNaming { public static void updateViewName(Context context, FacesContext facesContext) { - if (USE_VIEW_NAME_AS_HTTP_ROUTE) { - HttpRouteHolder.updateHttpRoute( - context, CONTROLLER, JsfServerSpanNaming.SERVER_SPAN_NAME, facesContext); - } else { - // just update the server span name, without touching the http.route - Span serverSpan = ServerSpan.fromContextOrNull(context); - if (serverSpan == null) { - return; - } - String name = SERVER_SPAN_NAME.get(context, facesContext); - if (name != null) { - serverSpan.updateName(name); - } + // just update the server span name, without touching the http.route + Span serverSpan = ServerSpan.fromContextOrNull(context); + if (serverSpan == null) { + return; } + + UIViewRoot uiViewRoot = facesContext.getViewRoot(); + if (uiViewRoot == null) { + return; + } + + // JSF spec 7.6.2 + // view id is a context relative path to the web application resource that produces the + // view, such as a JSP page or a Facelets page. + String viewId = uiViewRoot.getViewId(); + String name = ServletContextPath.prepend(context, viewId); + serverSpan.updateName(name); } private JsfServerSpanNaming() {}