Skip to content

Commit 699d1bd

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 7f50d5b commit 699d1bd

File tree

9 files changed

+18
-29
lines changed

9 files changed

+18
-29
lines changed

CONTRIBUTING.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,15 @@ uses [google-java-format](https://github.com/google/google-java-format) library:
146146
synchronized (lock) { ... }
147147
}
148148
```
149+
* When logging exceptions, pass the exception as the `Throwable` parameter to the logger
150+
rather than stringifying it via `getMessage()` or concatenation. This ensures logging
151+
frameworks can render the full stack trace.
152+
```java
153+
// Do:
154+
logger.log(Level.WARNING, "Failed to process request", exception);
155+
// Don't:
156+
logger.warning("Failed to process request: " + exception.getMessage());
157+
```
149158
* Don't
150159
use [gradle test fixtures](https://docs.gradle.org/current/userguide/java_testing.html#sec:java_test_fixtures) (
151160
i.e. `java-test-fixtures` plugin) to reuse code for internal testing. The test fixtures plugin has

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/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
@@ -287,8 +287,7 @@ static <M, R> R createAndMaybeCleanup(
287287
logger.fine("Closing " + closeable.getClass().getName());
288288
closeable.close();
289289
} catch (IOException ex) {
290-
logger.warning(
291-
"Error closing " + closeable.getClass().getName() + ": " + ex.getMessage());
290+
logger.log(Level.WARNING, "Error closing " + closeable.getClass().getName(), ex);
292291
}
293292
}
294293
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
@@ -94,7 +94,7 @@ public DoubleHistogramBuilder setExplicitBucketBoundariesAdvice(List<Double> buc
9494
Objects.requireNonNull(bucketBoundaries, "bucketBoundaries must not be null");
9595
ExplicitBucketHistogramUtils.validateBucketBoundaries(bucketBoundaries);
9696
} catch (IllegalArgumentException | NullPointerException e) {
97-
logger.warning("Error setting explicit bucket boundaries advice: " + e.getMessage());
97+
logger.log(Level.WARNING, "Error setting explicit bucket boundaries advice", e);
9898
return this;
9999
}
100100
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
@@ -99,7 +99,7 @@ public LongHistogramBuilder setExplicitBucketBoundariesAdvice(List<Long> bucketB
9999
boundaries = bucketBoundaries.stream().map(Long::doubleValue).collect(Collectors.toList());
100100
ExplicitBucketHistogramUtils.validateBucketBoundaries(boundaries);
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(boundaries);

0 commit comments

Comments
 (0)