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

Move attributes to span builder for use by samplers #2587

Merged
merged 3 commits into from
Mar 17, 2021
Merged

Move attributes to span builder for use by samplers #2587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5848260
Move attributes to span builder for sampler use
trask Mar 17, 2021
cb539df
Fix test
trask Mar 17, 2021
ef70e16
Remove unnecessary test
trask Mar 17, 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
36 changes: 24 additions & 12 deletions ...ation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,24 +135,36 @@ private Span internalStartSpan(
if (startTimeNanos > 0) {
spanBuilder.setStartTimestamp(startTimeNanos, TimeUnit.NANOSECONDS);
}
onRequest(spanBuilder, request);
Span span = spanBuilder.startSpan();
onRequest(span, request);
return span;
}

protected void onRequest(SpanBuilder spanBuilder, REQUEST request) {
onRequest(spanBuilder::setAttribute, request);
}

/**
* This method should only be used when the request is not yet available when {@link #startSpan}
* is called. Otherwise {@link #onRequest(SpanBuilder, Object)} should be used.
*/
protected void onRequest(Span span, REQUEST request) {
assert span != null;
onRequest(span::setAttribute, request);
}

private void onRequest(AttributeSetter setter, REQUEST request) {
assert setter != null;
if (request != null) {
span.setAttribute(SemanticAttributes.NET_TRANSPORT, "IP.TCP");
span.setAttribute(SemanticAttributes.HTTP_METHOD, method(request));
span.setAttribute(SemanticAttributes.HTTP_USER_AGENT, requestHeader(request, USER_AGENT));
setter.setAttribute(SemanticAttributes.NET_TRANSPORT, "IP.TCP");
setter.setAttribute(SemanticAttributes.HTTP_METHOD, method(request));
setter.setAttribute(SemanticAttributes.HTTP_USER_AGENT, requestHeader(request, USER_AGENT));

setFlavor(span, request);
setUrl(span, request);
setFlavor(setter, request);
setUrl(setter, request);
}
}

