Improve copilot-instructions.md for first-pass PR review (open-telemetry#18632)

Author: trask

.github/copilot-instructions.md

@@ -1,37 +1,173 @@
 # OpenTelemetry Java Instrumentation
 
-## Scope
+First-pass PR review rules. A deep review with full knowledge files runs
+separately later in the PR lifecycle. **Prefer silence over uncertainty.** Only
+flag substantive issues on changed lines. Skip stylistic preferences not listed
+below. Do not nitpick.
 
-Repository-wide defaults for Copilot behavior.
-Keep scoped language or file-type rules in `.github/instructions/*.instructions.md`.
+Use category tags like `[Style]`, `[Naming]`, `[Testing]`, `[General]`.
 
-## Repository Layout
+## [Style] Style Guide
 
-- Workspace-level overview: `.github/README.md`
-- Scoped instructions and `applyTo` patterns: `.github/instructions/README.md`
-- Agent review and implementation knowledge index: `.github/agents/knowledge/README.md`
+Follow `docs/contributing/style-guide.md`.
 
-## Core References
+- **Visibility**: principle of least access. Use the most restrictive modifier
+  that still works. Static fields should be `private` unless they are
+  constant-like with a `SCREAMING_SNAKE_CASE` name.
+- **`final` on classes**: declare public API classes `final` where possible. Do
+  **not** add `final` in `javaagent/src/main/`, in `.internal` packages, or in
+  test code (paths under `src/test/` or modules whose name starts/ends with
+  `testing` or `tests`).
+- **`final` on parameters and local variables**: never declare them `final`.
+- **Null comparisons**: use `value == null` / `value != null`, not
+  `null == value` / `null != value`. Applies to Java, Kotlin, and Scala.
+- **`equals` operand order**: prefer `value.equals(CONSTANT)` over
+  `CONSTANT.equals(value)`. Do not flip operand order solely as a defensive
+  null-safety cleanup; only flip when `value` can actually be null.
+- **Class organization**: static fields → static initializer → instance fields
+  → constructors → methods → nested classes. Place calling methods above the
+  methods they call.
+- **Static factory entry points**: place them below fields and immediately
+  above constructors — treat factories and constructors as one construction
+  section.
+- **Static utility classes**: place the private no-arg constructor after all
+  methods.
+- **Uppercase field names**: use `SCREAMING_SNAKE_CASE` only for constant-like
+  values — literals, immutable value constants (e.g. `Duration` timeouts),
+  semantic keys/handles (`AttributeKey`, `ContextKey`, `VirtualField`,
+  `MethodHandle`, `Pattern`), and canonical singletons (`INSTANCE`, `EMPTY`,
+  `NOOP`). Use lower camel case for runtime collaborators (loggers,
+  instrumenters, helpers, caches), even when `static final`.
+- **Avoid throwaway forwarding locals** that mirror an existing constant,
+  argument, or SDK field into both an SDK call and span attributes; pass the
+  original value directly unless real derivation justifies a local.
+- **`Optional`**: do not use in public API signatures or on the hot path.
+- **Semconv constants**: in `library/src/main/`, copy incubating semconv
+  constants locally as `private static final` with a `// copied from <Class>`
+  comment; do not depend on the semconv incubating artifact. In
+  `javaagent/src/main/` and tests, use semconv constants directly.
+- **`@Nullable` in tests**: do not add it to test code.
 
-- Style guide and core conventions: `docs/contributing/style-guide.md`
-- Review and implementation knowledge by topic: `.github/agents/knowledge/README.md`
+## [Style] `@SuppressWarnings` Scoping
 
-## Knowledge Loading
+Place `@SuppressWarnings` on the single member that needs it. Use class-level
+only when two or more members would need the same suppression.
 
-For coding, fix, and refactoring tasks, consult `.github/agents/knowledge/README.md`
-before making substantial changes.
+## [Naming] Catch Variable Names
 
-Use the knowledge index to load only the article(s) relevant to the current task.
-Do not load the entire knowledge folder by default.
+In **catch clauses only** (not method/lambda parameters or fields):
 
-## Gradle Execution Rules
+- Used exception → `e` (or `error` for a specific `*Error` subtype).
+- Used exception in nested catch where outer already uses `e` → `f`.
+- Used `Throwable` → `t`.
+- Intentionally unused → `ignored` (or `ignore` if `ignored` would shadow an
+  outer catch).
 
-Never use `--rerun-tasks`. Use `--rerun` when needed.
+## [Naming] Public API Getters
 
-Builds and tests in this repository can take several minutes.
-Run Gradle commands with timeout `0` (no timeout), and wait for completion.
-Do not treat slow output as a hang by default.
+Public API getters use `get*` (or `is*` for booleans).
 
-Never pipe Gradle output through `tail`, `head`, `grep`, or any other command.
-Piping masks the Gradle exit code — the shell reports the exit code of the last
-pipe segment, not Gradle, so a failing build silently appears to succeed.
+## [Style] No Redundant Null Guards on Attribute Puts
+
+`AttributesBuilder.put`, `Span.setAttribute`, `SpanBuilder.setAttribute`, and
+`LogRecordBuilder.setAttribute` are no-ops when the value is `null`. Do not wrap
+calls in `if (value != null)` when the value can be passed straight through:
+
+```java
+// BAD
+String v = getSomething();
+if (v != null) {
+  attributes.put(SOME_KEY, v);
+}
+// GOOD
+attributes.put(SOME_KEY, getSomething());
+```
+
+Do **not** flag when the guard protects a dereference or derived computation
+(e.g. `view.getClass().getName()`). When in doubt, stay silent.
+
+## [Testing] General Patterns
+
+- Use AssertJ (`assertThat(...)`) for assertions in new test code. Do not
+  use JUnit `Assert.*` or Hamcrest `assertThat`.
+- Do not add AssertJ `.as(...)` descriptions or `.withFailMessage(...)` in
+  tests. Direct assertions whose failure output already shows the unexpected
+  values are preferred.
+- Test methods do not need `throws Exception` clauses unless actually required.
+- Prefer the nearest common parent in `catch` (including `Exception` /
+  `Throwable`) over multi-catch.
+- Use `e` / `f` / `t` / `ignored` per the naming rules above.
+
+## [Testing] AssertJ Idiomatic Simplifications
+
+Prefer built-in AssertJ collection/string/map assertions over manual extraction:
+
+| Anti-pattern | Idiomatic |
+| --- | --- |
+| `assertThat(list.size()).isEqualTo(N)` | `assertThat(list).hasSize(N)` |
+| `assertThat(list.isEmpty()).isTrue()` / `.hasSize(0)` | `assertThat(list).isEmpty()` |
+| `assertThat(list.contains(x)).isTrue()` | `assertThat(list).contains(x)` |
+| per-index `get(i)` checks of every element | `assertThat(list).containsExactly(a, b, ...)` |
+
+`containsExactly` already verifies size, so a separate `hasSize` is redundant.
+Same shape applies to `String.length()`, `Map.size()`, and `array.length` →
+`assertThat(...).hasSize(N)`.
+
+## [Testing] Span Attribute Assertions
+
+- Prefer `hasAttributesSatisfyingExactly(...)` over `hasAttributesSatisfying(...)`
+  — the non-exact variant **silently ignores unexpected attributes**. Also
+  prefer it over `hasAttributes(...)` for consistency.
+- For zero-attribute span assertions, use `hasTotalAttributeCount(0)`.
+- `hasTotalAttributeCount(...)` paired with `hasAttributesSatisfyingExactly(...)`
+  is redundant — the exact variant already validates the count. Remove the
+  count call.
+- Metric points are different: there is no `hasTotalAttributeCount(...)` on
+  metric points, so use `point.hasAttributes(Attributes.empty())` for empty
+  metric-point checks.
+- Do not introduce redundant `(long)` casts in `equalTo(longKey(...), value)`
+  when `value` is already an `int` — the `equalTo(AttributeKey<Long>, int)`
+  overload exists.
+
+## [Testing] `satisfies()` Lambda Parameters
+
+Inside a `satisfies(AttributeKey, lambda)` attribute-assertion the lambda
+parameter is an `AbstractAssert` (e.g. `AbstractStringAssert<?>`), not the raw
+value. Fluent calls like `taskId.contains(jobName)` are already proper
+assertions — do **not** wrap them in `assertThat(value.contains(x)).isTrue()`,
+which degrades the failure message.
+
+Name the outer parameter `val` in Java (or `value` in Scala, where `val` is
+reserved). Use `v` only for a nested inner-lambda parameter.
+
+This guidance applies only to attribute-assertion `satisfies(...)`; for
+`span.satisfies(...)`, `point.satisfies(...)`, etc. use a descriptive name
+(`spanData`, `pointData`, `result`).
+
+## [Javaagent] Singleton Accessor Naming
+
+In `*Singletons`, `*SpanNaming`, and similar holder classes, zero-arg accessor
+methods that **directly return a stored singleton field** must match the field
+name with no `get` prefix:
+
+```java
+private static final Instrumenter<Request, Response> instrumenter = ...;
+
+public static Instrumenter<Request, Response> instrumenter() {
+  return instrumenter;
+}
+```
+
+- Methods that take arguments or compute a value are not singleton accessors —
+  keep their normal names (including `get*` when appropriate). Do not flag
+  `getAddressAndPort(client)` on this basis.
+- Uppercase constant-like fields (e.g. `VirtualField`, `ContextKey`) may be
+  exposed as `public static final` directly with no accessor.
+- Caller sites should static-import the accessor / constant and call it
+  unqualified.
+
+## [General] Engineering Correctness
+
+Flag real defects on changed lines: logic errors, concurrency hazards, resource
+leaks, copy/paste mistakes, incorrect comments, unsafe error handling, dead
+code, security regressions. Skip stylistic preferences not listed above.

AGENTS.md

@@ -2,3 +2,16 @@
 
 Read [CONTRIBUTING.md](CONTRIBUTING.md) first. It is the source of truth for repository layout,
 build and test commands, style expectations, and scope.
+
+## Knowledge Loading
+
+For coding, fix, and refactoring tasks, consult `.github/agents/knowledge/README.md`
+and load only the article(s) relevant to the current task.
+
+## Gradle Execution Rules
+
+- Never use `--rerun-tasks`. Use `--rerun` when needed.
+- Builds and tests can take several minutes. Run Gradle with timeout `0` and wait.
+  Slow output is not a hang.
+- Never pipe Gradle output through `tail`, `head`, `grep`, etc. Piping masks the
+  Gradle exit code.