|
| 1 | +# Style Guide |
| 2 | + |
| 3 | +This project follows the |
| 4 | +[Google Java Style Guide](https://google.github.io/styleguide/javaguide.html). |
| 5 | + |
| 6 | +## Code Formatting |
| 7 | + |
| 8 | +### Auto-formatting |
| 9 | + |
| 10 | +The build will fail if source code is not formatted according to Google Java Style. |
| 11 | + |
| 12 | +Run the following command to reformat all files: |
| 13 | + |
| 14 | +```bash |
| 15 | +./gradlew spotlessApply |
| 16 | +``` |
| 17 | + |
| 18 | +For IntelliJ users, an `.editorconfig` file is provided that IntelliJ will automatically use to |
| 19 | +adjust code formatting settings. However, it does not support all required rules, so you may still |
| 20 | +need to run `./gradlew spotlessApply` periodically. |
| 21 | + |
| 22 | +### Static imports |
| 23 | + |
| 24 | +Consider statically importing the following commonly used methods and constants: |
| 25 | + |
| 26 | +- **Test methods** |
| 27 | + - `io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.*` |
| 28 | + - `org.assertj.core.api.Assertions.*` |
| 29 | + - `org.mockito.Mockito.*` |
| 30 | + - `org.mockito.ArgumentMatchers.*` |
| 31 | +- **Utility methods** |
| 32 | + - `io.opentelemetry.api.common.AttributeKey.*` |
| 33 | + - `java.util.Arrays` - asList, stream |
| 34 | + - `java.util.Collections` - singleton*, empty*, unmodifiable*, synchronized*, checked* |
| 35 | + - `java.util.Objects` - requireNonNull |
| 36 | + - `java.util.function.Function` - identity |
| 37 | + - `java.util.stream.Collectors.*` |
| 38 | +- **Utility constants** |
| 39 | + - `java.util.Locale.*` |
| 40 | + - `java.util.concurrent.TimeUnit.*` |
| 41 | + - `java.util.logging.Level.*` |
| 42 | + - `java.nio.charset.StandardCharsets.*` |
| 43 | +- **OpenTelemetry semantic convention constants** |
| 44 | + - All constants under `io.opentelemetry.semconv.**`, except for |
| 45 | + `io.opentelemetry.semconv.SchemaUrls.*` constants. |
| 46 | + |
| 47 | +### Class organization |
| 48 | + |
| 49 | +Prefer this order: |
| 50 | + |
| 51 | +- Static fields (final before non-final) |
| 52 | +- Instance fields (final before non-final) |
| 53 | +- Constructors |
| 54 | +- Methods |
| 55 | +- Nested classes |
| 56 | + |
| 57 | +**Method ordering**: Place calling methods above the methods they call. For example, place private |
| 58 | +methods below the non-private methods that use them. |
| 59 | + |
| 60 | +**Static utility classes**: Place the private constructor (used to prevent instantiation) after all |
| 61 | +methods. |
| 62 | + |
| 63 | +## Java Language Conventions |
| 64 | + |
| 65 | +### Visibility modifiers |
| 66 | + |
| 67 | +Follow the principle of minimal necessary visibility. Use the most restrictive access modifier that |
| 68 | +still allows the code to function correctly. |
| 69 | + |
| 70 | +### Internal packages |
| 71 | + |
| 72 | +Classes in `.internal` packages are not considered public API and may change without notice. These |
| 73 | +packages contain implementation details that should not be used by external consumers. |
| 74 | + |
| 75 | +- Use `.internal` packages for implementation classes that need to be public within the module but |
| 76 | + should not be used externally |
| 77 | +- Try to avoid referencing `.internal` classes from other modules |
| 78 | + |
| 79 | +### `final` keyword usage |
| 80 | + |
| 81 | +Public non-internal classes should be declared `final` where possible. Internal and non-public |
| 82 | +classes should not be declared `final`. |
| 83 | + |
| 84 | +Methods should only be declared `final` if they are in public non-internal non-final classes. |
| 85 | + |
| 86 | +Fields should be declared `final` where possible. |
| 87 | + |
| 88 | +Method parameters and local variables should never be declared `final`. |
| 89 | + |
| 90 | +### `@Nullable` annotation usage |
| 91 | + |
| 92 | +**Note: This section is aspirational and may not reflect the current codebase.** |
| 93 | + |
| 94 | +Annotate all parameters and fields that can be `null` with `@Nullable` (specifically |
| 95 | +`javax.annotation.Nullable`, which is included by the `otel.java-conventions` Gradle plugin as a |
| 96 | +`compileOnly` dependency). |
| 97 | + |
| 98 | +`@NonNull` is unnecessary as it is the default. |
| 99 | + |
| 100 | +**Defensive programming**: Public APIs should still check for `null` parameters even if not |
| 101 | +annotated with `@Nullable`. Internal APIs do not need these checks. |
| 102 | + |
| 103 | +### `Optional` usage |
| 104 | + |
| 105 | +Following the reasoning from |
| 106 | +[Writing a Java library with better experience (slide 12)](https://speakerdeck.com/trustin/writing-a-java-library-with-better-experience?slide=12), |
| 107 | +`java.util.Optional` usage is kept to a minimum. |
| 108 | + |
| 109 | +**Guidelines**: |
| 110 | + |
| 111 | +- `Optional` shouldn't appear in public API signatures |
| 112 | +- Avoid `Optional` on the hot path (instrumentation code), unless the instrumented library uses it |
| 113 | + |
| 114 | +## Tooling conventions |
| 115 | + |
| 116 | +### AssertJ |
| 117 | + |
| 118 | +Prefer AssertJ assertions over JUnit assertions (assertEquals, assertTrue, etc.) for better error |
| 119 | +messages. |
| 120 | + |
| 121 | +### JUnit |
| 122 | + |
| 123 | +Test classes and test methods should generally be package-protected (no explicit visibility |
| 124 | +modifier) rather than `public`. This follows the principle of minimal necessary visibility and is |
| 125 | +sufficient for JUnit to discover and execute tests. |
| 126 | + |
| 127 | +### AutoService |
| 128 | + |
| 129 | +Use the `@AutoService` annotation when implementing SPI interfaces. This automatically generates the |
| 130 | +necessary `META-INF/services/` files at compile time, eliminating the need to manually create and |
| 131 | +maintain service registration files. |
| 132 | + |
| 133 | +```java |
| 134 | +@AutoService(AutoConfigurationCustomizerProvider.class) |
| 135 | +public class MyCustomizerProvider implements AutoConfigurationCustomizerProvider { |
| 136 | + // implementation |
| 137 | +} |
| 138 | +``` |
| 139 | + |
| 140 | +### Gradle |
| 141 | + |
| 142 | +- Use Kotlin instead of Groovy for build scripts |
| 143 | +- Plugin versions should be specified in `settings.gradle.kts`, not in individual modules |
| 144 | +- All modules should use `plugins { id("otel.java-conventions") }` |
| 145 | +- Set module names with `otelJava.moduleName.set("io.opentelemetry.contrib.mymodule")` |
| 146 | + |
| 147 | +## Configuration |
| 148 | + |
| 149 | +- Use `otel.` prefix for all configuration property keys |
| 150 | +- Read configuration via the `ConfigProperties` interface |
| 151 | +- Provide sensible defaults and document all options |
| 152 | +- Validate configuration early with clear error messages |
| 153 | + |
| 154 | +## Performance |
| 155 | + |
| 156 | +Avoid allocations on the hot path (instrumentation code) whenever possible. This includes `Iterator` |
| 157 | +allocations from collections; note that `for (SomeType t : plainJavaArray)` does not allocate an |
| 158 | +iterator object. |
| 159 | + |
| 160 | +Non-allocating Stream API usage on the hot path is acceptable but may not fit the surrounding code |
| 161 | +style; this is a judgment call. Some Stream APIs make efficient allocation difficult (e.g., |
| 162 | +`collect` with pre-sized sink data structures involves convoluted `Supplier` code, or lambdas passed |
| 163 | +to `forEach` may be capturing/allocating lambdas). |
| 164 | + |
| 165 | +## Documentation |
| 166 | + |
| 167 | +### Component README files |
| 168 | + |
| 169 | +- Include a component owners section in each module's README |
| 170 | +- Document configuration options with examples |
| 171 | + |
| 172 | +### Deprecation and breaking changes |
| 173 | + |
| 174 | +Breaking changes are allowed in unstable modules (published with `-alpha` version suffix). |
| 175 | + |
| 176 | +1. Mark APIs with `@Deprecated` and a removal timeline (there must be at least one release with the |
| 177 | + API marked as deprecated before removing it) |
| 178 | +2. Document the replacement in Javadoc with `@deprecated` tag |
| 179 | +3. Note the migration path for breaking changes under a "Migration notes" section of CHANGELOG.md |
| 180 | + (create this section at the top of the Unreleased section if not already present) |
0 commit comments