From e01ec3672c6a50cc4090564a802df557816f878f Mon Sep 17 00:00:00 2001 From: Vamshikrishna Monagari Date: Mon, 30 Mar 2026 20:50:30 -0400 Subject: [PATCH] Fix inconsistent exception logging patterns and add CONTRIBUTING.md guidance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Category A — Remove redundant e.getMessage() where exception is already passed as the Throwable parameter: - GrpcExporter.java - HttpExporter.java - JaegerRemoteSampler.java (also fix stale FINEST log to pass exception as Throwable instead of concatenating) Category B — Pass exception as Throwable instead of stringifying via getMessage(), so logging frameworks can render full stack traces: - AutoConfiguredOpenTelemetrySdkBuilder.java - DeclarativeConfiguration.java - TracerShim.java - SdkDoubleHistogram.java - SdkLongHistogram.java Add best practice guidance to CONTRIBUTING.md recommending the use of dedicated logger overloads that accept a Throwable parameter. Resolves #8228 --- CONTRIBUTING.md | 9 +++++++++ .../exporter/internal/grpc/GrpcExporter.java | 7 +------ .../exporter/internal/http/HttpExporter.java | 7 +------ .../io/opentelemetry/opentracingshim/TracerShim.java | 5 +---- .../AutoConfiguredOpenTelemetrySdkBuilder.java | 3 +-- .../AutoConfiguredOpenTelemetrySdkTest.java | 2 +- .../incubator/fileconfig/DeclarativeConfiguration.java | 3 +-- .../trace/jaeger/sampler/JaegerRemoteSampler.java | 9 ++------- .../io/opentelemetry/sdk/metrics/SdkDoubleHistogram.java | 2 +- .../io/opentelemetry/sdk/metrics/SdkLongHistogram.java | 2 +- .../sdk/metrics/ExplicitBucketBoundariesAdviceTest.java | 6 +++--- 11 files changed, 22 insertions(+), 33 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9cb55cf8a37..f5915074fc4 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -146,6 +146,15 @@ uses [google-java-format](https://github.com/google/google-java-format) library: synchronized (lock) { ... } } ``` +* When logging exceptions, pass the exception as the `Throwable` parameter to the logger + rather than stringifying it via `getMessage()` or concatenation. This ensures logging + frameworks can render the full stack trace. + ```java + // Do: + logger.log(Level.WARNING, "Failed to process request", exception); + // Don't: + logger.warning("Failed to process request: " + exception.getMessage()); + ``` * Don't use [gradle test fixtures](https://docs.gradle.org/current/userguide/java_testing.html#sec:java_test_fixtures) ( i.e. `java-test-fixtures` plugin) to reuse code for internal testing. The test fixtures plugin has diff --git a/exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporter.java b/exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporter.java index 8fd12d82909..e23fc57f652 100644 --- a/exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporter.java +++ b/exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporter.java @@ -126,12 +126,7 @@ private void onError( Throwable e) { metricRecording.finishFailed(e); logger.log( - Level.SEVERE, - "Failed to export " - + type - + "s. The request could not be executed. Error message: " - + e.getMessage(), - e); + Level.SEVERE, "Failed to export " + type + "s. The request could not be executed.", e); if (logger.isLoggable(Level.FINEST)) { logger.log(Level.FINEST, "Failed to export " + type + "s. Details follow:", e); } diff --git a/exporters/common/src/main/java/io/opentelemetry/exporter/internal/http/HttpExporter.java b/exporters/common/src/main/java/io/opentelemetry/exporter/internal/http/HttpExporter.java index 277177eda19..db2dd70a10c 100644 --- a/exporters/common/src/main/java/io/opentelemetry/exporter/internal/http/HttpExporter.java +++ b/exporters/common/src/main/java/io/opentelemetry/exporter/internal/http/HttpExporter.java @@ -117,12 +117,7 @@ private void onError( Throwable e) { metricRecording.finishFailed(e); logger.log( - Level.SEVERE, - "Failed to export " - + type - + "s. The request could not be executed. Full error message: " - + e.getMessage(), - e); + Level.SEVERE, "Failed to export " + type + "s. The request could not be executed.", e); result.failExceptionally(FailedExportException.httpFailedExceptionally(e)); } diff --git a/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/TracerShim.java b/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/TracerShim.java index ace67e893e6..8f3ba836328 100644 --- a/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/TracerShim.java +++ b/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/TracerShim.java @@ -108,10 +108,7 @@ public SpanContext extract(Format format, C carrier) { } } catch (RuntimeException e) { logger.log( - Level.WARNING, - "Exception caught while extracting span context; returning null. " - + "Exception: [{0}] Message: [{1}]", - new String[] {e.getClass().getName(), e.getMessage()}); + Level.WARNING, "Exception caught while extracting span context; returning null.", e); } return null; diff --git a/sdk-extensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkBuilder.java b/sdk-extensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkBuilder.java index c4900f6d92a..b5a8fb153c0 100644 --- a/sdk-extensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkBuilder.java +++ b/sdk-extensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkBuilder.java @@ -501,8 +501,7 @@ private AutoConfiguredOpenTelemetrySdk buildImpl() { logger.fine("Closing " + closeable.getClass().getName()); closeable.close(); } catch (IOException ex) { - logger.warning( - "Error closing " + closeable.getClass().getName() + ": " + ex.getMessage()); + logger.log(Level.WARNING, "Error closing " + closeable.getClass().getName(), ex); } } if (e instanceof ConfigurationException) { diff --git a/sdk-extensions/autoconfigure/src/test/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkTest.java b/sdk-extensions/autoconfigure/src/test/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkTest.java index 8ed8c578693..c2bd7221408 100644 --- a/sdk-extensions/autoconfigure/src/test/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkTest.java +++ b/sdk-extensions/autoconfigure/src/test/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkTest.java @@ -703,7 +703,7 @@ void configurationError_ClosesResources() { verify(tracerProvider).close(); verify(meterProvider).close(); - logs.assertContains("Error closing io.opentelemetry.sdk.trace.SdkTracerProvider: Error!"); + logs.assertContains("Error closing io.opentelemetry.sdk.trace.SdkTracerProvider"); } @Test diff --git a/sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/DeclarativeConfiguration.java b/sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/DeclarativeConfiguration.java index 6d351386e1d..2d38f24aec3 100644 --- a/sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/DeclarativeConfiguration.java +++ b/sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/DeclarativeConfiguration.java @@ -287,8 +287,7 @@ static R createAndMaybeCleanup( logger.fine("Closing " + closeable.getClass().getName()); closeable.close(); } catch (IOException ex) { - logger.warning( - "Error closing " + closeable.getClass().getName() + ": " + ex.getMessage()); + logger.log(Level.WARNING, "Error closing " + closeable.getClass().getName(), ex); } } if (e instanceof DeclarativeConfigException) { diff --git a/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSampler.java b/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSampler.java index 75202072524..ca4d581ac3b 100644 --- a/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSampler.java +++ b/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSampler.java @@ -128,14 +128,9 @@ private void onResponse(GrpcResponse grpcResponse) { private static void onError(Throwable e) { logger.log( - Level.SEVERE, - "Failed to execute " - + TYPE - + "s. The request could not be executed. Error message: " - + e.getMessage(), - e); + Level.SEVERE, "Failed to execute " + TYPE + "s. The request could not be executed.", e); if (logger.isLoggable(Level.FINEST)) { - logger.log(Level.FINEST, "Failed to execute " + TYPE + "s. Details follow: " + e); + logger.log(Level.FINEST, "Failed to execute " + TYPE + "s. Details follow:", e); } } diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkDoubleHistogram.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkDoubleHistogram.java index 15f0f5ea44d..8fb69109b13 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkDoubleHistogram.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkDoubleHistogram.java @@ -94,7 +94,7 @@ public DoubleHistogramBuilder setExplicitBucketBoundariesAdvice(List buc Objects.requireNonNull(bucketBoundaries, "bucketBoundaries must not be null"); ExplicitBucketHistogramUtils.validateBucketBoundaries(bucketBoundaries); } catch (IllegalArgumentException | NullPointerException e) { - logger.warning("Error setting explicit bucket boundaries advice: " + e.getMessage()); + logger.log(Level.WARNING, "Error setting explicit bucket boundaries advice", e); return this; } builder.setExplicitBucketBoundaries(bucketBoundaries); diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkLongHistogram.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkLongHistogram.java index 4f935d0d091..72f737a895c 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkLongHistogram.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkLongHistogram.java @@ -99,7 +99,7 @@ public LongHistogramBuilder setExplicitBucketBoundariesAdvice(List bucketB boundaries = bucketBoundaries.stream().map(Long::doubleValue).collect(Collectors.toList()); ExplicitBucketHistogramUtils.validateBucketBoundaries(boundaries); } catch (IllegalArgumentException | NullPointerException e) { - logger.warning("Error setting explicit bucket boundaries advice: " + e.getMessage()); + logger.log(Level.WARNING, "Error setting explicit bucket boundaries advice", e); return this; } builder.setExplicitBucketBoundaries(boundaries); diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/ExplicitBucketBoundariesAdviceTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/ExplicitBucketBoundariesAdviceTest.java index 765d583e520..c35864e1cef 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/ExplicitBucketBoundariesAdviceTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/ExplicitBucketBoundariesAdviceTest.java @@ -266,7 +266,7 @@ private static Stream histogramsWithInvalidAdvice() { .build(); return histogram::record; }, - "Error setting explicit bucket boundaries advice: Bucket boundaries must be in increasing order: 10.0 >= 9.0"), + "Error setting explicit bucket boundaries advice"), Arguments.of( (Function>) meterProvider -> { @@ -279,7 +279,7 @@ private static Stream histogramsWithInvalidAdvice() { .build(); return histogram::record; }, - "Error setting explicit bucket boundaries advice: Bucket boundaries must be in increasing order: 10.0 >= 9.0"), + "Error setting explicit bucket boundaries advice"), Arguments.of( (Function>) meterProvider -> { @@ -291,6 +291,6 @@ private static Stream histogramsWithInvalidAdvice() { .build(); return histogram::record; }, - "Error setting explicit bucket boundaries advice: bucketBoundaries must not be null")); + "Error setting explicit bucket boundaries advice")); } }