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

Refactor ServerSpanNaming (in preparation for http.route) #4852

Merged
merged 4 commits into from
Dec 14, 2021
Merged

Refactor ServerSpanNaming (in preparation for http.route) #4852

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
22f4f5c
Refactor ServerSpanNaming (in preparation for http.route)
Dec 9, 2021
a8a225a
fix tests
Dec 9, 2021
8a833b5
Add ServerSpanNaming to all HTTP server instrumentations
Dec 10, 2021
580fe68
fix tests
Dec 10, 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
51 changes: 24 additions & 27 deletions ...tion-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.ContextKey;
import io.opentelemetry.instrumentation.api.instrumenter.ContextCustomizer;
import io.opentelemetry.instrumentation.api.tracer.ServerSpan;
import javax.annotation.Nullable;

Expand All @@ -17,14 +18,13 @@ public final class ServerSpanNaming {
private static final ContextKey<ServerSpanNaming> CONTEXT_KEY =
ContextKey.named("opentelemetry-servlet-span-naming-key");

public static Context init(Context context, Source initialSource) {
ServerSpanNaming serverSpanNaming = context.get(CONTEXT_KEY);
if (serverSpanNaming != null) {
// TODO (trask) does this ever happen?
serverSpanNaming.updatedBySource = initialSource;
return context;
}
return context.with(CONTEXT_KEY, new ServerSpanNaming(initialSource));
public static <REQUEST> ContextCustomizer<REQUEST> get() {
return (context, request, startAttributes) -> {
if (context.get(CONTEXT_KEY) != null) {
return context;
}
return context.with(CONTEXT_KEY, new ServerSpanNaming(Source.CONTAINER));
};
}

private volatile Source updatedBySource;
Expand All @@ -37,34 +37,31 @@ private ServerSpanNaming(Source initialSource) {
}

/**
* If there is a server span in the context, and {@link #init(Context, Source)} has been called to
* populate a {@code ServerSpanName} into the context, then this method will update the server
* span name using the provided {@link ServerSpanNameSupplier} if and only if the last {@link
* Source} to update the span name using this method has strictly lower priority than the provided
* {@link Source}, and the value returned from the {@link ServerSpanNameSupplier} is non-null.
* If there is a server span in the context, and the context has been customized with a {@code
* ServerSpanName}, then this method will update the server span name using the provided {@link
* ServerSpanNameSupplier} if and only if the last {@link Source} to update the span name using
* this method has strictly lower priority than the provided {@link Source}, and the value
* returned from the {@link ServerSpanNameSupplier} is non-null.
*
* <p>If there is a server span in the context, and {@link #init(Context, Source)} has NOT been
* called to populate a {@code ServerSpanName} into the context, then this method will update the
* server span name using the provided {@link ServerSpanNameSupplier} if the value returned from
* it is non-null.
* <p>If there is a server span in the context, and the context has NOT been customized with a
* {@code ServerSpanName}, then this method will update the server span name using the provided
* {@link ServerSpanNameSupplier} if the value returned from it is non-null.
*/
public static <T> void updateServerSpanName(
Context context, Source source, ServerSpanNameSupplier<T> serverSpanName, T arg1) {
updateServerSpanName(context, source, OneArgAdapter.getInstance(), arg1, serverSpanName);
}

/**
* If there is a server span in the context, and {@link #init(Context, Source)} has been called to
* populate a {@code ServerSpanName} into the context, then this method will update the server
* span name using the provided {@link ServerSpanNameTwoArgSupplier} if and only if the last
* {@link Source} to update the span name using this method has strictly lower priority than the
* provided {@link Source}, and the value returned from the {@link ServerSpanNameTwoArgSupplier}
* is non-null.
* If there is a server span in the context, and the context has been customized with a {@code
* ServerSpanName}, then this method will update the server span name using the provided {@link
* ServerSpanNameTwoArgSupplier} if and only if the last {@link Source} to update the span name
* using this method has strictly lower priority than the provided {@link Source}, and the value
* returned from the {@link ServerSpanNameTwoArgSupplier} is non-null.
*
* <p>If there is a server span in the context, and {@link #init(Context, Source)} has NOT been
* called to populate a {@code ServerSpanName} into the context, then this method will update the
* server span name using the provided {@link ServerSpanNameTwoArgSupplier} if the value returned
* from it is non-null.
* <p>If there is a server span in the context, and the context has NOT been customized with a
* {@code ServerSpanName}, then this method will update the server span name using the provided
* {@link ServerSpanNameTwoArgSupplier} if the value returned from it is non-null.
*/
public static <T, U> void updateServerSpanName(
Context context,
Expand Down
6 changes: 2 additions & 4 deletions ...t/src/main/java/io/opentelemetry/javaagent/instrumentation/grizzly/GrizzlySingletons.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,8 @@ public final class GrizzlySingletons {
.addAttributesExtractor(netAttributesExtractor)
.addRequestMetrics(HttpServerMetrics.get())
.addContextCustomizer(
(context, httpRequestPacket, startAttributes) -> {
context = GrizzlyErrorHolder.init(context);
return ServerSpanNaming.init(context, ServerSpanNaming.Source.CONTAINER);
})
(context, httpRequestPacket, startAttributes) -> GrizzlyErrorHolder.init(context))
.addContextCustomizer(ServerSpanNaming.get())
.newServerInstrumenter(HttpRequestHeadersGetter.INSTANCE);
}

Expand Down
8 changes: 1 addition & 7 deletions ...c/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11Singletons.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,7 @@

package io.opentelemetry.javaagent.instrumentation.jetty.v11_0;

import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTAINER;

import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
import io.opentelemetry.javaagent.bootstrap.servlet.AppServerBridge;
import io.opentelemetry.javaagent.instrumentation.jetty.common.JettyHelper;
import io.opentelemetry.javaagent.instrumentation.servlet.ServletInstrumenterBuilder;
Expand All @@ -26,10 +23,7 @@ public final class Jetty11Singletons {
INSTRUMENTER =
ServletInstrumenterBuilder.<HttpServletRequest, HttpServletResponse>create()
.addContextCustomizer(
(context, request, attributes) -> {
context = ServerSpanNaming.init(context, CONTAINER);
return new AppServerBridge.Builder().init(context);
})
(context, request, attributes) -> new AppServerBridge.Builder().init(context))
Copy link
Member

Choose a reason for hiding this comment

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

should ServerSpanNaming.get() context customizer be added here? I notice it's missing in some cases and included in others, but not clear to me why

Copy link
Member Author

Choose a reason for hiding this comment

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

It actually is added, via the ServletInstrumenterBuilder class.
All server instrumentations should add ServerSpanNaming.get()

Copy link
Member

Choose a reason for hiding this comment

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

oh, indeed!

.build(INSTRUMENTATION_NAME, Servlet5Accessor.INSTANCE);

private static final JettyHelper<HttpServletRequest, HttpServletResponse> HELPER =
Expand Down
8 changes: 1 addition & 7 deletions ...src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8Singletons.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,7 @@

package io.opentelemetry.javaagent.instrumentation.jetty.v8_0;

import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTAINER;

import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
import io.opentelemetry.javaagent.bootstrap.servlet.AppServerBridge;
import io.opentelemetry.javaagent.instrumentation.jetty.common.JettyHelper;
import io.opentelemetry.javaagent.instrumentation.servlet.ServletInstrumenterBuilder;
Expand All @@ -26,10 +23,7 @@ public final class Jetty8Singletons {
INSTRUMENTER =
ServletInstrumenterBuilder.<HttpServletRequest, HttpServletResponse>create()
.addContextCustomizer(
(context, request, attributes) -> {
context = ServerSpanNaming.init(context, CONTAINER);
return new AppServerBridge.Builder().init(context);
})
(context, request, attributes) -> new AppServerBridge.Builder().init(context))
.build(INSTRUMENTATION_NAME, Servlet3Accessor.INSTANCE);

private static final JettyHelper<HttpServletRequest, HttpServletResponse> HELPER =
Expand Down
5 changes: 1 addition & 4 deletions ...entelemetry/javaagent/instrumentation/liberty/dispatcher/LibertyDispatcherSingletons.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@

package io.opentelemetry.javaagent.instrumentation.liberty.dispatcher;

import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTAINER;

import io.opentelemetry.api.GlobalOpenTelemetry;
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor;
Expand Down Expand Up @@ -39,8 +37,7 @@ public final class LibertyDispatcherSingletons {
.setSpanStatusExtractor(spanStatusExtractor)
.addAttributesExtractor(httpAttributesExtractor)
.addAttributesExtractor(netAttributesExtractor)
.addContextCustomizer(
(context, request, attributes) -> ServerSpanNaming.init(context, CONTAINER))
.addContextCustomizer(ServerSpanNaming.get())
.addRequestMetrics(HttpServerMetrics.get())
.newServerInstrumenter(LibertyDispatcherRequestGetter.INSTANCE);
}
Expand Down
9 changes: 2 additions & 7 deletions ...t/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertySingletons.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,7 @@

package io.opentelemetry.javaagent.instrumentation.liberty;

import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTAINER;

import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
import io.opentelemetry.javaagent.bootstrap.servlet.AppServerBridge;
import io.opentelemetry.javaagent.instrumentation.servlet.ServletInstrumenterBuilder;
import io.opentelemetry.javaagent.instrumentation.servlet.ServletRequestContext;
Expand All @@ -25,10 +22,8 @@ public final class LibertySingletons {
INSTRUMENTER =
ServletInstrumenterBuilder.<HttpServletRequest, HttpServletResponse>create()
.addContextCustomizer(
(context, request, attributes) -> {
context = ServerSpanNaming.init(context, CONTAINER);
return new AppServerBridge.Builder().recordException().init(context);
})
(context, request, attributes) ->
new AppServerBridge.Builder().recordException().init(context))
.build(INSTRUMENTATION_NAME, Servlet3Accessor.INSTANCE);

private static final LibertyHelper<HttpServletRequest, HttpServletResponse> HELPER =
Expand Down
8 changes: 2 additions & 6 deletions ...lemetry/javaagent/instrumentation/netty/common/server/NettyServerInstrumenterFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,8 @@ public static Instrumenter<HttpRequestAndChannel, HttpResponse> create(
.addAttributesExtractor(httpAttributesExtractor)
.addAttributesExtractor(new NettyNetServerAttributesExtractor())
.addRequestMetrics(HttpServerMetrics.get())
.addContextCustomizer(
(context, request, attributes) -> {
context = NettyErrorHolder.init(context);
// netty is not exactly a "container", but it's the best match out of these
return ServerSpanNaming.init(context, ServerSpanNaming.Source.CONTAINER);
})
.addContextCustomizer((context, request, attributes) -> NettyErrorHolder.init(context))
.addContextCustomizer(ServerSpanNaming.get())
.newServerInstrumenter(HttpRequestHeadersGetter.INSTANCE);
}

Expand Down
5 changes: 0 additions & 5 deletions ...main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2Singletons.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,9 @@

package io.opentelemetry.javaagent.instrumentation.servlet.v2_2;

import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.SERVLET;

import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpServerAttributesExtractor;
import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
import io.opentelemetry.javaagent.instrumentation.servlet.ServletInstrumenterBuilder;
import io.opentelemetry.javaagent.instrumentation.servlet.ServletRequestContext;
import io.opentelemetry.javaagent.instrumentation.servlet.ServletResponseContext;
Expand All @@ -33,8 +30,6 @@ public final class Servlet2Singletons {
ServletRequestContext<HttpServletRequest>, ServletResponseContext<HttpServletResponse>>
instrumenter =
ServletInstrumenterBuilder.<HttpServletRequest, HttpServletResponse>create()
.addContextCustomizer(
(context, request, attributes) -> ServerSpanNaming.init(context, SERVLET))
.build(
INSTRUMENTATION_NAME,
Servlet2Accessor.INSTANCE,
Expand Down
62 changes: 25 additions & 37 deletions ...src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3Advice.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.instrumentation.api.tracer.ServerSpan;
import io.opentelemetry.javaagent.bootstrap.servlet.AppServerBridge;
import io.opentelemetry.javaagent.bootstrap.servlet.MappingResolver;
import io.opentelemetry.javaagent.instrumentation.api.CallDepth;
Expand Down Expand Up @@ -39,52 +38,41 @@ public static void onEnter(
if (!(request instanceof HttpServletRequest) || !(response instanceof HttpServletResponse)) {
return;
}
HttpServletRequest httpServletRequest = (HttpServletRequest) request;

callDepth = CallDepth.forClass(AppServerBridge.getCallDepthKey());
callDepth.getAndIncrement();

HttpServletRequest httpServletRequest = (HttpServletRequest) request;

Context currentContext = Java8BytecodeBridge.currentContext();
Context attachedContext = helper().getServerContext(httpServletRequest);
if (attachedContext != null && helper().needsRescoping(currentContext, attachedContext)) {
MappingResolver mappingResolver = Servlet3Singletons.getMappingResolver(servletOrFilter);
boolean servlet = servletOrFilter instanceof Servlet;
attachedContext =
helper().updateContext(attachedContext, httpServletRequest, mappingResolver, servlet);
scope = attachedContext.makeCurrent();
// We are inside nested servlet/filter/app-server span, don't create new span
return;
}

if (attachedContext != null || ServerSpan.fromContextOrNull(currentContext) != null) {
// Update context with info from current request to ensure that server span gets the best
// possible name.
// In case server span was created by app server instrumentations calling updateContext
// returns a new context that contains servlet context path that is used in other
// instrumentations for naming server span.
MappingResolver mappingResolver = Servlet3Singletons.getMappingResolver(servletOrFilter);
boolean servlet = servletOrFilter instanceof Servlet;
Context updatedContext =
helper().updateContext(currentContext, httpServletRequest, mappingResolver, servlet);
if (currentContext != updatedContext) {
// updateContext updated context, need to re-scope
scope = updatedContext.makeCurrent();
}
// We are inside nested servlet/filter/app-server span, don't create new span
return;
}
Context contextToUpdate;

requestContext = new ServletRequestContext<>(httpServletRequest, servletOrFilter);

if (!helper().shouldStart(currentContext, requestContext)) {
return;
if (attachedContext == null && helper().shouldStart(currentContext, requestContext)) {
context = helper().start(currentContext, requestContext);
helper().setAsyncListenerResponse(httpServletRequest, (HttpServletResponse) response);

contextToUpdate = context;
} else if (attachedContext != null
&& helper().needsRescoping(currentContext, attachedContext)) {
// Given request already has a context associated with it.
// see the needsRescoping() javadoc for more explanation
contextToUpdate = attachedContext;
} else {
// We are inside nested servlet/filter/app-server span, don't create new span
contextToUpdate = currentContext;
}

context = helper().start(currentContext, requestContext);
scope = context.makeCurrent();

helper().setAsyncListenerResponse(httpServletRequest, (HttpServletResponse) response);
// Update context with info from current request to ensure that server span gets the best
// possible name.
// In case server span was created by app server instrumentations calling updateContext
// returns a new context that contains servlet context path that is used in other
// instrumentations for naming server span.
MappingResolver mappingResolver = Servlet3Singletons.getMappingResolver(servletOrFilter);
boolean servlet = servletOrFilter instanceof Servlet;
contextToUpdate =
helper().updateContext(contextToUpdate, httpServletRequest, mappingResolver, servlet);
Comment on lines +73 to +74
Copy link
Member

Choose a reason for hiding this comment

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

the previous behavior was nice of setting everything up correctly in startSpan (e.g. correct span name and correct ServerSpanNaming.Source)

though I guess it doesn't matter in practice most of the time since we instrument so many servlet containers that we probably don't startSpan that often here

Copy link
Member Author

Choose a reason for hiding this comment

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

the previous behavior was nice of setting everything up correctly in startSpan (e.g. correct span name and correct ServerSpanNaming.Source)

Honestly I found setting the Source a bit confusing: what if your span name was HTTP GET with FILTER as a source? If you had another filter trying to update the name to /abc it wouldn't, because it was a shorter name - which I'm not so sure is correct; setting the source seems to be closely connected to the route, not necessarily the span name (even if they're the same in most cases). (Although I'm not sure how "real" this example is 😅 )

Also, the only 2 instrumentations I found that have access to the route in the beginning are armeria and servlets; and servlet span name will usually be updated by something like spring or jax-rs. Which implies that for most cases, the route should be extracted on end() (and span name updated somewhere in the meantime).

scope = contextToUpdate.makeCurrent();
}

@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
Expand Down
14 changes: 0 additions & 14 deletions ...main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3Singletons.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,8 @@

package io.opentelemetry.javaagent.instrumentation.servlet.v3_0;

import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.FILTER;
import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.SERVLET;

import io.opentelemetry.instrumentation.api.field.VirtualField;
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
import io.opentelemetry.javaagent.bootstrap.servlet.MappingResolver;
import io.opentelemetry.javaagent.instrumentation.servlet.ServletHelper;
import io.opentelemetry.javaagent.instrumentation.servlet.ServletInstrumenterBuilder;
Expand All @@ -28,11 +24,6 @@ public final class Servlet3Singletons {
ServletRequestContext<HttpServletRequest>, ServletResponseContext<HttpServletResponse>>
INSTRUMENTER =
ServletInstrumenterBuilder.<HttpServletRequest, HttpServletResponse>create()
.setMappingResolverFunction(Servlet3Singletons::getMappingResolver)
.addContextCustomizer(
(context, request, attributes) ->
ServerSpanNaming.init(
context, request.servletOrFilter() instanceof Servlet ? SERVLET : FILTER))
.build(INSTRUMENTATION_NAME, Servlet3Accessor.INSTANCE);

private static final ServletHelper<HttpServletRequest, HttpServletResponse> HELPER =
Expand All @@ -47,11 +38,6 @@ public static ServletHelper<HttpServletRequest, HttpServletResponse> helper() {
return HELPER;
}

private static MappingResolver getMappingResolver(
ServletRequestContext<?> servletRequestContext) {
return getMappingResolver(servletRequestContext.servletOrFilter());
}

public static MappingResolver getMappingResolver(Object servletOrFilter) {
MappingResolver.Factory factory = getMappingResolverFactory(servletOrFilter);
if (factory != null) {
Expand Down
Loading