private void setFlavor(Span span, REQUEST request) {
private void setFlavor(AttributeSetter setter, REQUEST request) {
String flavor = flavor(request);
if (flavor == null) {
return;
Expand All @@ -163,15 +175,15 @@ private void setFlavor(Span span, REQUEST request) {
flavor = flavor.substring(httpProtocolPrefix.length());
}

span.setAttribute(SemanticAttributes.HTTP_FLAVOR, flavor);
setter.setAttribute(SemanticAttributes.HTTP_FLAVOR, flavor);
}

private void setUrl(Span span, REQUEST request) {
private void setUrl(AttributeSetter setter, REQUEST request) {
try {
URI url = url(request);
if (url != null) {
netPeerAttributes.setNetPeer(span, url.getHost(), null, url.getPort());
span.setAttribute(SemanticAttributes.HTTP_URL, url.toString());
netPeerAttributes.setNetPeer(setter, url.getHost(), null, url.getPort());
setter.setAttribute(SemanticAttributes.HTTP_URL, url.toString());
}
} catch (Exception e) {
log.debug("Error tagging url", e);
Expand Down
40 changes: 21 additions & 19 deletions ...ation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,18 +70,17 @@ public Context startSpan(
// whether to call end() or not on the Span in the returned Context

Context parentContext = extract(request, getGetter());
SpanBuilder builder = spanBuilder(parentContext, spanName, SERVER);
SpanBuilder spanBuilder = spanBuilder(parentContext, spanName, SERVER);

if (startTimestamp >= 0) {
builder.setStartTimestamp(startTimestamp, TimeUnit.NANOSECONDS);
spanBuilder.setStartTimestamp(startTimestamp, TimeUnit.NANOSECONDS);
}

Span span = builder.startSpan();
onConnection(span, connection);
onRequest(span, request);
onConnectionAndRequest(span, connection, request);
onConnection(spanBuilder, connection);
onRequest(spanBuilder, request);
onConnectionAndRequest(spanBuilder, connection, request);

Context context = withServerSpan(parentContext, span);
Context context = withServerSpan(parentContext, spanBuilder.startSpan());
context = customizeContext(context, request);
attachServerContext(context, storage);

Expand Down Expand Up @@ -113,6 +112,7 @@ public void end(Context context, RESPONSE response, long timestamp) {
* Convenience method. Delegates to {@link #endExceptionally(Context, Throwable, Object)}, passing
* {@code response} value of {@code null}.
*/
@Override
public void endExceptionally(Context context, Throwable throwable) {
endExceptionally(context, throwable, null);
}
Expand Down Expand Up @@ -153,21 +153,22 @@ public Span getServerSpan(STORAGE storage) {
@Nullable
public abstract Context getServerContext(STORAGE storage);

protected void onConnection(Span span, CONNECTION connection) {
protected void onConnection(SpanBuilder spanBuilder, CONNECTION connection) {
// TODO: use NetPeerAttributes here
span.setAttribute(SemanticAttributes.NET_PEER_IP, peerHostIP(connection));
spanBuilder.setAttribute(SemanticAttributes.NET_PEER_IP, peerHostIP(connection));
Integer port = peerPort(connection);
// Negative or Zero ports might represent an unset/null value for an int type. Skip setting.
if (port != null && port > 0) {
span.setAttribute(SemanticAttributes.NET_PEER_PORT, (long) port);
spanBuilder.setAttribute(SemanticAttributes.NET_PEER_PORT, (long) port);
}
}

protected void onRequest(Span span, REQUEST request) {
span.setAttribute(SemanticAttributes.HTTP_METHOD, method(request));
span.setAttribute(SemanticAttributes.HTTP_USER_AGENT, requestHeader(request, USER_AGENT));
protected void onRequest(SpanBuilder spanBuilder, REQUEST request) {
spanBuilder.setAttribute(SemanticAttributes.HTTP_METHOD, method(request));
spanBuilder.setAttribute(
SemanticAttributes.HTTP_USER_AGENT, requestHeader(request, USER_AGENT));

setUrl(span, request);
setUrl(spanBuilder, request);

// TODO set resource name from URL.
}
Expand All @@ -183,20 +184,21 @@ protected void onRequest(Span span, REQUEST request) {
which is the recommended value for http.target attribute. Therefore we cannot use any of the
recommended combinations of attributes and are forced to use http.url.
*/
private void setUrl(Span span, REQUEST request) {
span.setAttribute(SemanticAttributes.HTTP_URL, url(request));
private void setUrl(SpanBuilder spanBuilder, REQUEST request) {
spanBuilder.setAttribute(SemanticAttributes.HTTP_URL, url(request));
}

protected void onConnectionAndRequest(Span span, CONNECTION connection, REQUEST request) {
protected void onConnectionAndRequest(
SpanBuilder spanBuilder, CONNECTION connection, REQUEST request) {
String flavor = flavor(connection, request);
if (flavor != null) {
// remove HTTP/ prefix to comply with semantic conventions
if (flavor.startsWith("HTTP/")) {
flavor = flavor.substring("HTTP/".length());
}
span.setAttribute(SemanticAttributes.HTTP_FLAVOR, flavor);
spanBuilder.setAttribute(SemanticAttributes.HTTP_FLAVOR, flavor);
}
span.setAttribute(SemanticAttributes.HTTP_CLIENT_IP, clientIP(connection, request));
spanBuilder.setAttribute(SemanticAttributes.HTTP_CLIENT_IP, clientIP(connection, request));
}

private String clientIP(CONNECTION connection, REQUEST request) {
Expand Down
19 changes: 1 addition & 18 deletions ...i/src/test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpClientTracerTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

package io.opentelemetry.instrumentation.api.tracer

import io.opentelemetry.api.trace.Span

import io.opentelemetry.context.propagation.TextMapSetter
import io.opentelemetry.instrumentation.api.tracer.net.NetPeerAttributes
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes
Expand Down Expand Up @@ -132,23 +132,6 @@ class HttpClientTracerTest extends BaseTracerTest {
null | null
}

def "test assert null span"() {
setup:
def tracer = newTracer()

when:
tracer.onRequest((Span) null, null)

then:
thrown(AssertionError)

when:
tracer.onResponse((Span) null, null)

then:
thrown(AssertionError)
}

@Override
def newTracer() {
def netPeerAttributes = new NetPeerAttributes([
Expand Down
19 changes: 9 additions & 10 deletions ...t-2.2/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package io.opentelemetry.instrumentation.servlet;

import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.SpanContext;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.TextMapGetter;
import io.opentelemetry.instrumentation.api.servlet.AppServerBridge;
Expand All @@ -27,7 +28,14 @@ public abstract class ServletHttpServerTracer<RESPONSE>
private static final Logger log = LoggerFactory.getLogger(ServletHttpServerTracer.class);

public Context startSpan(HttpServletRequest request, String spanName) {
return startSpan(request, request, request, spanName);
Context context = startSpan(request, request, request, spanName);

SpanContext spanContext = Span.fromContext(context).getSpanContext();
// we do this e.g. so that servlet containers can use these values in their access logs
request.setAttribute("traceId", spanContext.getTraceId());
request.setAttribute("spanId", spanContext.getSpanId());
Comment on lines +35 to +36
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll send a follow-up PR to change these attribute names to trace_id and span_id to match logging MDC attribute names.


return context;
}

@Override
Expand Down Expand Up @@ -108,15 +116,6 @@ protected String method(HttpServletRequest request) {
return request.getMethod();
}

@Override
public void onRequest(Span span, HttpServletRequest request) {
// we do this e.g. so that servlet containers can use these values in their access logs
request.setAttribute("traceId", span.getSpanContext().getTraceId());
request.setAttribute("spanId", span.getSpanContext().getSpanId());

super.onRequest(span, request);
}

@Override
protected TextMapGetter<HttpServletRequest> getGetter() {
return HttpServletRequestGetter.GETTER;
Expand Down
8 changes: 5 additions & 3 deletions ...a/io/opentelemetry/javaagent/instrumentation/kubernetesclient/KubernetesClientTracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import io.kubernetes.client.openapi.ApiException;
import io.kubernetes.client.openapi.ApiResponse;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.SpanBuilder;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.TextMapSetter;
import io.opentelemetry.instrumentation.api.config.Config;
Expand Down Expand Up @@ -89,11 +90,12 @@ public void onException(Context context, Throwable throwable) {
}

@Override
protected void onRequest(Span span, Request request) {
super.onRequest(span, request);
protected void onRequest(SpanBuilder spanBuilder, Request request) {
super.onRequest(spanBuilder, request);
if (CAPTURE_EXPERIMENTAL_SPAN_ATTRIBUTES) {
KubernetesRequestDigest digest = KubernetesRequestDigest.parse(request);
span.setAttribute("kubernetes-client.namespace", digest.getResourceMeta().getNamespace())
spanBuilder
.setAttribute("kubernetes-client.namespace", digest.getResourceMeta().getNamespace())
.setAttribute("kubernetes-client.name", digest.getResourceMeta().getName());
}
}
Expand Down
10 changes: 5 additions & 5 deletions ...a/io/opentelemetry/javaagent/instrumentation/netty/v3_8/client/NettyHttpClientTracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import static io.opentelemetry.javaagent.instrumentation.netty.v3_8.client.NettyResponseInjectAdapter.SETTER;
import static org.jboss.netty.handler.codec.http.HttpHeaders.Names.HOST;

import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.SpanBuilder;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.TextMapSetter;
import io.opentelemetry.instrumentation.api.tracer.HttpClientTracer;
Expand All @@ -36,12 +36,12 @@ public static NettyHttpClientTracer tracer() {
}

public Context startSpan(Context parentContext, ChannelHandlerContext ctx, HttpRequest request) {
Span span = spanBuilder(parentContext, spanNameForRequest(request), CLIENT).startSpan();
onRequest(span, request);
SpanBuilder spanBuilder = spanBuilder(parentContext, spanNameForRequest(request), CLIENT);
onRequest(spanBuilder, request);
NetPeerAttributes.INSTANCE.setNetPeer(
span, (InetSocketAddress) ctx.getChannel().getRemoteAddress());
spanBuilder, (InetSocketAddress) ctx.getChannel().getRemoteAddress());

Context context = withClientSpan(parentContext, span);
Context context = withClientSpan(parentContext, spanBuilder.startSpan());
inject(context, request.headers(), SETTER);
return context;
}
Expand Down
11 changes: 6 additions & 5 deletions ...a/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/NettyHttpClientTracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import io.netty.handler.codec.http.HttpHeaders;
import io.netty.handler.codec.http.HttpRequest;
import io.netty.handler.codec.http.HttpResponse;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.SpanBuilder;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.TextMapSetter;
import io.opentelemetry.instrumentation.api.tracer.HttpClientTracer;
Expand All @@ -36,11 +36,12 @@ public static NettyHttpClientTracer tracer() {
}

public Context startSpan(Context parentContext, ChannelHandlerContext ctx, HttpRequest request) {
Span span = spanBuilder(parentContext, spanNameForRequest(request), CLIENT).startSpan();
onRequest(span, request);
NetPeerAttributes.INSTANCE.setNetPeer(span, (InetSocketAddress) ctx.channel().remoteAddress());
SpanBuilder spanBuilder = spanBuilder(parentContext, spanNameForRequest(request), CLIENT);
onRequest(spanBuilder, request);
NetPeerAttributes.INSTANCE.setNetPeer(
spanBuilder, (InetSocketAddress) ctx.channel().remoteAddress());

Context context = withClientSpan(parentContext, span);
Context context = withClientSpan(parentContext, spanBuilder.startSpan());
inject(context, request.headers(), SETTER);
return context;
}
Expand Down
11 changes: 6 additions & 5 deletions ...a/io/opentelemetry/javaagent/instrumentation/netty/v4_1/client/NettyHttpClientTracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import io.netty.handler.codec.http.HttpHeaders;
import io.netty.handler.codec.http.HttpRequest;
import io.netty.handler.codec.http.HttpResponse;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.SpanBuilder;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.TextMapSetter;
import io.opentelemetry.instrumentation.api.tracer.HttpClientTracer;
Expand All @@ -36,11 +36,12 @@ public static NettyHttpClientTracer tracer() {
}

public Context startSpan(Context parentContext, ChannelHandlerContext ctx, HttpRequest request) {
Span span = spanBuilder(parentContext, spanNameForRequest(request), CLIENT).startSpan();
onRequest(span, request);
NetPeerAttributes.INSTANCE.setNetPeer(span, (InetSocketAddress) ctx.channel().remoteAddress());
SpanBuilder spanBuilder = spanBuilder(parentContext, spanNameForRequest(request), CLIENT);
onRequest(spanBuilder, request);
NetPeerAttributes.INSTANCE.setNetPeer(
spanBuilder, (InetSocketAddress) ctx.channel().remoteAddress());

Context context = withClientSpan(parentContext, span);
Context context = withClientSpan(parentContext, spanBuilder.startSpan());
inject(context, request.headers(), SETTER);
return context;
}
Expand Down