More cleanup to exporter code (Azure#19602)

srnagar · trask · web-flow · commit 282deafe0123 · 2021-03-03T22:19:33.000-08:00
* More cleanup to exporter code

* Remove unused imports

* Update sdk/monitor/azure-monitor-opentelemetry-exporter/src/main/java/com/azure/monitor/opentelemetry/exporter/AzureMonitorTraceExporter.java

Co-authored-by: Trask Stalnaker &lt;trask.stalnaker@gmail.com&gt;

Co-authored-by: Trask Stalnaker &lt;trask.stalnaker@gmail.com&gt;
diff --git a/sdk/monitor/azure-monitor-opentelemetry-exporter/pom.xml b/sdk/monitor/azure-monitor-opentelemetry-exporter/pom.xml
@@ -77,12 +77,6 @@
       <version>5.6.3</version> <!-- {x-version-update;org.junit.jupiter:junit-jupiter-params;external_dependency} -->
       <scope>test</scope>
     </dependency>
-    <dependency>
-      <groupId>com.azure</groupId>
-      <artifactId>azure-core-test</artifactId>
-      <version>1.5.3</version> <!-- {x-version-update;com.azure:azure-core-test;dependency} -->
-      <scope>test</scope>
-    </dependency>
 
     <dependency>
       <groupId>com.azure</groupId>
diff --git a/sdk/monitor/azure-monitor-opentelemetry-exporter/src/main/java/com/azure/monitor/opentelemetry/exporter/AzureMonitorExporterBuilder.java b/sdk/monitor/azure-monitor-opentelemetry-exporter/src/main/java/com/azure/monitor/opentelemetry/exporter/AzureMonitorExporterBuilder.java
@@ -3,18 +3,19 @@
 
 package com.azure.monitor.opentelemetry.exporter;
 
-import com.azure.monitor.opentelemetry.exporter.implementation.ApplicationInsightsClientImpl;
-import com.azure.monitor.opentelemetry.exporter.implementation.ApplicationInsightsClientImplBuilder;
-import com.azure.monitor.opentelemetry.exporter.implementation.NdJsonSerializer;
 import com.azure.core.http.HttpClient;
 import com.azure.core.http.HttpPipeline;
 import com.azure.core.http.policy.HttpLogDetailLevel;
 import com.azure.core.http.policy.HttpLogOptions;
 import com.azure.core.http.policy.HttpPipelinePolicy;
 import com.azure.core.http.policy.RetryPolicy;
+import com.azure.core.util.ClientOptions;
 import com.azure.core.util.Configuration;
 import com.azure.core.util.logging.ClientLogger;
 import com.azure.core.util.serializer.JacksonAdapter;
+import com.azure.monitor.opentelemetry.exporter.implementation.ApplicationInsightsClientImpl;
+import com.azure.monitor.opentelemetry.exporter.implementation.ApplicationInsightsClientImplBuilder;
+import com.azure.monitor.opentelemetry.exporter.implementation.NdJsonSerializer;
 import com.fasterxml.jackson.databind.module.SimpleModule;
 import io.opentelemetry.sdk.trace.export.SpanExporter;
 
@@ -29,11 +30,10 @@
  * {@link SpanExporter} interface defined by OpenTelemetry API specification.
  */
 public final class AzureMonitorExporterBuilder {
-    public static final String APPLICATIONINSIGHTS_CONNECTION_STRING = "APPLICATIONINSIGHTS_CONNECTION_STRING";
+    private static final String APPLICATIONINSIGHTS_CONNECTION_STRING = "APPLICATIONINSIGHTS_CONNECTION_STRING";
     private final ClientLogger logger = new ClientLogger(AzureMonitorExporterBuilder.class);
     private final ApplicationInsightsClientImplBuilder restServiceClientBuilder;
     private String instrumentationKey;
-    private String endpoint;
     private String connectionString;
     private AzureMonitorExporterServiceVersion serviceVersion;
 
@@ -53,10 +53,8 @@ public AzureMonitorExporterBuilder() {
      */
     AzureMonitorExporterBuilder endpoint(String endpoint) {
         Objects.requireNonNull(endpoint, "'endpoint' cannot be null.");
-
         try {
             URL url = new URL(endpoint);
-            this.endpoint = endpoint;
             restServiceClientBuilder.host(url.getProtocol() + "://" + url.getHost());
         } catch (MalformedURLException ex) {
             throw logger.logExceptionAsWarning(
@@ -140,6 +138,18 @@ public AzureMonitorExporterBuilder configuration(Configuration configuration) {
         return this;
     }
 
+
+    /**
+     * Sets the client options such as application ID and custom headers to set on a request.
+     *
+     * @param clientOptions The client options.
+     * @return The updated {@link AzureMonitorExporterBuilder} object.
+     */
+    public AzureMonitorExporterBuilder clientOptions(ClientOptions clientOptions) {
+        restServiceClientBuilder.clientOptions(clientOptions);
+        return this;
+    }
+
     /**
      * Sets the connection string to use for exporting telemetry events to Azure Monitor.
      * @param connectionString The connection string for the Azure Monitor resource.
diff --git a/sdk/monitor/azure-monitor-opentelemetry-exporter/src/main/java/com/azure/monitor/opentelemetry/exporter/AzureMonitorTraceExporter.java b/sdk/monitor/azure-monitor-opentelemetry-exporter/src/main/java/com/azure/monitor/opentelemetry/exporter/AzureMonitorTraceExporter.java
@@ -122,7 +122,7 @@ public final class AzureMonitorTraceExporter implements SpanExporter {
      */
     @Override
     public CompletableResultCode export(Collection<SpanData> spans) {
-
+        CompletableResultCode completableResultCode = new CompletableResultCode();
         try {
             List<TelemetryItem> telemetryItems = new ArrayList<>();
             for (SpanData span : spans) {
@@ -131,11 +131,11 @@ public CompletableResultCode export(Collection<SpanData> spans) {
             }
             client.export(telemetryItems)
                 .subscriberContext(Context.of(Tracer.DISABLE_TRACING_KEY, true))
-                .subscribe();
-            return CompletableResultCode.ofSuccess();
+                .subscribe(ignored -> { }, error -> completableResultCode.fail(), completableResultCode::succeed);
+            return completableResultCode;
         } catch (Throwable t) {
             logger.error(t.getMessage(), t);
-            return CompletableResultCode.ofFailure();
+            return completableResultCode.fail();
         }
     }
 
@@ -160,25 +160,18 @@ private void export(SpanData span, List<TelemetryItem> telemetryItems) {
         String instrumentationName = span.getInstrumentationLibraryInfo().getName();
         Matcher matcher = COMPONENT_PATTERN.matcher(instrumentationName);
         String stdComponent = matcher.matches() ? matcher.group(1) : null;
-        if ("jms".equals(stdComponent) && !span.getParentSpanContext().isValid() && kind == SpanKind.CONSUMER) {
-            // no need to capture these, at least is consistent with prior behavior
-            // these tend to be frameworks pulling messages which are then pushed to consumers
-            // where we capture them
-            return;
-        }
         if (kind == SpanKind.INTERNAL) {
             if ("spring-scheduling".equals(stdComponent) && !span.getParentSpanContext().isValid()) {
-                // TODO (srnagar) need semantic convention for determining whether to map INTERNAL to request or dependency
-                //  (or need clarification to use SERVER for this)
+            // if (!span.getParentSpanContext().isValid()) {
+                // TODO (trask) need semantic convention for determining whether to map INTERNAL to request or
+                //  dependency (or need clarification to use SERVER for this)
                 exportRequest(span, telemetryItems);
             } else {
                 exportRemoteDependency(span, true, telemetryItems);
             }
         } else if (kind == SpanKind.CLIENT || kind == SpanKind.PRODUCER) {
             exportRemoteDependency(span, false, telemetryItems);
         } else if (kind == SpanKind.CONSUMER && !span.getParentSpanContext().isRemote()) {
-            // TODO (srnagar) need spec clarification, but it seems polling for messages can be CONSUMER also
-            //  in which case the span will not have a remote parent and should be treated as a dependency instead of a request
             exportRemoteDependency(span, false, telemetryItems);
         } else if (kind == SpanKind.SERVER || kind == SpanKind.CONSUMER) {
             exportRequest(span, telemetryItems);
@@ -199,6 +192,7 @@ private static List<TelemetryExceptionDetails> minimalParse(String errorStack) {
         } else {
             details.setTypeName(line);
         }
+        // TODO (trask): map OpenTelemetry exception to Application Insights exception better
         details.setStack(errorStack);
         return Collections.singletonList(details);
     }
@@ -305,7 +299,7 @@ private void applyHttpClientSpan(Attributes attributes, RemoteDependencyData tel
                     target += ":" + uri.getPort();
                 }
             } catch (URISyntaxException e) {
-                // TODO (srnagar) "log once"
+                // TODO (trask) "log once"
                 logger.error(e.getMessage());
                 logger.verbose(e.getMessage(), e);
             }
@@ -323,7 +317,7 @@ private void applyHttpClientSpan(Attributes attributes, RemoteDependencyData tel
         } else {
             // using "Http (tracked component)" is important for dependencies that go cross-component (have an appId in their target field)
             // if you use just HTTP, Breeze will remove appid from the target
-            // TODO (srnagar) remove this once confirmed by zakima that it is no longer needed
+            // TODO (trask) remove this once confirmed by zakima that it is no longer needed
             telemetry.setType("Http (tracked component)");
             telemetry.setTarget(target + " | " + targetAppId);
         }
@@ -413,9 +407,8 @@ private void applyMessagingClientSpan(Attributes attributes, RemoteDependencyDat
 
     private static int getDefaultPortForDbSystem(String dbSystem) {
         switch (dbSystem) {
-            // TODO (srnagar) replace these with constants from OpenTelemetry API after upgrading to 0.10.0
-            // TODO (srnagar) add these default ports to the OpenTelemetry database semantic conventions spec
-            // TODO (srnagar) need to add more default ports once jdbc instrumentation reports net.peer.*
+            // TODO (trask) add these default ports to the OpenTelemetry database semantic conventions spec
+            // TODO (trask) need to add more default ports once jdbc instrumentation reports net.peer.*
             case "mongodb":
                 return 27017;
             case "cassandra":
@@ -454,7 +447,7 @@ private void exportRequest(SpanData span, List<TelemetryItem> telemetryItems) {
         if (source == null) {
             String messagingSystem = attributes.get(SemanticAttributes.MESSAGING_SYSTEM);
             if (messagingSystem != null) {
-                // TODO (srnagar) should this pass default port for messaging.system?
+                // TODO (trask) should this pass default port for messaging.system?
                 source = nullAwareConcat(getTargetFromPeerAttributes(attributes, 0),
                     attributes.get(SemanticAttributes.MESSAGING_DESTINATION), "/");
                 if (source == null) {
@@ -565,16 +558,10 @@ private void exportEvents(SpanData span, Double samplingPercentage, List<Telemet
 
             if (event.getAttributes().get(SemanticAttributes.EXCEPTION_TYPE) != null
                 || event.getAttributes().get(SemanticAttributes.EXCEPTION_MESSAGE) != null) {
-                // TODO (srnagar): Remove this boolean after we can confirm that the exception duplicate
-                //  is a bug from the opentelmetry-java-instrumentation
-                if (!foundException) {
-                    // TODO (srnagar): map OpenTelemetry exception to Application Insights exception better
-                    String stacktrace = event.getAttributes().get(SemanticAttributes.EXCEPTION_STACKTRACE);
-                    if (stacktrace != null) {
-                        trackException(stacktrace, span, operationId, span.getSpanId(), samplingPercentage, telemetryItems);
-                    }
+                String stacktrace = event.getAttributes().get(SemanticAttributes.EXCEPTION_STACKTRACE);
+                if (stacktrace != null) {
+                    trackException(stacktrace, span, operationId, span.getSpanId(), samplingPercentage, telemetryItems);
                 }
-                foundException = true;
             } else {
                 telemetryItem.setSampleRate(samplingPercentage.floatValue());
                 telemetryItems.add(telemetryItem);
diff --git a/sdk/monitor/azure-monitor-opentelemetry-exporter/src/main/java/com/azure/monitor/opentelemetry/exporter/implementation/ApplicationInsightsClientImplBuilder.java b/sdk/monitor/azure-monitor-opentelemetry-exporter/src/main/java/com/azure/monitor/opentelemetry/exporter/implementation/ApplicationInsightsClientImplBuilder.java
@@ -11,13 +11,14 @@
 import com.azure.core.http.policy.HttpLogOptions;
 import com.azure.core.http.policy.HttpLoggingPolicy;
 import com.azure.core.http.policy.HttpPipelinePolicy;
-import com.azure.core.http.policy.HttpPolicyProviders;
 import com.azure.core.http.policy.RetryPolicy;
 import com.azure.core.http.policy.UserAgentPolicy;
+import com.azure.core.util.ClientOptions;
 import com.azure.core.util.Configuration;
 import com.azure.core.util.CoreUtils;
 import com.azure.core.util.serializer.JacksonAdapter;
 import com.azure.core.util.serializer.SerializerAdapter;
+
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -31,6 +32,7 @@ public final class ApplicationInsightsClientImplBuilder {
 
     private final Map<String, String> properties =
             CoreUtils.getProperties("azure-monitor-opentelemetry-exporter.properties");
+    private ClientOptions clientOptions;
 
     public ApplicationInsightsClientImplBuilder() {
         this.pipelinePolicies = new ArrayList<>();
@@ -166,6 +168,17 @@ public ApplicationInsightsClientImplBuilder addPolicy(HttpPipelinePolicy customP
         return this;
     }
 
+    /**
+     * Sets the client options such as application ID and custom headers to set on a request.
+     *
+     * @param clientOptions The client options.
+     * @return The updated {@link ApplicationInsightsClientImplBuilder} object.
+     */
+    public ApplicationInsightsClientImplBuilder clientOptions(ClientOptions clientOptions) {
+        this.clientOptions = clientOptions;
+        return this;
+    }
+
     /**
      * Builds an instance of ApplicationInsightsClientImpl with the provided parameters.
      *
@@ -191,16 +204,20 @@ private HttpPipeline createHttpPipeline() {
         if (httpLogOptions == null) {
             httpLogOptions = new HttpLogOptions();
         }
+
+        if (clientOptions == null) {
+            clientOptions = new ClientOptions();
+        }
         List<HttpPipelinePolicy> policies = new ArrayList<>();
         String clientName = properties.getOrDefault(SDK_NAME, "UnknownName");
         String clientVersion = properties.getOrDefault(SDK_VERSION, "UnknownVersion");
-        policies.add(
-                new UserAgentPolicy(httpLogOptions.getApplicationId(), clientName, clientVersion, buildConfiguration));
-        HttpPolicyProviders.addBeforeRetryPolicies(policies);
+
+        String applicationId = CoreUtils.getApplicationId(clientOptions, httpLogOptions);
+
+        policies.add(new UserAgentPolicy(applicationId, clientName, clientVersion, buildConfiguration));
         policies.add(retryPolicy == null ? new RetryPolicy() : retryPolicy);
         policies.add(new CookiePolicy());
         policies.addAll(this.pipelinePolicies);
-        HttpPolicyProviders.addAfterRetryPolicies(policies);
         policies.add(new HttpLoggingPolicy(httpLogOptions));
         HttpPipeline httpPipeline =
                 new HttpPipelineBuilder()
