Better log messages (Azure#36064)

Co-authored-by: heyams <[email protected]>
azure-sdk · Jul 26, 2023 · b3307ed · b3307ed
1 parent d0b8fbf
commit b3307ed
Show file tree

Hide file tree

Showing 8 changed files with 51 additions and 29 deletions.
diff --git a/...r/src/main/java/com/azure/monitor/opentelemetry/exporter/AzureMonitorExporterBuilder.java b/...r/src/main/java/com/azure/monitor/opentelemetry/exporter/AzureMonitorExporterBuilder.java
@@ -290,8 +290,7 @@ private TelemetryItemExporter initExporterBuilder() {
         File tempDir =
             TempDirs.getApplicationInsightsTempDir(
                 LOGGER,
-                "Telemetry will not be stored to disk and retried later"
-                    + " on sporadic network failures");
+                "Telemetry will not be stored to disk and retried on sporadic network failures");
 
         TelemetryItemExporter telemetryItemExporter;
         if (tempDir != null) {

diff --git a/...com/azure/monitor/opentelemetry/exporter/implementation/localstorage/LocalFileSender.java b/...com/azure/monitor/opentelemetry/exporter/implementation/localstorage/LocalFileSender.java
@@ -37,10 +37,11 @@ class LocalFileSender implements Runnable {
         this.telemetryPipeline = telemetryPipeline;
 
         diagnosticListener =
-            suppressWarnings
-                ? TelemetryPipelineListener.noop()
-                : new DiagnosticTelemetryPipelineListener(
-                "Sending telemetry to the ingestion service (retry from disk)", false);
+            suppressWarnings ? TelemetryPipelineListener.noop() :
+                new DiagnosticTelemetryPipelineListener(
+                    "Sending telemetry to the ingestion service (retry from disk)",
+                    true,
+                    " (will be retried again)");
 
         scheduledExecutor.scheduleWithFixedDelay(
             this, intervalSeconds, intervalSeconds, TimeUnit.SECONDS);

diff --git a/...com/azure/monitor/opentelemetry/exporter/implementation/localstorage/LocalFileWriter.java b/...com/azure/monitor/opentelemetry/exporter/implementation/localstorage/LocalFileWriter.java
@@ -3,6 +3,7 @@
 
 package com.azure.monitor.opentelemetry.exporter.implementation.localstorage;
 
+import com.azure.monitor.opentelemetry.exporter.implementation.logging.DiagnosticTelemetryPipelineListener;
 import com.azure.monitor.opentelemetry.exporter.implementation.logging.OperationLogger;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 
@@ -53,11 +54,11 @@ final class LocalFileWriter {
         value = "SECPTI", // Potential Path Traversal
         justification =
             "The constructed file path cannot be controlled by an end user of the instrumented application")
-    void writeToDisk(String connectionString, List<ByteBuffer> buffers) {
+    void writeToDisk(String connectionString, List<ByteBuffer> buffers, String originalErrorMessage) {
         long size = getTotalSizeOfPersistedFiles(telemetryFolder);
         if (size >= diskPersistenceMaxSizeBytes) {
-            operationLogger.recordFailure(
-                "Local persistent storage capacity has been reached. It's currently at ("
+            operationLogger.recordFailure(originalErrorMessage
+                    + ". Local persistent storage capacity has been reached. It's currently at ("
                     + (size / 1024)
                     + "KB). Telemetry will be lost.",
                 DISK_PERSISTENCE_WRITER_ERROR);

diff --git a/...telemetry/exporter/implementation/localstorage/LocalStorageTelemetryPipelineListener.java b/...telemetry/exporter/implementation/localstorage/LocalStorageTelemetryPipelineListener.java
@@ -3,6 +3,7 @@
 
 package com.azure.monitor.opentelemetry.exporter.implementation.localstorage;
 
+import com.azure.monitor.opentelemetry.exporter.implementation.logging.DiagnosticTelemetryPipelineListener;
 import com.azure.monitor.opentelemetry.exporter.implementation.pipeline.TelemetryPipeline;
 import com.azure.monitor.opentelemetry.exporter.implementation.pipeline.TelemetryPipelineListener;
 import com.azure.monitor.opentelemetry.exporter.implementation.pipeline.TelemetryPipelineRequest;
@@ -46,14 +47,15 @@ public LocalStorageTelemetryPipelineListener(
     @Override
     public void onResponse(TelemetryPipelineRequest request, TelemetryPipelineResponse response) {
         if (StatusCode.isRetryable(response.getStatusCode())) {
-            localFileWriter.writeToDisk(request.getConnectionString(), request.getTelemetry());
+            localFileWriter.writeToDisk(
+                request.getConnectionString(), request.getTelemetry(), getOriginalErrorMessage(response));
         }
     }
 
     @Override
     public void onException(
         TelemetryPipelineRequest request, String errorMessage, Throwable throwable) {
-        localFileWriter.writeToDisk(request.getConnectionString(), request.getTelemetry());
+        localFileWriter.writeToDisk(request.getConnectionString(), request.getTelemetry(), errorMessage);
     }
 
     @Override
@@ -66,4 +68,14 @@ public CompletableResultCode shutdown() {
         }
         return CompletableResultCode.ofSuccess();
     }
+
+    private static String getOriginalErrorMessage(TelemetryPipelineResponse response) {
+        int statusCode = response.getStatusCode();
+        if (statusCode == 401 || statusCode == 403) {
+            return DiagnosticTelemetryPipelineListener
+                .getErrorMessageFromCredentialRelatedResponse(statusCode, response.getBody());
+        } else {
+            return "Received response code " + statusCode;
+        }
+    }
 }
diff --git a/...or/opentelemetry/exporter/implementation/logging/DiagnosticTelemetryPipelineListener.java b/...or/opentelemetry/exporter/implementation/logging/DiagnosticTelemetryPipelineListener.java
@@ -31,13 +31,15 @@ public class DiagnosticTelemetryPipelineListener implements TelemetryPipelineLis
     private static final AtomicBoolean friendlyExceptionThrown = new AtomicBoolean();
 
     private final OperationLogger operationLogger;
-    private final boolean suppressWarningsOnRetryableFailures;
+    private final boolean logRetryableFailures;
+    private final String retryableFailureSuffix;
 
     // e.g. "Sending telemetry to the ingestion service"
     public DiagnosticTelemetryPipelineListener(
-        String operation, boolean suppressWarningsOnRetryableFailures) {
+        String operation, boolean logRetryableFailures, String retryableFailureSuffix) {
         operationLogger = new OperationLogger(FOR_CLASS, operation);
-        this.suppressWarningsOnRetryableFailures = suppressWarningsOnRetryableFailures;
+        this.logRetryableFailures = logRetryableFailures;
+        this.retryableFailureSuffix = retryableFailureSuffix;
     }
 
     @Override
@@ -62,7 +64,7 @@ public void onResponse(TelemetryPipelineRequest request, TelemetryPipelineRespon
                 break;
             case 401: // breeze returns if aad enabled and no authentication token provided
             case 403: // breeze returns if aad enabled or disabled (both cases) and
-                if (!suppressWarningsOnRetryableFailures) {
+                if (logRetryableFailures) {
                     operationLogger.recordFailure(
                         getErrorMessageFromCredentialRelatedResponse(responseCode, response.getBody()),
                         INGESTION_ERROR);
@@ -71,12 +73,12 @@ public void onResponse(TelemetryPipelineRequest request, TelemetryPipelineRespon
             case 408: // REQUEST TIMEOUT
             case 429: // TOO MANY REQUESTS
             case 500: // INTERNAL SERVER ERROR
+            case 502: // BAD GATEWAY
             case 503: // SERVICE UNAVAILABLE
-                if (!suppressWarningsOnRetryableFailures) {
+            case 504: // GATEWAY TIMEOUT
+                if (logRetryableFailures) {
                     operationLogger.recordFailure(
-                        "Received response code "
-                            + responseCode
-                            + " (telemetry will be stored to disk and retried later)",
+                        "Received response code " + responseCode + retryableFailureSuffix,
                         INGESTION_ERROR);
                 }
                 break;
@@ -99,9 +101,9 @@ public void onResponse(TelemetryPipelineRequest request, TelemetryPipelineRespon
     public void onException(TelemetryPipelineRequest request, String reason, Throwable throwable) {
         if (!NetworkFriendlyExceptions.logSpecialOneTimeFriendlyException(
             throwable, request.getUrl().toString(), friendlyExceptionThrown, logger)) {
-            if (!suppressWarningsOnRetryableFailures) {
+            if (logRetryableFailures) {
                 operationLogger.recordFailure(
-                    reason + " (telemetry will be stored to disk and retried later)",
+                    reason + retryableFailureSuffix,
                     throwable,
                     INGESTION_ERROR);
             }
@@ -129,7 +131,7 @@ private static Set<String> getErrors(String body) {
             .collect(Collectors.toSet());
     }
 
-    private static String getErrorMessageFromCredentialRelatedResponse(
+    public static String getErrorMessageFromCredentialRelatedResponse(
         int responseCode, String responseBody) {
         JsonNode jsonNode;
         try {
@@ -148,6 +150,6 @@ private static String getErrorMessageFromCredentialRelatedResponse(
         jsonNode.get("errors").forEach(errors::add);
         return errors.get(0).get("message").asText()
             + action
-            + " (telemetry will be stored to disk and retried later)";
+            + " (telemetry will be stored to disk and retried)";
     }
 }
diff --git a/...zure/monitor/opentelemetry/exporter/implementation/localstorage/LocalFileLoaderTests.java b/...zure/monitor/opentelemetry/exporter/implementation/localstorage/LocalFileLoaderTests.java
@@ -90,7 +90,9 @@ public void testDeleteFilePermanentlyOnSuccess() throws Exception {
         // persist 10 files to disk
         for (int i = 0; i < 10; i++) {
             localFileWriter.writeToDisk(
-                CONNECTION_STRING, singletonList(ByteBuffer.wrap("hello world".getBytes(UTF_8))));
+                CONNECTION_STRING,
+                singletonList(ByteBuffer.wrap("hello world".getBytes(UTF_8))),
+                "original error message");
         }
 
         assertThat(localFileCache.getPersistedFilesCache().size()).isEqualTo(10);
@@ -141,7 +143,9 @@ public void testDeleteFilePermanentlyOnFailure() throws Exception {
         // persist 10 files to disk
         for (int i = 0; i < 10; i++) {
             localFileWriter.writeToDisk(
-                CONNECTION_STRING, singletonList(ByteBuffer.wrap("hello world".getBytes(UTF_8))));
+                CONNECTION_STRING,
+                singletonList(ByteBuffer.wrap("hello world".getBytes(UTF_8))),
+                "original error message");
         }
 
         assertThat(localFileCache.getPersistedFilesCache().size()).isEqualTo(10);

diff --git a/...zure/monitor/opentelemetry/exporter/implementation/localstorage/LocalFilePurgerTests.java b/...zure/monitor/opentelemetry/exporter/implementation/localstorage/LocalFilePurgerTests.java
@@ -32,7 +32,8 @@ public void testPurgedExpiredFiles() throws Exception {
         for (int i = 0; i < 100; i++) {
             writer.writeToDisk(
                 "InstrumentationKey=00000000-0000-0000-0000-0FEEDDADBEE;IngestionEndpoint=http://foo.bar/",
-                singletonList(ByteBuffer.wrap(text.getBytes(UTF_8))));
+                singletonList(ByteBuffer.wrap(text.getBytes(UTF_8))),
+                "original error message");
         }
 
         List<File> files = FileUtil.listTrnFiles(tempFolder);

diff --git a/...zure/monitor/opentelemetry/exporter/implementation/localstorage/LocalFileWriterTests.java b/...zure/monitor/opentelemetry/exporter/implementation/localstorage/LocalFileWriterTests.java
@@ -54,15 +54,15 @@ public void testWriteByteBuffersList() throws IOException {
         assertThat(byteBuffers.size()).isEqualTo(10);
 
         LocalFileWriter writer = new LocalFileWriter(50, localFileCache, tempFolder, null, false);
-        writer.writeToDisk(CONNECTION_STRING, byteBuffers);
+        writer.writeToDisk(CONNECTION_STRING, byteBuffers, "original error message");
         assertThat(localFileCache.getPersistedFilesCache().size()).isEqualTo(1);
     }
 
     @Test
     public void testWriteRawByteArray() throws IOException {
         LocalFileWriter writer = new LocalFileWriter(50, localFileCache, tempFolder, null, false);
         byte[] content = Resources.readBytes("write-transmission.txt");
-        writer.writeToDisk(CONNECTION_STRING, singletonList(ByteBuffer.wrap(content)));
+        writer.writeToDisk(CONNECTION_STRING, singletonList(ByteBuffer.wrap(content)), "original error message");
         assertThat(localFileCache.getPersistedFilesCache().size()).isEqualTo(1);
     }
 
@@ -79,7 +79,9 @@ public void testWriteUnderMultipleThreadsEnvironment() throws InterruptedExcepti
                         LocalFileWriter writer =
                             new LocalFileWriter(50, localFileCache, tempFolder, null, false);
                         writer.writeToDisk(
-                            CONNECTION_STRING, singletonList(ByteBuffer.wrap(telemetry.getBytes(UTF_8))));
+                            CONNECTION_STRING,
+                            singletonList(ByteBuffer.wrap(telemetry.getBytes(UTF_8))),
+                            "original error message");
                     }
                 });
         }