code review comments

open-telemetry · Apr 5, 2022 · 710f5e8 · 710f5e8
1 parent 6dbdf62
commit 710f5e8
Show file tree

Hide file tree

Showing 14 changed files with 71 additions and 69 deletions.
diff --git a/docs/logger-mdc-instrumentation.md b/docs/logger-mdc-instrumentation.md
@@ -19,7 +19,7 @@ event's MDC copy:
   (same as `Span.current().getSpanContext().getTraceFlags().asHex()`).
 
 Those three pieces of information can be included in log statements produced by the logging library
-by specifying them in the pattern/format. 
+by specifying them in the pattern/format.
 
 Tip: for Spring Boot configuration which uses logback, you can add MDC to log lines by overriding only the `logging.pattern.level`:
 ```properties

diff --git a/.../java/io/opentelemetry/instrumentation/api/instrumenter/code/CodeAttributesExtractor.java b/.../java/io/opentelemetry/instrumentation/api/instrumenter/code/CodeAttributesExtractor.java
@@ -5,7 +5,7 @@
 
 package io.opentelemetry.instrumentation.api.instrumenter.code;
 
-import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.setAttr;
+import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.internalSet;
 
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
@@ -37,9 +37,9 @@ private CodeAttributesExtractor(CodeAttributesGetter<REQUEST> getter) {
   public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
     Class<?> cls = getter.codeClass(request);
     if (cls != null) {
-      setAttr(attributes, SemanticAttributes.CODE_NAMESPACE, cls.getName());
+      internalSet(attributes, SemanticAttributes.CODE_NAMESPACE, cls.getName());
     }
-    setAttr(attributes, SemanticAttributes.CODE_FUNCTION, getter.methodName(request));
+    internalSet(attributes, SemanticAttributes.CODE_FUNCTION, getter.methodName(request));
   }
 
   @Override

diff --git a/...ava/io/opentelemetry/instrumentation/api/instrumenter/db/DbClientAttributesExtractor.java b/...ava/io/opentelemetry/instrumentation/api/instrumenter/db/DbClientAttributesExtractor.java
@@ -5,7 +5,7 @@
 
 package io.opentelemetry.instrumentation.api.instrumenter.db;
 
-import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.setAttr;
+import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.internalSet;
 
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
@@ -37,7 +37,7 @@ public static <REQUEST, RESPONSE> DbClientAttributesExtractor<REQUEST, RESPONSE>
   public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
     super.onStart(attributes, parentContext, request);
 
-    setAttr(attributes, SemanticAttributes.DB_STATEMENT, getter.statement(request));
-    setAttr(attributes, SemanticAttributes.DB_OPERATION, getter.operation(request));
+    internalSet(attributes, SemanticAttributes.DB_STATEMENT, getter.statement(request));
+    internalSet(attributes, SemanticAttributes.DB_OPERATION, getter.operation(request));
   }
 }
diff --git a/.../opentelemetry/instrumentation/api/instrumenter/db/DbClientCommonAttributesExtractor.java b/.../opentelemetry/instrumentation/api/instrumenter/db/DbClientCommonAttributesExtractor.java
@@ -5,7 +5,7 @@
 
 package io.opentelemetry.instrumentation.api.instrumenter.db;
 
-import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.setAttr;
+import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.internalSet;
 
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
@@ -28,10 +28,11 @@ abstract class DbClientCommonAttributesExtractor<
 
   @Override
   public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
-    setAttr(attributes, SemanticAttributes.DB_SYSTEM, getter.system(request));
-    setAttr(attributes, SemanticAttributes.DB_USER, getter.user(request));
-    setAttr(attributes, SemanticAttributes.DB_NAME, getter.name(request));
-    setAttr(attributes, SemanticAttributes.DB_CONNECTION_STRING, getter.connectionString(request));
+    internalSet(attributes, SemanticAttributes.DB_SYSTEM, getter.system(request));
+    internalSet(attributes, SemanticAttributes.DB_USER, getter.user(request));
+    internalSet(attributes, SemanticAttributes.DB_NAME, getter.name(request));
+    internalSet(
+        attributes, SemanticAttributes.DB_CONNECTION_STRING, getter.connectionString(request));
   }
 
   @Override

diff --git a/...va/io/opentelemetry/instrumentation/api/instrumenter/db/SqlClientAttributesExtractor.java b/...va/io/opentelemetry/instrumentation/api/instrumenter/db/SqlClientAttributesExtractor.java
@@ -5,7 +5,7 @@
 
 package io.opentelemetry.instrumentation.api.instrumenter.db;
 
-import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.setAttr;
+import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.internalSet;
 
 import io.opentelemetry.api.common.AttributeKey;
 import io.opentelemetry.api.common.AttributesBuilder;
@@ -57,8 +57,8 @@ public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST
 
     SqlStatementInfo sanitizedStatement =
         SqlStatementSanitizer.sanitize(getter.rawStatement(request));
-    setAttr(attributes, SemanticAttributes.DB_STATEMENT, sanitizedStatement.getFullStatement());
-    setAttr(attributes, SemanticAttributes.DB_OPERATION, sanitizedStatement.getOperation());
-    setAttr(attributes, dbTableAttribute, sanitizedStatement.getTable());
+    internalSet(attributes, SemanticAttributes.DB_STATEMENT, sanitizedStatement.getFullStatement());
+    internalSet(attributes, SemanticAttributes.DB_OPERATION, sanitizedStatement.getOperation());
+    internalSet(attributes, dbTableAttribute, sanitizedStatement.getTable());
   }
 }
diff --git a/...io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractor.java b/...io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractor.java
@@ -5,7 +5,7 @@
 
 package io.opentelemetry.instrumentation.api.instrumenter.http;
 
-import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.setAttr;
+import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.internalSet;
 
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
@@ -55,7 +55,7 @@ public static <REQUEST, RESPONSE> HttpClientAttributesExtractorBuilder<REQUEST,
   @Override
   public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
     super.onStart(attributes, parentContext, request);
-    setAttr(attributes, SemanticAttributes.HTTP_URL, getter.url(request));
+    internalSet(attributes, SemanticAttributes.HTTP_URL, getter.url(request));
   }
 
   @Override
@@ -66,7 +66,7 @@ public void onEnd(
       @Nullable RESPONSE response,
       @Nullable Throwable error) {
     super.onEnd(attributes, context, request, response, error);
-    setAttr(attributes, SemanticAttributes.HTTP_FLAVOR, getter.flavor(request, response));
+    internalSet(attributes, SemanticAttributes.HTTP_FLAVOR, getter.flavor(request, response));
   }
 
   /**

diff --git a/...io/opentelemetry/instrumentation/api/instrumenter/http/HttpCommonAttributesExtractor.java b/...io/opentelemetry/instrumentation/api/instrumenter/http/HttpCommonAttributesExtractor.java
@@ -8,7 +8,7 @@
 import static io.opentelemetry.instrumentation.api.instrumenter.http.CapturedHttpHeadersUtil.lowercase;
 import static io.opentelemetry.instrumentation.api.instrumenter.http.CapturedHttpHeadersUtil.requestAttributeKey;
 import static io.opentelemetry.instrumentation.api.instrumenter.http.CapturedHttpHeadersUtil.responseAttributeKey;
-import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.setAttr;
+import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.internalSet;
 
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
@@ -39,13 +39,13 @@ abstract class HttpCommonAttributesExtractor<
 
   @Override
   public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
-    setAttr(attributes, SemanticAttributes.HTTP_METHOD, getter.method(request));
-    setAttr(attributes, SemanticAttributes.HTTP_USER_AGENT, userAgent(request));
+    internalSet(attributes, SemanticAttributes.HTTP_METHOD, getter.method(request));
+    internalSet(attributes, SemanticAttributes.HTTP_USER_AGENT, userAgent(request));
 
     for (String name : capturedRequestHeaders) {
       List<String> values = getter.requestHeader(request, name);
       if (!values.isEmpty()) {
-        setAttr(attributes, requestAttributeKey(name), values);
+        internalSet(attributes, requestAttributeKey(name), values);
       }
     }
   }
@@ -58,33 +58,33 @@ public void onEnd(
       @Nullable RESPONSE response,
       @Nullable Throwable error) {
 
-    setAttr(
+    internalSet(
         attributes,
         SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH,
         getter.requestContentLength(request, response));
-    setAttr(
+    internalSet(
         attributes,
         SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH_UNCOMPRESSED,
         getter.requestContentLengthUncompressed(request, response));
 
     if (response != null) {
       Integer statusCode = getter.statusCode(request, response);
       if (statusCode != null && statusCode > 0) {
-        setAttr(attributes, SemanticAttributes.HTTP_STATUS_CODE, (long) statusCode);
+        internalSet(attributes, SemanticAttributes.HTTP_STATUS_CODE, (long) statusCode);
       }
-      setAttr(
+      internalSet(
           attributes,
           SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH,
           getter.responseContentLength(request, response));
-      setAttr(
+      internalSet(
           attributes,
           SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH_UNCOMPRESSED,
           getter.responseContentLengthUncompressed(request, response));
 
       for (String name : capturedResponseHeaders) {
         List<String> values = getter.responseHeader(request, response, name);
         if (!values.isEmpty()) {
-          setAttr(attributes, responseAttributeKey(name), values);
+          internalSet(attributes, responseAttributeKey(name), values);
         }
       }
     }

diff --git a/...io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractor.java b/...io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractor.java
@@ -9,7 +9,7 @@
 import static io.opentelemetry.instrumentation.api.instrumenter.http.ForwardedHeaderParser.extractClientIpFromForwardedHeader;
 import static io.opentelemetry.instrumentation.api.instrumenter.http.ForwardedHeaderParser.extractProtoFromForwardedHeader;
 import static io.opentelemetry.instrumentation.api.instrumenter.http.ForwardedHeaderParser.extractProtoFromForwardedProtoHeader;
-import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.setAttr;
+import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.internalSet;
 
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
@@ -69,15 +69,15 @@ public static <REQUEST, RESPONSE> HttpServerAttributesExtractorBuilder<REQUEST,
   public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
     super.onStart(attributes, parentContext, request);
 
-    setAttr(attributes, SemanticAttributes.HTTP_FLAVOR, getter.flavor(request));
+    internalSet(attributes, SemanticAttributes.HTTP_FLAVOR, getter.flavor(request));
     String forwardedProto = forwardedProto(request);
     String value = forwardedProto != null ? forwardedProto : getter.scheme(request);
-    setAttr(attributes, SemanticAttributes.HTTP_SCHEME, value);
-    setAttr(attributes, SemanticAttributes.HTTP_HOST, host(request));
-    setAttr(attributes, SemanticAttributes.HTTP_TARGET, getter.target(request));
-    setAttr(attributes, SemanticAttributes.HTTP_ROUTE, getter.route(request));
-    setAttr(attributes, SemanticAttributes.HTTP_SERVER_NAME, getter.serverName(request));
-    setAttr(attributes, SemanticAttributes.HTTP_CLIENT_IP, clientIp(request));
+    internalSet(attributes, SemanticAttributes.HTTP_SCHEME, value);
+    internalSet(attributes, SemanticAttributes.HTTP_HOST, host(request));
+    internalSet(attributes, SemanticAttributes.HTTP_TARGET, getter.target(request));
+    internalSet(attributes, SemanticAttributes.HTTP_ROUTE, getter.route(request));
+    internalSet(attributes, SemanticAttributes.HTTP_SERVER_NAME, getter.serverName(request));
+    internalSet(attributes, SemanticAttributes.HTTP_CLIENT_IP, clientIp(request));
   }
 
   @Override
@@ -89,7 +89,7 @@ public void onEnd(
       @Nullable Throwable error) {
 
     super.onEnd(attributes, context, request, response, error);
-    setAttr(attributes, SemanticAttributes.HTTP_ROUTE, httpRouteHolderGetter.apply(context));
+    internalSet(attributes, SemanticAttributes.HTTP_ROUTE, httpRouteHolderGetter.apply(context));
   }
 
   @Nullable

diff --git a/...pentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractor.java b/...pentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractor.java
@@ -7,7 +7,7 @@
 
 import static io.opentelemetry.instrumentation.api.instrumenter.messaging.MessageOperation.PROCESS;
 import static io.opentelemetry.instrumentation.api.instrumenter.messaging.MessageOperation.RECEIVE;
-import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.setAttr;
+import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.internalSet;
 
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
@@ -51,32 +51,33 @@ private MessagingAttributesExtractor(
   @SuppressWarnings("deprecation") // operationName
   @Override
   public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
-    setAttr(attributes, SemanticAttributes.MESSAGING_SYSTEM, getter.system(request));
-    setAttr(
+    internalSet(attributes, SemanticAttributes.MESSAGING_SYSTEM, getter.system(request));
+    internalSet(
         attributes, SemanticAttributes.MESSAGING_DESTINATION_KIND, getter.destinationKind(request));
     boolean isTemporaryDestination = getter.temporaryDestination(request);
     if (isTemporaryDestination) {
-      setAttr(attributes, SemanticAttributes.MESSAGING_TEMP_DESTINATION, true);
-      setAttr(attributes, SemanticAttributes.MESSAGING_DESTINATION, TEMP_DESTINATION_NAME);
+      internalSet(attributes, SemanticAttributes.MESSAGING_TEMP_DESTINATION, true);
+      internalSet(attributes, SemanticAttributes.MESSAGING_DESTINATION, TEMP_DESTINATION_NAME);
     } else {
-      setAttr(attributes, SemanticAttributes.MESSAGING_DESTINATION, getter.destination(request));
+      internalSet(
+          attributes, SemanticAttributes.MESSAGING_DESTINATION, getter.destination(request));
     }
-    setAttr(attributes, SemanticAttributes.MESSAGING_PROTOCOL, getter.protocol(request));
-    setAttr(
+    internalSet(attributes, SemanticAttributes.MESSAGING_PROTOCOL, getter.protocol(request));
+    internalSet(
         attributes, SemanticAttributes.MESSAGING_PROTOCOL_VERSION, getter.protocolVersion(request));
-    setAttr(attributes, SemanticAttributes.MESSAGING_URL, getter.url(request));
-    setAttr(
+    internalSet(attributes, SemanticAttributes.MESSAGING_URL, getter.url(request));
+    internalSet(
         attributes, SemanticAttributes.MESSAGING_CONVERSATION_ID, getter.conversationId(request));
-    setAttr(
+    internalSet(
         attributes,
         SemanticAttributes.MESSAGING_MESSAGE_PAYLOAD_SIZE_BYTES,
         getter.messagePayloadSize(request));
-    setAttr(
+    internalSet(
         attributes,
         SemanticAttributes.MESSAGING_MESSAGE_PAYLOAD_COMPRESSED_SIZE_BYTES,
         getter.messagePayloadCompressedSize(request));
     if (operation == RECEIVE || operation == PROCESS) {
-      setAttr(attributes, SemanticAttributes.MESSAGING_OPERATION, operation.operationName());
+      internalSet(attributes, SemanticAttributes.MESSAGING_OPERATION, operation.operationName());
     }
   }
 
@@ -87,7 +88,7 @@ public void onEnd(
       REQUEST request,
       @Nullable RESPONSE response,
       @Nullable Throwable error) {
-    setAttr(
+    internalSet(
         attributes, SemanticAttributes.MESSAGING_MESSAGE_ID, getter.messageId(request, response));
   }
 

diff --git a/...a/io/opentelemetry/instrumentation/api/instrumenter/net/NetClientAttributesExtractor.java b/...a/io/opentelemetry/instrumentation/api/instrumenter/net/NetClientAttributesExtractor.java
@@ -5,7 +5,7 @@
 
 package io.opentelemetry.instrumentation.api.instrumenter.net;
 
-import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.setAttr;
+import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.internalSet;
 
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
@@ -47,19 +47,19 @@ public void onEnd(
       @Nullable RESPONSE response,
       @Nullable Throwable error) {
 
-    setAttr(attributes, SemanticAttributes.NET_TRANSPORT, getter.transport(request, response));
+    internalSet(attributes, SemanticAttributes.NET_TRANSPORT, getter.transport(request, response));
 
     String peerIp = getter.peerIp(request, response);
     String peerName = getter.peerName(request, response);
 
     if (peerName != null && !peerName.equals(peerIp)) {
-      setAttr(attributes, SemanticAttributes.NET_PEER_NAME, peerName);
+      internalSet(attributes, SemanticAttributes.NET_PEER_NAME, peerName);
     }
-    setAttr(attributes, SemanticAttributes.NET_PEER_IP, peerIp);
+    internalSet(attributes, SemanticAttributes.NET_PEER_IP, peerIp);
 
     Integer peerPort = getter.peerPort(request, response);
     if (peerPort != null && peerPort > 0) {
-      setAttr(attributes, SemanticAttributes.NET_PEER_PORT, (long) peerPort);
+      internalSet(attributes, SemanticAttributes.NET_PEER_PORT, (long) peerPort);
     }
   }
 }
diff --git a/...a/io/opentelemetry/instrumentation/api/instrumenter/net/NetServerAttributesExtractor.java b/...a/io/opentelemetry/instrumentation/api/instrumenter/net/NetServerAttributesExtractor.java
@@ -5,7 +5,7 @@
 
 package io.opentelemetry.instrumentation.api.instrumenter.net;
 
-import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.setAttr;
+import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.internalSet;
 
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
@@ -35,15 +35,15 @@ private NetServerAttributesExtractor(NetServerAttributesGetter<REQUEST> getter)
 
   @Override
   public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
-    setAttr(attributes, SemanticAttributes.NET_TRANSPORT, getter.transport(request));
+    internalSet(attributes, SemanticAttributes.NET_TRANSPORT, getter.transport(request));
 
     String peerIp = getter.peerIp(request);
 
-    setAttr(attributes, SemanticAttributes.NET_PEER_IP, peerIp);
+    internalSet(attributes, SemanticAttributes.NET_PEER_IP, peerIp);
 
     Integer peerPort = getter.peerPort(request);
     if (peerPort != null && peerPort > 0) {
-      setAttr(attributes, SemanticAttributes.NET_PEER_PORT, (long) peerPort);
+      internalSet(attributes, SemanticAttributes.NET_PEER_PORT, (long) peerPort);
     }
   }
 

diff --git a/...a/io/opentelemetry/instrumentation/api/instrumenter/rpc/RpcCommonAttributesExtractor.java b/...a/io/opentelemetry/instrumentation/api/instrumenter/rpc/RpcCommonAttributesExtractor.java
@@ -5,7 +5,7 @@
 
 package io.opentelemetry.instrumentation.api.instrumenter.rpc;
 
-import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.setAttr;
+import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.internalSet;
 
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
@@ -24,9 +24,9 @@ abstract class RpcCommonAttributesExtractor<REQUEST, RESPONSE>
 
   @Override
   public final void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
-    setAttr(attributes, SemanticAttributes.RPC_SYSTEM, getter.system(request));
-    setAttr(attributes, SemanticAttributes.RPC_SERVICE, getter.service(request));
-    setAttr(attributes, SemanticAttributes.RPC_METHOD, getter.method(request));
+    internalSet(attributes, SemanticAttributes.RPC_SYSTEM, getter.system(request));
+    internalSet(attributes, SemanticAttributes.RPC_SERVICE, getter.service(request));
+    internalSet(attributes, SemanticAttributes.RPC_METHOD, getter.method(request));
   }
 
   @Override

diff --git a/.../src/main/java/io/opentelemetry/instrumentation/api/instrumenter/AttributesExtractor.java b/.../src/main/java/io/opentelemetry/instrumentation/api/instrumenter/AttributesExtractor.java
@@ -8,7 +8,6 @@
 import io.opentelemetry.api.common.AttributeKey;
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
-import io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil;
 import javax.annotation.Nullable;
 
 /**
@@ -47,7 +46,9 @@ void onEnd(
   // TODO: remove after 1.13 release
   @Deprecated
   default <T> void set(AttributesBuilder attributes, AttributeKey<T> key, @Nullable T value) {
-    AttributesExtractorUtil.setAttr(attributes, key, value);
+    if (value != null) {
+      attributes.put(key, value);
+    }
   }
 
   /**