Skip to content

Commit 984fd4f

Browse files
jack-bergtrask
andauthored
Rework CONTRIBUTING.md, extracting bits to new /docs/knowledge/* for … (#8303)
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
1 parent 6336abd commit 984fd4f

10 files changed

Lines changed: 610 additions & 277 deletions

File tree

AGENTS.md

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
# AGENTS.md
22

3-
Read [CONTRIBUTING.md](CONTRIBUTING.md) first. It is the source of truth for repository layout,
4-
build and test commands, style expectations, and scope.
3+
Start with [docs/knowledge/README.md](docs/knowledge/README.md). It is the entry point for
4+
coding patterns, build conventions, testing guidance, and API stability rules. Load documents
5+
based on the scope of your work — the README contains a table mapping topics to load conditions.
56

6-
Additional guidance for agents:
7+
For project scope, PR process, and CLA requirements, see [CONTRIBUTING.md](CONTRIBUTING.md).
8+
9+
## Additional guidance
710

811
* Prefer small, localized changes over broad refactors.

CONTRIBUTING.md

Lines changed: 35 additions & 253 deletions
Large diffs are not rendered by default.

docs/jmh.md

Lines changed: 0 additions & 21 deletions
This file was deleted.

docs/knowledge/README.md

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
# Knowledge Index
2+
3+
Repository guidance for coding and review, written to be useful for both humans and machines.
4+
5+
**For humans**: these documents use plain language and examples rather than rigid rules. Where
6+
something is a strong convention, it's described as such. Where something is a matter of
7+
judgment, it's described that way too.
8+
9+
**For machines**: load only files relevant to the current scope — the "Load when" column below
10+
is the signal.
11+
12+
## Topics
13+
14+
| File | Load when |
15+
| --- | --- |
16+
| [build.md](build.md) | Always — build requirements and common tasks |
17+
| [general-patterns.md](general-patterns.md) | Always — style, nullability, visibility, AutoValue, locking, logging |
18+
| [api-stability.md](api-stability.md) | Public API additions, removals, renames, or deprecations; stable vs alpha compatibility |
19+
| [gradle-conventions.md](gradle-conventions.md) | `build.gradle.kts` or `settings.gradle.kts` changes; new modules |
20+
| [testing-patterns.md](testing-patterns.md) | Test files in scope — assertions, test utilities, test suites |
21+
| [other-tasks.md](other-tasks.md) | Dev environment setup, benchmarks, composite builds, OTLP protobuf updates |
22+
23+
## Conventions
24+
25+
- File names are kebab-cased and topic-oriented. Most follow a `<domain>-<focus>.md` pattern
26+
(e.g. `api-stability.md`, `testing-patterns.md`).
27+
- Sections within each document are ordered alphabetically, with the exception of any
28+
introductory content placed directly under the document title.

docs/knowledge/api-stability.md

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
# API Stability and Breaking Changes
2+
3+
See [VERSIONING.md](../../VERSIONING.md) for the full versioning and compatibility policy.
4+
5+
## Breaking changes in alpha modules
6+
7+
Breaking changes are allowed in alpha modules but should still be approached carefully:
8+
9+
- Prefer going through a deprecation cycle to ease migration, unless the deprecation would be too
10+
cumbersome or the old API is genuinely unusable.
11+
- Bundle breaking changes together where possible to reduce churn for consumers.
12+
- Deprecations in alpha modules are typically introduced and removed within one release cycle
13+
(approximately one month).
14+
15+
## Breaking changes in stable modules
16+
17+
Breaking changes are not allowed in stable modules outside of a major version bump. The build
18+
enforces this via japicmp — see [japicmp](#japicmp) below.
19+
20+
## Deprecating API
21+
22+
Applies to both stable and alpha modules. Use plain `@Deprecated` — do **not** use
23+
`forRemoval = true` or `since = "..."` (Java 8 compatibility requirement).
24+
25+
```java
26+
/**
27+
* @deprecated Use {@link #newMethod()} instead.
28+
*/
29+
@Deprecated
30+
public ReturnType oldMethod() {
31+
return newMethod(); // delegate to replacement
32+
}
33+
```
34+
35+
Rules:
36+
- Include a `@deprecated` Javadoc tag naming the replacement.
37+
- The deprecated method must delegate to its replacement, not the other way around — this ensures
38+
overriders of the old method still get called.
39+
- Deprecated items in stable modules cannot be removed until the next major version.
40+
- Deprecated items in alpha modules should indicate when they will be removed (e.g.
41+
`// to be removed in 1.X.0`). It is also common to log a warning when a deprecated-for-removal
42+
feature is used, to increase visibility for consumers.
43+
- Add the `deprecation` label to the PR.
44+
45+
## japicmp
46+
47+
`otel.japicmp-conventions` runs the `jApiCmp` task as part of `check` for every published module.
48+
It compares the locally-built jar against the latest release to detect breaking changes, and writes
49+
a human-readable diff to `docs/apidiffs/current_vs_latest/<artifact>.txt`.
50+
51+
- Breaking changes in stable modules fail the build.
52+
- AutoValue classes are exempt from the abstract-method-added check — the generated implementation
53+
handles those automatically.
54+
- The diff files are committed to the repo. Include any changes to them in your PR.
55+
- Run `./gradlew jApiCmp` to regenerate diffs after API changes.
56+
57+
## Stable vs alpha modules
58+
59+
Artifacts without an `-alpha` version suffix are **stable**. Artifacts with `-alpha` have no
60+
compatibility guarantees and may break on every release. The `internal` package is always exempt
61+
from compatibility guarantees regardless of artifact stability.
62+
63+
To mark a module as alpha, add a `gradle.properties` file at the module root containing:
64+
65+
```properties
66+
otel.release=alpha
67+
```
68+
69+
See [`exporters/prometheus/gradle.properties`](../../exporters/prometheus/gradle.properties) for
70+
an example.
71+
72+
See [VERSIONING.md](../../VERSIONING.md) for the full compatibility policy, including what
73+
constitutes a source-incompatible vs binary-incompatible change.

