Skip to content

Commit d6b3a39

Browse files
traskCopilot
andauthored
Catch exception variable naming (open-telemetry#17220)
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent c92e699 commit d6b3a39

37 files changed

Lines changed: 228 additions & 185 deletions

File tree

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ When a "Knowledge File" is listed, load it from `knowledge/` before reviewing th
1717
| Style | Style guide | Always ||
1818
| Style | Uppercase field names should reflect semantic constants or immutable value constants such as `Duration` timeouts/intervals, not simply `static final` | Always ||
1919
| Naming | Getter naming (`get` / `is`) | Always ||
20+
| Style | Prefer `e` for used exceptions, prefer `f` for used exceptions in nested catch clauses when an outer catch already uses `e`, allow `error` for specific `*Error` catch types, prefer `t` for used `Throwable` values, prefer `ignored` for intentionally unused catch parameters, and use `ignore` for nested intentionally unused catch parameters when `ignored` would shadow an outer catch variable | Catch clauses, `Throwable` params ||
2021
| Naming | Module/package naming | New or renamed modules/packages | `module-naming.md` |
2122
| Javaagent | Advice patterns | `@Advice` classes | `javaagent-advice-patterns.md` |
2223
| Javaagent | Module structure patterns | `InstrumentationModule`, `TypeInstrumentation` | `javaagent-module-patterns.md` |
@@ -56,6 +57,21 @@ Flag real defects, including:
5657

5758
Only flag substantive problems, not stylistic preference.
5859

60+
## [Javaagent] Best-Effort Suppressed Failures
61+
62+
When javaagent runtime code intentionally suppresses a `Throwable` to avoid breaking the
63+
application, do not silently swallow it.
64+
65+
- Prefer `ExceptionLogger.logSuppressedError(...)` for suppressed failures in javaagent code.
66+
It logs at `FINE` and matches the agent's default ByteBuddy advice suppression path in
67+
`ExceptionHandlers.defaultExceptionHandler()`.
68+
- Use this for best-effort hooks such as response customizers, bootstrap fallbacks, or other
69+
optional extension points where failure must not change application behavior.
70+
- Do not introduce ad-hoc local loggers for one-off suppressed javaagent failures when
71+
`ExceptionLogger` is available from bootstrap.
72+
- Keep the message action-oriented and specific (for example, "Failed to customize Netty 4.1
73+
HTTP server response").
74+
5975
## [Style] Style Guide
6076

6177
Read and apply `docs/contributing/style-guide.md`.
@@ -87,6 +103,28 @@ Do not flag the following patterns (common false positives):
87103

88104
Public API getters should use `get*` (or `is*` for booleans).
89105

106+
## [Style] Catch Exception Variable Naming
107+
108+
Prefer `e` as the exception variable name in catch clauses when the exception is
109+
used.
110+
111+
For nested catch clauses, when an outer catch already uses `e`, prefer `f` as the
112+
inner exception variable name when the exception is used.
113+
114+
`error` is also acceptable when the caught type is a specific `*Error` subtype.
115+
116+
Prefer `t` for used `Throwable` values, including `catch (Throwable t)` and
117+
`Throwable` callback parameters.
118+
119+
If a catch parameter is intentionally unused, prefer `ignored` over `ignore`.
120+
121+
For nested catch clauses, if `ignored` would shadow an outer catch variable,
122+
prefer `ignore` for the inner intentionally unused catch parameter.
123+
124+
Both `ignored` and `ignore` are recognized by IntelliJ's `CatchMayIgnoreException`
125+
inspection as intentional unused-catch names. In test sources, IntelliJ also
126+
recognizes `expected` and `ok`.
127+
90128
## [Style] Prefer Instance Creation Over Singletons
91129

92130
Stateless implementations of telemetry interfaces — `TextMapGetter`, `TextMapSetter`,

instrumentation/grails-3.0/javaagent/src/test/java/test/GrailsTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ private static String getContextPathKey() {
8686
try {
8787
ServerProperties.class.getDeclaredMethod("getServlet");
8888
return "server.servlet.contextPath";
89-
} catch (NoSuchMethodException ignore) {
89+
} catch (NoSuchMethodException ignored) {
9090
return "server.context-path";
9191
}
9292
}

instrumentation/grpc-1.6/library/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/BodySizeUtil.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,15 +18,15 @@ final class BodySizeUtil {
1818
private static Class<?> getMessageLiteClass() {
1919
try {
2020
return Class.forName("com.google.protobuf.MessageLite");
21-
} catch (Exception ignore) {
21+
} catch (Exception ignored) {
2222
return null;
2323
}
2424
}
2525

2626
private static Method getSerializedSizeMethod(Class<?> clazz) {
2727
try {
2828
return clazz.getMethod("getSerializedSize");
29-
} catch (NoSuchMethodException ignore) {
29+
} catch (NoSuchMethodException ignored) {
3030
return null;
3131
}
3232
}
@@ -40,7 +40,7 @@ static <T> Long getBodySize(T message) {
4040
}
4141
try {
4242
return ((Integer) serializedSizeMethod.invoke(message)).longValue();
43-
} catch (Throwable ignore) {
43+
} catch (Throwable ignored) {
4444
return null;
4545
}
4646
}

instrumentation/grpc-1.6/testing/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/AbstractGrpcStreamingTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -500,7 +500,7 @@ private static void usePlainText(ManagedChannelBuilder<?> channelBuilder) throws
500500
.getClass()
501501
.getMethod("usePlaintext", boolean.class)
502502
.invoke(channelBuilder, true);
503-
} catch (NoSuchMethodException unused) {
503+
} catch (NoSuchMethodException ignored) {
504504
channelBuilder.getClass().getMethod("usePlaintext").invoke(channelBuilder);
505505
}
506506
}

