Skip to content

Commit 86fd832

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 86fd832

12 files changed

Lines changed: 213 additions & 33 deletions

File tree

CONTRIBUTING.md

Lines changed: 189 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,8 +63,197 @@ iterations, scores with error margins) so reviewers can evaluate the change.
6363

6464
## User-facing documentation
6565

66+
<<<<<<< HEAD
6667
End-user documentation for the Java SDK lives at
6768
[opentelemetry.io/docs/languages/java/](https://opentelemetry.io/docs/languages/java/), with
6869
source in [github.com/open-telemetry/opentelemetry.io](https://github.com/open-telemetry/opentelemetry.io).
6970
If your change affects user-visible behavior — configuration options, new features, changed
7071
defaults — please update or open an issue against the documentation there.
72+
=======
73+
```bash
74+
$ ./gradlew spotlessApply
75+
```
76+
77+
To verify code style manually run the following command, which
78+
uses [google-java-format](https://github.com/google/google-java-format) library:
79+
80+
`./gradlew spotlessCheck`
81+
82+
### Best practices that we follow
83+
84+
* This project uses [semantic versioning](https://semver.org/). Except for major versions, a user
85+
should be able to update their dependency version on this project and have nothing break. This
86+
means we do not make breaking changes to the API (e.g., remove a public method) or to the ABI (
87+
e.g., change return type from void to non-void).
88+
* Avoid exposing publicly any class/method/variable that don't need to be public.
89+
* By default, all arguments/members are treated as non-null. Every argument/member that can
90+
be `null` must be annotated with `@Nullable`.
91+
* The project aims to provide a consistent experience across all the public APIs. It is important to
92+
ensure consistency (same look and feel) across different public packages.
93+
* Use `final` for public classes everywhere it is possible, this ensures that these classes cannot
94+
be extended when the API does not intend to offer that functionality.
95+
* In general, we use the following ordering of class members:
96+
* Static fields (final before non-final)
97+
* Instance fields (final before non-final)
98+
* Constructors
99+
* In static utility classes (where all members are static), the private constructor
100+
(used to prevent construction) should be ordered after methods instead of before methods.
101+
* Methods
102+
* If methods call each other, it's nice if the calling method is ordered (somewhere) above the
103+
method that it calls. So, for one example, a private method would be ordered (somewhere) below
104+
the non-private methods that use it.
105+
* Nested classes
106+
* Adding `toString()` overrides on classes is encouraged, but we only use `toString()` to provide
107+
debugging assistance. The implementations of all `toString()` methods should be considered to be
108+
unstable unless explicitly documented otherwise.
109+
* Avoid synchronizing using a class's intrinsic lock. Instead, synchronize on a dedicated lock object. E.g:
110+
```java
111+
private final Object lock = new Object();
112+
113+
public void doSomething() {
114+
synchronized (lock) { ... }
115+
}
116+
```
117+
* When logging exceptions, pass the exception as the `Throwable` parameter to the logger
118+
rather than stringifying it via `getMessage()` or concatenation. This ensures logging
119+
frameworks can render the full stack trace.
120+
```java
121+
// Do:
122+
logger.log(Level.WARNING, "Failed to process request", exception);
123+
// Don't:
124+
logger.warning("Failed to process request: " + exception.getMessage());
125+
```
126+
* Don't
127+
use [gradle test fixtures](https://docs.gradle.org/current/userguide/java_testing.html#sec:java_test_fixtures) (
128+
i.e. `java-test-fixtures` plugin) to reuse code for internal testing. The test fixtures plugin has
129+
side effects where test dependencies are added to the `pom.xml` and publishes an
130+
extra `*-test-fixtures.jar` artifact which is unnecessary for internal testing. Instead, create a
131+
new `*:testing-internal` module and omit the `otel.java-conventions`. For example,
132+
see [/exporters/otlp/testing-internal](./exporters/otlp/testing-internal).
133+
134+
If you notice any practice being applied in the project consistently that isn't listed here, please
135+
consider a pull request to add it.
136+
137+
### Pre-commit hook
138+
139+
To completely delegate code style formatting to the machine, you can
140+
add [git pre-commit hook](https://git-scm.com/docs/githooks). We provide an example script
141+
in `buildscripts/pre-commit` file. Just copy or symlink it into `.git/hooks` folder.
142+
143+
### Editorconfig
144+
145+
As additional convenience for IntelliJ Idea users, we provide `.editorconfig` file. Idea will
146+
automatically use it to adjust its code formatting settings. It does not support all required rules,
147+
so you still have to run `spotlessApply` from time to time.
148+
149+
### Javadoc
150+
151+
* All public classes and their public and protected methods MUST have javadoc. It MUST be complete (
152+
all params documented etc.) Everything else
153+
(package-protected classes, private) MAY have javadoc, at the code writer's whim. It does not have
154+
to be complete, and reviewers are not allowed to require or disallow it.
155+
* Each API element should have a `@since` tag specifying the minor version when it was released (or
156+
the next minor version).
157+
* There MUST be NO javadoc errors.
158+
159+
See [section 7.3.1](https://google.github.io/styleguide/javaguide.html#s7.3.1-javadoc-exception-self-explanatory)
160+
in the guide for exceptions to the Javadoc requirement.
161+
162+
* Reviewers may request documentation for any element that doesn't require Javadoc, though the style
163+
of documentation is up to the author.
164+
* Try to do the least amount of change when modifying existing documentation. Don't change the style
165+
unless you have a good reason.
166+
* We do not use `@author` tags in our javadoc.
167+
* Our javadoc is available via [
168+
javadoc.io}(https://javadoc.io/doc/io.opentelemetry/opentelemetry-api)
169+
170+
### SDK Configuration Documentation
171+
172+
All changes to the SDK configuration options or autoconfigure module should be documented on
173+
[opentelemetry.io](https://opentelemetry.io/docs/languages/java/configuration/).
174+
175+
### AutoValue
176+
177+
* Use [AutoValue](https://github.com/google/auto/tree/master/value), when possible, for any new
178+
value classes. Remember to add package-private constructors to all AutoValue classes to prevent
179+
classes in other packages from extending them.
180+
181+
### Unit Tests
182+
183+
* Unit tests target Java 8, so language features such as lambda and streams can be used in tests.
184+
185+
## Specific tasks
186+
187+
### Updating the Snapshot build number
188+
189+
The overall version number for opentelemetry-java is determined from git tags, and not fixed in any
190+
file.
191+
192+
This means it will not update, even if you `git pull` from the repo tip. It will still produce a set
193+
of libraries with the old version number.
194+
195+
To update it, you must fetch the tags, via `git fetch --all --tags` - which should work, even if you
196+
have forked the repo, as long as the trunk repo is set as an upstream remote.
197+
198+
### Composing builds
199+
200+
Beware that this section is only meant for developers of opentelemetry-java, or closely related
201+
projects. The steps described here could change at any time and what you do for one version (commit)
202+
may break with the next one already.
203+
204+
Gradle provides a feature
205+
called ["composite builds"](https://docs.gradle.org/current/userguide/composite_builds.html)
206+
that allows to replace some normally externally provided dependencies with a project that is built
207+
(included) in the same Gradle invocation. This can be useful to quickly test a new feature or bug
208+
fix you are developing in opentelemetry-java with the examples or the app or instrumentation library
209+
where you need the feature or run into the bug. Unfortunately, opentelemetry-java does not work out
210+
of the box with this feature because Gradle is unable to map the project names to the customized
211+
artifact coordinates (see e.g. [gradle/gradle#18291](https://github.com/gradle/gradle/issues/18291)
212+
and related issues. However, gradle supports manually declaring the mapping between ("substitution
213+
of")
214+
artifact coordinates and project names. To ease this tedious task, opentelemetry-java provides a
215+
gradle task `:generateBuildSubstitutions` that generates a code snippet with these substitutions in
216+
kts (Kotlin Script) format.
217+
218+
Example usage could be as follows:
219+
220+
1. Run `./gradlew generateBuildSubstitutions`
221+
2. Two files named `build/substitutions.gradle.kts` are generated in the bom and bom-alpha project's
222+
directory, containing substitutions for the stable and alpha projects respectively.
223+
3. Copy & paste the content of these files to a new `settings.gradle.kts` or the one where you want
224+
to include the opentelemetry build into, so that it contains something like the following:
225+
226+
```kotlin
227+
includeBuild("PATH/TO/OPENTELEMETRY-JAVA/ROOT/DIRECTORY") {
228+
// Copy & paste following block from the generated substitutions.gradle.kts, *not* from here!
229+
dependencySubstitution {
230+
substitute(module("io.opentelemetry:opentelemetry-api")).using(project(":api:all"))
231+
substitute(module("io.opentelemetry:opentelemetry-sdk")).using(project(":sdk:all"))
232+
// ...
233+
}
234+
}
235+
```
236+
237+
Please confirm whether the local opentelemetry-java version is consistent with the
238+
opentelemetry-java version declared in the project that relies on opentelemetry-java.
239+
If it is inconsistent, `dependencySubstitution` may not take effect.
240+
241+
See [the Gradle documentation](https://docs.gradle.org/current/userguide/composite_builds.html#included_build_declaring_substitutions)
242+
for more information.
243+
4. If you now build your project, it will use the included build to supply the opentelemetry-java
244+
artifacts, ignoring any version declarations. Use the prefix `:DIRECTORY:` to refer to
245+
tasks/projects within the included build, where DIRECTORY is the name of the directory in the
246+
included build (only the part after the last `/`).
247+
5. Here are some issues and solutions ([discussions/6551](https://github.com/open-telemetry/opentelemetry-java/discussions/6551))
248+
you may encounter that may be helpful to you.
249+
250+
### Updating the OTLP protobufs
251+
252+
OTLP protobuf Java bindings are published via
253+
the [opentelemetry-proto-java](https://github.com/open-telemetry/opentelemetry-proto-java)
254+
repository. This project does not use the java bindings, but does use the `.proto` files that are
255+
published in the binding jar by that project.
256+
257+
To update the OTLP protobuf version,
258+
first [release a new version of the java bindings](https://github.com/open-telemetry/opentelemetry-proto-java/blob/main/RELEASING.md)
259+
then simply update the dependency version that this project has on that jar.

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);

0 commit comments

Comments
 (0)