docs/knowledge/build.md

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
# Build
2+
3+
Java 21 or higher is required to build. The built artifacts target Java 8 or higher.
4+
5+
## Common tasks
6+
7+
```bash
8+
# Fix formatting violations
9+
./gradlew spotlessApply
10+
11+
# Run checks only (tests + static analysis + formatting verification)
12+
./gradlew check
13+
14+
# Build, run tests, static analysis, and check formatting
15+
./gradlew build
16+
```
17+
18+
All tasks can be scoped to a single module by prefixing with the module path:
19+
20+
```bash
21+
./gradlew :sdk:metrics:spotlessApply
22+
./gradlew :sdk:metrics:check
23+
./gradlew :sdk:metrics:build
24+
```
25+
26+
`./gradlew build` and `./gradlew check` both depend on the `jApiCmp` task, which compares the
27+
locally-built jars against the latest release and writes diffs to `docs/apidiffs/current_vs_latest/`.
28+
Include any changes to those files in your PR. See [api-stability.md](api-stability.md#japicmp)
29+
for details.
30+
31+
If your branch is not up to date with `main`, `jApiCmp` may produce a diff that reflects changes
32+
already merged to `main` rather than your own changes. Rebase or merge `main` before treating
33+
the diff as meaningful.
34+
35+
For dev environment setup, composite builds, OTLP protobuf updates, and other tasks, see
36+
[other-tasks.md](other-tasks.md).

docs/knowledge/general-patterns.md

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
# General Patterns
2+
3+
## @Nullable
4+
5+
All arguments and members are treated as non-null by default. Annotate with `@Nullable` (from
6+
`javax.annotation`) only when `null` is actually possible.
7+
8+
- **Fields**: annotate only if the field can hold `null` after construction.
9+
- **Parameters**: annotate only if `null` is actually passed by callers.
10+
- **Return types**: annotate only if the method actually returns `null`. A non-null implementation
11+
of a `@Nullable`-declared interface method should omit the annotation — it is more precise.
12+
13+
## API consistency
14+
15+
The project aims to provide a consistent experience across all public APIs. When designing new
16+
API, look for prior art in the project before introducing new patterns — prefer the style already
17+
established in the same package, then the same module, then the broader project.
18+
19+
## AutoValue
20+
21+
Use [AutoValue](https://github.com/google/auto/tree/master/value) for new value classes.
22+
23+
- Add `annotationProcessor("com.google.auto.value:auto-value")` in `build.gradle.kts`.
24+
- `auto-value-annotations` is already available via `otel.java-conventions`.
25+
- Add a package-private constructor to all AutoValue classes to prevent external extension.
26+
- `japicmp` allows new abstract methods on AutoValue classes — the `AllowNewAbstractMethodOnAutovalueClasses` rule handles this.
27+
28+
## Class member ordering
29+
30+
In general, order class members as follows:
31+
32+
1. Static fields (final before non-final)
33+
2. Instance fields (final before non-final)
34+
3. Constructors
35+
- In static utility classes, the private constructor goes after methods, not before.
36+
4. Methods
37+
- If methods call each other, the calling method should appear above the method it calls.
38+
5. Nested classes
39+
40+
## Dedicated lock objects
41+
42+
Do not synchronize using a class's intrinsic lock (`synchronized(this)` or `synchronized` method).
43+
Use a dedicated lock object:
44+
45+
```java
46+
private final Object lock = new Object();
47+
48+
public void doSomething() {
49+
synchronized (lock) { ... }
50+
}
51+
```
52+
53+
## Formatting
54+
55+
Formatting is enforced by three tools. Run `./gradlew spotlessApply` before committing — it fixes
56+
most violations automatically.
57+
58+
- **Spotless** — formatting rules vary by file type (see `buildSrc/src/main/kotlin/otel.spotless-conventions.gradle.kts`):
59+
Java uses google-java-format; Kotlin uses ktlint. Also enforces the Apache license header
60+
(template in `buildscripts/spotless.license.java`) and misc file hygiene (trailing whitespace,
61+
final newline, etc.).
62+
- **Checkstyle** — enforces naming conventions, import ordering, Javadoc structure, and other
63+
rules not covered by formatting alone. Config is in `buildscripts/checkstyle.xml`.
64+
- **EditorConfig** (`.editorconfig`) — configures IntelliJ to match project style automatically.
65+
It doesn't cover all rules, so `spotlessApply` is still required.
66+
67+
## Internal code
68+
69+
Prefer package-private over putting code in an `internal` package. Use `internal` only when the
70+
code must be `public` for technical reasons (e.g. accessed across packages within the same module)
71+
but should not be part of the public API.
72+
73+
Public classes in `internal` packages are excluded from semver guarantees and Javadoc, but they
74+
must carry one of the two standard disclaimers (enforced by the `OtelInternalJavadoc` Error Prone
75+
check in `custom-checks/`):
76+
77+
```java
78+
// Standard internal disclaimer:
79+
/**
80+
* This class is internal and is hence not for public use. Its APIs are unstable and can change
81+
* at any time.
82+
*/
83+
84+
// For incubating internal code that may be promoted to public API:
85+
/**
86+
* This class is internal and experimental. Its APIs are unstable and can change at any time.
87+
* Its APIs (or a version of them) may be promoted to the public stable API in the future, but
88+
* no guarantees are made.
89+
*/
90+
```
91+
92+
Internal code must not be used across module boundaries — module `foo` must not call internal
93+
code from module `bar`. Cross-module internal usage is a known issue being tracked and cleaned up
94+
in open-telemetry/opentelemetry-java#6970.
95+
96+
## Javadoc
97+
98+
All public classes, and their public and protected methods, must have complete Javadoc including
99+
all parameters — this is enforced for public APIs. Package-private and private members may have
100+
Javadoc at the author's discretion.
101+
102+
- No `@author` tags.
103+
- New public API elements require a `@since` annotation. This is added automatically during the
104+
[release process](../../RELEASING.md) — do not include it in your PR.
105+
- See [section 7.3.1](https://google.github.io/styleguide/javaguide.html#s7.3.1-javadoc-exception-self-explanatory)
106+
for self-explanatory exceptions.
107+
108+
Published Javadoc is available at https://javadoc.io/doc/io.opentelemetry.
109+
110+
## Language version compatibility
111+
112+
Production code targets Java 8. Test code also targets Java 8. Do not use Java 9+ APIs unless
113+
the module explicitly sets a higher minimum. See [VERSIONING.md](../../VERSIONING.md) for the full
114+
language version compatibility policy, including Android and Kotlin minimum versions.
115+
116+
## Logging
117+
118+
Use `java.util.logging` (JUL) in production source sets. Do not use SLF4J or other logging
119+
frameworks in `src/main/`. Tests bridge JUL to SLF4J via `JulBridgeInitializer` (configured
120+
automatically by `otel.java-conventions`).
121+
122+
```java
123+
private static final Logger LOGGER = Logger.getLogger(MyClass.class.getName());
124+
```
125+
126+
## toString()
127+
128+
Adding `toString()` overrides is encouraged for debugging assistance. All `toString()`
129+
implementations should be considered unstable unless explicitly documented otherwise — do not
130+
rely on their output programmatically.
131+
132+
## Visibility
133+
134+
Use the most restrictive access modifier that still allows the code to function correctly. Use
135+
`final` on classes unless extension is explicitly intended.
Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
# Gradle Conventions
2+
3+
## AutoValue in build files
4+
5+
See [general-patterns.md](general-patterns.md#autovalue) for the AutoValue usage pattern.
6+
`auto-value-annotations` is provided globally by `otel.java-conventions`. Only add the
7+
annotation processor if the module uses AutoValue in production code:
8+
9+
```kotlin
10+
dependencies {
11+
annotationProcessor("com.google.auto.value:auto-value") // production
12+
testAnnotationProcessor("com.google.auto.value:auto-value") // tests only
13+
}
14+
```
15+
16+
## Convention plugins
17+
18+
Every module applies a base set of convention plugins from `buildSrc/src/main/kotlin/`.
19+
20+
| Plugin | Purpose | Who applies it |
21+
| --- | --- | --- |
22+
| `otel.java-conventions` | Base Java toolchain, Checkstyle, Spotless, Error Prone, test config | All modules |
23+
| `otel.publish-conventions` | Maven publishing, POM generation | Published (non-internal) modules |
24+
| `otel.animalsniffer-conventions` | Android API level compatibility checking | Modules targeting Android |
25+
| `otel.jmh-conventions` | JMH benchmark support | Modules with benchmarks |
26+
| `otel.japicmp-conventions` | API diff generation against latest release | Published modules (applied by `otel.publish-conventions`) |
27+
| `otel.protobuf-conventions` | Protobuf code generation | Protobuf modules only |
28+
29+
A typical published module:
30+
31+
```kotlin
32+
plugins {
33+
id("otel.java-conventions")
34+
id("otel.publish-conventions")
35+
}
36+
```
37+
38+
A testing-internal module (shared test utilities, not published):
39+
40+
```kotlin
41+
plugins {
42+
id("otel.java-conventions")
43+
// no otel.publish-conventions
44+
}
45+
```
46+
47+
## Dependency resolution
48+
49+
All external dependency versions are managed by `:dependencyManagement` (a BOM). Do not
50+
specify versions directly in `build.gradle.kts` — add new entries to the BOM if a version
51+
needs to be pinned.
52+
53+
`otel.java-conventions` configures `failOnVersionConflict()` and `preferProjectModules()`
54+
globally. Do not override these without a strong reason.
55+
56+
## Module naming
57+
58+
Every module must declare its Java module name:
59+
60+
```kotlin
61+
otelJava.moduleName.set("io.opentelemetry.sdk.metrics")
62+
```
63+
64+
When the archive name cannot be derived correctly from the project name, set it explicitly:
65+
66+
```kotlin
67+
base.archivesName.set("opentelemetry-api")
68+
```
69+
70+
## `settings.gradle.kts` ordering
71+
72+
New `include(...)` entries must be in **alphabetical order** within their surrounding group. The
73+
file is large — find the right lexicographic position before inserting.
74+
75+
## Shared test utilities and test suites
76+
77+
See [testing-patterns.md](testing-patterns.md) for shared test utility patterns and test suite
78+
registration.

0 commit comments

Comments
 (0)