Skip to content

Commit c189c0d

Browse files
committed
Code review guide: instrumentation module renames
1 parent fe4688b commit c189c0d

3 files changed

Lines changed: 95 additions & 3 deletions

File tree

.github/agents/knowledge/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ Load only files relevant to the current scope to reduce noise and avoid over-con
88

99
| File | Load when |
1010
| --- | --- |
11-
| `api-deprecation-policy.md` | Public API removal, rename, or deprecation; stable vs alpha breaking changes |
11+
| `api-deprecation-policy.md` | Public API removal, rename, or deprecation; stable vs alpha breaking changes; module renames that touch config keys or emitted scope names |
1212
| `config-property-stability.md` | `otel.instrumentation.*` property add, remove, rename, or deprecation |
1313
| `general-rules.md` | Always — review checklist table and core rules enforced on every review |
1414
| `metadata-yaml-format.md` | Always — mandatory review of metadata.yaml for config coverage |

.github/agents/knowledge/api-deprecation-policy.md

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

33
## Quick Reference
44

5-
- Use when: reviewing public API removals/renames, `@Deprecated` usage, stable-vs-alpha compatibility
6-
- Review focus: deprecate-then-remove timing, delegation direction, required Javadoc/CHANGELOG coverage
5+
- Use when: reviewing public API removals/renames, `@Deprecated` usage, stable-vs-alpha compatibility, or any module rename that touches user-facing config keys or emitted telemetry identity
6+
- Review focus: deprecate-then-remove timing, delegation direction, required Javadoc/CHANGELOG coverage, v3-preview gating for config keys and scope names
7+
8+
## What Counts as "Public API"
9+
10+
"API" here means **anything a user's code or configuration depends on by name**, including:
11+
12+
- Java symbols in published artifacts (classes, methods, fields in `:library`, `:testing`,
13+
`instrumentation-api*`).
14+
- User-facing configuration keys — `otel.instrumentation.<name>.enabled`, any
15+
`otel.instrumentation.*` property, and the equivalent declarative YAML keys.
16+
- Outgoing telemetry identity — anything users can match on in their backend, including
17+
`otel.scope.name`, span names, metric names, attribute keys, and attribute values. Users
18+
build dashboards, filters, and alerts on these values, so silently renaming them is a
19+
breaking change.
20+
21+
A rename of any of these surfaces is a breaking change even if no Java symbol moved.
722

823
## When Are Breaking Changes Allowed?
924

@@ -75,6 +90,71 @@ default void configure(IgnoredTypesBuilder builder, ConfigProperties config) {
7590
}
7691
```
7792

93+
## Module renames: config keys and emitted scope names
94+
95+
A module rename touches **two user-facing API surfaces** that must each be preserved by default
96+
and only change under `otel.instrumentation.common.v3-preview`. Both cases use
97+
`AgentCommonConfig.get().isV3Preview()` to gate the switch, but the mechanism differs because
98+
the surfaces are different: config keys can coexist as aliases, emitted scope names cannot.
99+
100+
### 1. `InstrumentationModule` names (controls `otel.instrumentation.<name>.enabled`)
101+
102+
The names passed to the `InstrumentationModule` constructor drive the
103+
`otel.instrumentation.<name>.enabled` config keys. A rename silently breaks users who have
104+
the old key in their config.
105+
106+
Keep the pre-rename name as an **additional alias at the end of the list**, and drop it under
107+
v3-preview:
108+
109+
```java
110+
public CxfInstrumentationModule() {
111+
super("cxf", additionalNames());
112+
}
113+
114+
private static String[] additionalNames() {
115+
if (AgentCommonConfig.get().isV3Preview()) {
116+
return new String[] {"jaxws-2.0-cxf-3.0", "jaxws"};
117+
}
118+
return new String[] {"jaxws-2.0-cxf-3.0", "jaxws", "jaxws-cxf-3.0"};
119+
}
120+
```
121+
122+
Order matters: `AgentDistributionConfig#isInstrumentationEnabled` checks names in the order
123+
given and returns on the first explicit setting. Put the alias **last** so the new name wins
124+
when both are set.
125+
126+
### 2. Emitted instrumentation scope name (`INSTRUMENTATION_NAME` in `*Singletons`)
127+
128+
The `INSTRUMENTATION_NAME` string passed to `Instrumenter.builder(...)` becomes the
129+
`otel.scope.name` attribute on every emitted span / metric / log. A rename silently breaks
130+
dashboards and filters that match on the old scope.
131+
132+
Keep emitting the **pre-rename** scope name by default, and switch to the new one only under
133+
v3-preview:
134+
135+
```java
136+
public class CxfSingletons {
137+
private static final String INSTRUMENTATION_NAME =
138+
AgentCommonConfig.get().isV3Preview()
139+
? "io.opentelemetry.jaxws-2.0-cxf-3.0"
140+
: "io.opentelemetry.jaxws-cxf-3.0";
141+
...
142+
}
143+
```
144+
145+
Note the asymmetry with the config-key case: there the list contains **both** names at once
146+
(aliases); here only **one** scope name is emitted at a time, and the default is the **old**
147+
one.
148+
149+
### CHANGELOG
150+
151+
The rename is **not** a breaking change or a deprecation in the current release: by default
152+
the old config keys and scope names continue to work unchanged, and the new names are only
153+
visible under `otel.instrumentation.common.v3-preview` — a preview flag that users are not
154+
generally encouraged to enable. Do not add a `⚠️ Breaking changes to non-stable APIs` or
155+
`🚫 Deprecations` entry for this kind of rename.
156+
The breaking change will be recorded when v3-preview becomes the default in 3.0.
157+
78158
## What to Flag in Review
79159

80160
- **Breaking change without a prior deprecation**: a method/class was removed or its signature
@@ -98,3 +178,7 @@ default void configure(IgnoredTypesBuilder builder, ConfigProperties config) {
98178

99179
- **Missing CHANGELOG entry**: a breaking change PR that does not add an
100180
`⚠️ Breaking changes to non-stable APIs` bullet in the `Unreleased` section of `CHANGELOG.md`.
181+
182+
- **Module rename without backcompat**: `InstrumentationModule` constructor doesn't list the
183+
pre-rename name as an alias, and/or `*Singletons#INSTRUMENTATION_NAME` was changed
184+
unconditionally instead of being gated on `AgentCommonConfig.get().isV3Preview()`.

.github/agents/knowledge/module-naming.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,14 @@ General rules:
8888
- Library packages use `io.opentelemetry.instrumentation.<lib>...`
8989
- Internal-only classes go in a `.internal` subpackage
9090

91+
## Renaming an existing module: backwards compatibility
92+
93+
A module rename touches two user-facing API surfaces — the
94+
`otel.instrumentation.<name>.enabled` config keys and the emitted `otel.scope.name` — that
95+
must both be preserved by default and only change under `otel.instrumentation.common.v3-preview`.
96+
See [api-deprecation-policy.md](api-deprecation-policy.md#module-renames-config-keys-and-emitted-scope-names)
97+
for the required pattern.
98+
9199
## What to Flag in Review
92100

93101
- **Module directory name missing the minimum version** for a third-party library.

0 commit comments

Comments
 (0)