Skip to content

Commit aa592d2

Browse files
committed
Code review agent: document more *Singletons patterns
1 parent 7b1e3da commit aa592d2

4 files changed

Lines changed: 80 additions & 18 deletions

File tree

.github/agents/knowledge/README.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,8 @@ Load only files relevant to the current scope to reduce noise and avoid over-con
1313
| `general-rules.md` | Always — review checklist table and core rules enforced on every review |
1414
| `gradle-conventions.md` | `build.gradle.kts` or `settings.gradle.kts` changes, custom test task registration or wiring |
1515
| `javaagent-advice-patterns.md` | ByteBuddy `@Advice` class or advice-method changes |
16-
| `javaagent-module-patterns.md` | `InstrumentationModule`, `TypeInstrumentation`, `Singletons`, `VirtualField`, `CallDepth` |
16+
| `javaagent-module-patterns.md` | `InstrumentationModule`, `TypeInstrumentation`, `VirtualField`, `CallDepth` |
17+
| `javaagent-singletons-patterns.md` | `*Singletons` holder classes, singleton accessors, static-imported singleton callers |
1718
| `library-patterns.md` | Library instrumentation telemetry, builder, getter, or setter pattern changes |
1819
| `module-naming.md` | New or renamed modules or packages; settings includes |
1920
| `testing-general-patterns.md` | Test files in scope — assertion style, attribute assertion patterns, `satisfies()` lambda usage |

.github/agents/knowledge/general-rules.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@ When a "Knowledge File" is listed, load it from `knowledge/` before reviewing th
1919
| Naming | Getter naming (`get` / `is`) | Always ||
2020
| Naming | Module/package naming | New or renamed modules/packages | `module-naming.md` |
2121
| Javaagent | Advice patterns | `@Advice` classes | `javaagent-advice-patterns.md` |
22-
| Javaagent | Module structure patterns | `InstrumentationModule`, `TypeInstrumentation`, `Singletons` | `javaagent-module-patterns.md` |
22+
| Javaagent | Module structure patterns | `InstrumentationModule`, `TypeInstrumentation` | `javaagent-module-patterns.md` |
23+
| Javaagent | Singletons patterns | `*Singletons` holder classes, singleton accessors, static-imported singleton callers | `javaagent-singletons-patterns.md` |
2324
| Javaagent | Incorrect `classLoaderMatcher()` | `classLoaderMatcher()` override that is redundant (muzzle already handles it) or missing when needed (muzzle cannot distinguish version range) | `javaagent-module-patterns.md` |
2425
| Semconv | Library vs javaagent semconv constant usage | Semconv constants/assertions ||
2526
| Semconv | Dual semconv testing | `SemconvStability`, `maybeStable`, semconv Gradle tasks | `testing-semconv-stability.md` |

.github/agents/knowledge/javaagent-module-patterns.md

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@
22

33
## Quick Reference
44

5-
- Use when: reviewing `InstrumentationModule`, `TypeInstrumentation`, `Singletons`, `VirtualField`, or `CallDepth` code
6-
- Review focus: registration and naming, matcher performance, safe advice wiring, singleton and hot-path patterns
5+
- Use when: reviewing `InstrumentationModule`, `TypeInstrumentation`, `VirtualField`, or `CallDepth` code
6+
- Review focus: registration and naming, matcher performance, safe advice wiring
77

88
## InstrumentationModule
99

@@ -318,20 +318,6 @@ sufficient for optimization.
318318
instance, not a class literal. Omit the redundant `this.` qualifier and use the shorter
319319
repository convention.
320320

321-
## Singletons Pattern
322-
323-
Javaagent modules hold their `Instrumenter` instances and shared resources in a dedicated
324-
`Singletons` holder class (e.g., `MyLibrarySingletons`).
325-
326-
### Rules
327-
328-
- `Instrumenter` instances must be initialized at class-load time — either as a `static final`
329-
field initializer or in a `static {}` block.
330-
- Use `GlobalOpenTelemetry.get()` to obtain the `OpenTelemetry` instance.
331-
- The instrumentation name string (second argument to `builder()`) should match the Gradle
332-
module path: `"io.opentelemetry.<module-name>"` (e.g., `"io.opentelemetry.jedis-4.0"`).
333-
- Provide a static accessor method (typically named `instrumenter()`).
334-
335321
## CallDepth (Preventing Recursive Instrumentation)
336322

337323
When an instrumented method may call itself through the instrumented library, use `CallDepth`
Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
# [Javaagent] Singletons Patterns
2+
3+
## Quick Reference
4+
5+
- Use when: reviewing `*Singletons` holder classes and their callers
6+
- Review focus: private fields, accessor naming, eager initialization, static-import call sites
7+
8+
Javaagent modules keep shared `Instrumenter` instances and related collaborators in a dedicated
9+
`Singletons` holder class such as `MyLibrarySingletons`.
10+
11+
## Rules
12+
13+
- Keep exported singleton-holder fields `private`. Do not expose package-private or public
14+
`static` fields from `*Singletons` classes.
15+
- Initialize shared collaborators at class-load time, either with `static final` field
16+
initializers or in a `static {}` block.
17+
- Use `GlobalOpenTelemetry.get()` to obtain the `OpenTelemetry` instance.
18+
- The instrumentation name string should match the Gradle module path:
19+
`"io.opentelemetry.<module-name>"`.
20+
- For exported collaborators, use lower camel case field names and give the accessor method the
21+
exact same name as the field. Do not prefix these accessors with `get`.
22+
- `instrumenter` -> `instrumenter()`
23+
- `helper` -> `helper()`
24+
- `setter` -> `setter()`
25+
- Callers should static import singleton accessor methods and invoke them unqualified.
26+
- Keep verb-named helper methods as verbs when they perform work instead of returning a stored
27+
field. This naming rule applies to field accessors.
28+
29+
## Preferred Pattern
30+
31+
```java
32+
public final class MyLibrarySingletons {
33+
34+
private static final Instrumenter<Request, Response> instrumenter =
35+
JavaagentHttpServerInstrumenters.create(...);
36+
37+
private static final Helper helper = new Helper();
38+
39+
public static Instrumenter<Request, Response> instrumenter() {
40+
return instrumenter;
41+
}
42+
43+
public static Helper helper() {
44+
return helper;
45+
}
46+
47+
private MyLibrarySingletons() {}
48+
}
49+
```
50+
51+
Caller:
52+
53+
```java
54+
import static io.opentelemetry.javaagent.instrumentation.example.MyLibrarySingletons.helper;
55+
import static io.opentelemetry.javaagent.instrumentation.example.MyLibrarySingletons.instrumenter;
56+
57+
class MyInstrumentation implements TypeInstrumentation {
58+
void doSomething(Request request) {
59+
if (instrumenter().shouldStart(parentContext, request)) {
60+
helper().beforeStart(request);
61+
}
62+
}
63+
}
64+
```
65+
66+
## What to Flag in Review
67+
68+
- Exposed singleton-holder fields such as `public static final Instrumenter ...`.
69+
- Accessor methods named `getInstrumenter()`, `getHelper()`, `getSetter()`, and similar when they
70+
simply return a backing field.
71+
- Call sites that qualify singleton field-accessor calls with the holder class instead of static
72+
importing the accessor method.
73+
- Mismatches between a field name and its accessor, such as `private static final Helper helper;`
74+
with `public static Helper getHelper()`.

0 commit comments

Comments
 (0)