Skip to content

Commit 718b3c7

Browse files
Fix inconsistent exception logging patterns and add CONTRIBUTING.md guidance
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
1 parent 3c0f6ff commit 718b3c7

11 files changed

Lines changed: 24 additions & 33 deletions

File tree

docs/knowledge/general-patterns.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,17 @@ automatically by `otel.java-conventions`).
123123
private static final Logger LOGGER = Logger.getLogger(MyClass.class.getName());
124124
```
125125

126+
When logging exceptions, pass the exception as the `Throwable` parameter to the logger rather
127+
than stringifying it via `getMessage()` or concatenation. This ensures logging frameworks can
128+
render the full stack trace.
129+
130+
```java
131+
// Do:
132+
logger.log(Level.WARNING, "Failed to process request", exception);
133+
// Don't:
134+
logger.warning("Failed to process request: " + exception.getMessage());
135+
```
136+
126137
## toString()
127138

128139
Adding `toString()` overrides is encouraged for debugging assistance. All `toString()`

exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporter.java

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -126,12 +126,7 @@ private void onError(
126126
Throwable e) {
127127
metricRecording.finishFailed(e);
128128
logger.log(
129-
Level.SEVERE,
130-
"Failed to export "
131-
+ type
132-
+ "s. The request could not be executed. Error message: "
133-
+ e.getMessage(),
134-
e);
129+
Level.SEVERE, "Failed to export " + type + "s. The request could not be executed.", e);
135130
if (logger.isLoggable(Level.FINEST)) {
136131
logger.log(Level.FINEST, "Failed to export " + type + "s. Details follow:", e);
137132
}

exporters/common/src/main/java/io/opentelemetry/exporter/internal/http/HttpExporter.java

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -117,12 +117,7 @@ private void onError(
117117
Throwable e) {
118118
metricRecording.finishFailed(e);
119119
logger.log(
120-
Level.SEVERE,
121-
"Failed to export "
122-
+ type
123-
+ "s. The request could not be executed. Full error message: "
124-
+ e.getMessage(),
125-
e);
120+
Level.SEVERE, "Failed to export " + type + "s. The request could not be executed.", e);
126121
result.failExceptionally(FailedExportException.httpFailedExceptionally(e));
127122
}
128123

opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/TracerShim.java

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -108,10 +108,7 @@ public <C> SpanContext extract(Format<C> format, C carrier) {
108108
}
109109
} catch (RuntimeException e) {
110110
logger.log(
111-
Level.WARNING,
112-
"Exception caught while extracting span context; returning null. "
113-
+ "Exception: [{0}] Message: [{1}]",
114-
new String[] {e.getClass().getName(), e.getMessage()});
111+
Level.WARNING, "Exception caught while extracting span context; returning null.", e);
115112
}
116113

117114
return null;

sdk-extensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkBuilder.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -501,8 +501,7 @@ private AutoConfiguredOpenTelemetrySdk buildImpl() {
501501
logger.fine("Closing " + closeable.getClass().getName());
502502
closeable.close();
503503
} catch (IOException ex) {
504-
logger.warning(
505-
"Error closing " + closeable.getClass().getName() + ": " + ex.getMessage());
504+
logger.log(Level.WARNING, "Error closing " + closeable.getClass().getName(), ex);
506505
}
507506
}
508507
if (e instanceof ConfigurationException) {

sdk-extensions/autoconfigure/src/test/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -703,7 +703,7 @@ void configurationError_ClosesResources() {
703703
verify(tracerProvider).close();
704704
verify(meterProvider).close();
705705

706-
logs.assertContains("Error closing io.opentelemetry.sdk.trace.SdkTracerProvider: Error!");
706+
logs.assertContains("Error closing io.opentelemetry.sdk.trace.SdkTracerProvider");
707707
}
708708

709709
@Test

sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/DeclarativeConfiguration.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -284,8 +284,7 @@ static <M, R> R createAndMaybeCleanup(
284284
logger.fine("Closing " + closeable.getClass().getName());
285285
closeable.close();
286286
} catch (IOException ex) {
287-
logger.warning(
288-
"Error closing " + closeable.getClass().getName() + ": " + ex.getMessage());
287+
logger.log(Level.WARNING, "Error closing " + closeable.getClass().getName(), ex);
289288
}
290289
}
291290
if (e instanceof DeclarativeConfigException) {

sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSampler.java

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -128,14 +128,9 @@ private void onResponse(GrpcResponse grpcResponse) {
128128

129129
private static void onError(Throwable e) {
130130
logger.log(
131-
Level.SEVERE,
132-
"Failed to execute "
133-
+ TYPE
134-
+ "s. The request could not be executed. Error message: "
135-
+ e.getMessage(),
136-
e);
131+
Level.SEVERE, "Failed to execute " + TYPE + "s. The request could not be executed.", e);
137132
if (logger.isLoggable(Level.FINEST)) {
138-
logger.log(Level.FINEST, "Failed to execute " + TYPE + "s. Details follow: " + e);
133+
logger.log(Level.FINEST, "Failed to execute " + TYPE + "s. Details follow:", e);
139134
}
140135
}
141136

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkDoubleHistogram.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ public DoubleHistogramBuilder setExplicitBucketBoundariesAdvice(List<Double> buc
9999
Objects.requireNonNull(bucketBoundaries, "bucketBoundaries must not be null");
100100
ExplicitBucketHistogramUtils.validateBucketBoundaries(bucketBoundaries);
101101
} catch (IllegalArgumentException | NullPointerException e) {
102-
logger.warning("Error setting explicit bucket boundaries advice: " + e.getMessage());
102+
logger.log(Level.WARNING, "Error setting explicit bucket boundaries advice", e);
103103
return this;
104104
}
105105
builder.setExplicitBucketBoundaries(bucketBoundaries);

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkLongHistogram.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ public LongHistogramBuilder setExplicitBucketBoundariesAdvice(List<Long> bucketB
104104
boundaries = bucketBoundaries.stream().map(Long::doubleValue).collect(Collectors.toList());
105105
ExplicitBucketHistogramUtils.validateBucketBoundaries(boundaries);
106106
} catch (IllegalArgumentException | NullPointerException e) {
107-
logger.warning("Error setting explicit bucket boundaries advice: " + e.getMessage());
107+
logger.log(Level.WARNING, "Error setting explicit bucket boundaries advice", e);
108108
return this;
109109
}
110110
builder.setExplicitBucketBoundaries(boundaries);

0 commit comments

Comments
 (0)