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.

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

Do not set the http.route attribute in JSF instrumentations by default #5819

Merged
merged 2 commits into from
Apr 14, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
mateuszrzeszutek marked this conversation as resolved.
Show resolved Hide resolved

public boolean messagingReceiveInstrumentationEnabled() {
// TODO: remove that `suppress...` flag after 1.13 release
boolean receiveSpansSuppressed =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<FacesContext> SERVER_SPAN_NAME =
private static final boolean USE_VIEW_NAME_AS_HTTP_ROUTE =
ExperimentalConfig.get().useViewNameAsHttpRoute();

private static final HttpRouteGetter<FacesContext> SERVER_SPAN_NAME =
(context, facesContext) -> {
UIViewRoot uiViewRoot = facesContext.getViewRoot();
if (uiViewRoot == null) {
Expand All @@ -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() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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"() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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])
}
Expand Down