instrumentation/grpc-1.6/testing/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/AbstractGrpcTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1703,7 +1703,7 @@ private static void usePlainText(ManagedChannelBuilder<?> channelBuilder) throws
17031703
.getClass()
17041704
.getMethod("usePlaintext", boolean.class)
17051705
.invoke(channelBuilder, true);
1706-
} catch (NoSuchMethodException unused) {
1706+
} catch (NoSuchMethodException ignored) {
17071707
channelBuilder.getClass().getMethod("usePlaintext").invoke(channelBuilder);
17081708
}
17091709
}

instrumentation/jaxws/jaxws-metro-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/metro/SoapFaultBuilderInstrumentation.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ public static void onExit(@Advice.Argument(2) Throwable throwable) {
4545
// if fiber is not attached current() throws IllegalStateException
4646
try {
4747
request = Fiber.current().getPacket();
48-
} catch (IllegalStateException ignore) {
48+
} catch (IllegalStateException ignored) {
4949
// fiber not available
5050
}
5151
if (request != null) {

instrumentation/jaxws/jaxws-metro-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/metro/TracingTube.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ public NextAction processException(Throwable throwable) {
5454
// if fiber is not attached current() throws IllegalStateException
5555
try {
5656
request = Fiber.current().getPacket();
57-
} catch (IllegalStateException ignore) {
57+
} catch (IllegalStateException ignored) {
5858
// fiber not available
5959
}
6060
if (request != null) {

instrumentation/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/JdbcUtils.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ public static DbInfo computeDbInfo(@Nullable Connection connection) {
109109
if (url != null) {
110110
try {
111111
return JdbcConnectionUrlParser.parse(url, connection.getClientInfo());
112-
} catch (Throwable ex) {
112+
} catch (Throwable ignored) {
113113
// getClientInfo is likely not allowed.
114114
return JdbcConnectionUrlParser.parse(url, null);
115115
}

instrumentation/jdbc/testing/src/main/java/io/opentelemetry/instrumentation/jdbc/testing/AbstractJdbcInstrumentationTest.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,8 +140,8 @@ static void tearDown() {
140140
if (dataSource instanceof Closeable) {
141141
try {
142142
((Closeable) dataSource).close();
143-
} catch (IOException ignore) {
144-
// ignore
143+
} catch (IOException ignored) {
144+
// ignore exceptions during close
145145
}
146146
}
147147
}));

instrumentation/jetty/jetty-8.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/QueuedThreadPoolTest.java

Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
package io.opentelemetry.javaagent.instrumentation.jetty.v8_0;
77

8-
import static org.junit.jupiter.api.Assumptions.assumeTrue;
8+
import static org.junit.jupiter.api.Assumptions.abort;
99

1010
import io.opentelemetry.api.trace.SpanKind;
1111
import io.opentelemetry.instrumentation.testing.junit.AgentInstrumentationExtension;
@@ -25,29 +25,26 @@ void dispatchPropagates() throws Exception {
2525
QueuedThreadPool pool = new QueuedThreadPool();
2626
// run test only if QueuedThreadPool has dispatch method
2727
// dispatch method was removed in jetty 9.1
28-
Method dispatch = null;
28+
Method dispatch;
2929
try {
3030
dispatch = QueuedThreadPool.class.getMethod("dispatch", Runnable.class);
31-
} catch (NoSuchMethodException ignore) {
32-
// ignore
31+
} catch (NoSuchMethodException ignored) {
32+
abort("Jetty 9.1+ removed dispatch(Runnable)");
33+
return;
3334
}
34-
assumeTrue(dispatch != null);
3535
pool.start();
3636

37-
Method finalDispatch = dispatch;
3837
testing.runWithSpan(
3938
"parent",
4039
() -> {
4140
// this child will have a span
4241
JavaAsyncChild child1 = new JavaAsyncChild();
4342
// this child won't
4443
JavaAsyncChild child2 = new JavaAsyncChild(false, false);
45-
if (finalDispatch != null) {
46-
finalDispatch.invoke(pool, child1);
47-
finalDispatch.invoke(pool, child2);
48-
child1.waitForCompletion();
49-
child2.waitForCompletion();
50-
}
44+
dispatch.invoke(pool, child1);
45+
dispatch.invoke(pool, child2);
46+
child1.waitForCompletion();
47+
child2.waitForCompletion();
5148
});
5249

5350
testing.waitAndAssertTraces(
@@ -67,23 +64,20 @@ void dispatchPropagatesLambda() throws Exception {
6764
QueuedThreadPool pool = new QueuedThreadPool();
6865
// run test only if QueuedThreadPool has dispatch method
6966
// dispatch method was removed in jetty 9.1
70-
Method dispatch = null;
67+
Method dispatch;
7168
try {
7269
dispatch = QueuedThreadPool.class.getMethod("dispatch", Runnable.class);
73-
} catch (NoSuchMethodException ignore) {
74-
// ignore
70+
} catch (NoSuchMethodException ignored) {
71+
abort("Jetty 9.1+ removed dispatch(Runnable)");
72+
return;
7573
}
76-
assumeTrue(dispatch != null);
7774
pool.start();
7875

7976
JavaAsyncChild child = new JavaAsyncChild(true, true);
80-
Method finalDispatch = dispatch;
8177
testing.runWithSpan(
8278
"parent",
8379
() -> {
84-
if (finalDispatch != null) {
85-
finalDispatch.invoke(pool, JavaLambdaMaker.lambda(child));
86-
}
80+
dispatch.invoke(pool, JavaLambdaMaker.lambda(child));
8781
});
8882

8983
// We block in child to make sure spans close in predictable order

0 commit comments

Comments
 (0)