Make decorator exception safe 2/3#12016
Conversation
There was a problem hiding this comment.
Found and fixed a critical double-exception-wrapping bug: HttpServerDecorator.doOnResponse() was calling the public wrapper method onResponseStatus() instead of the protected implementation doOnResponseStatus(), causing exceptions to be silently swallowed instead of propagating through the proper exception barrier. This violates the exception-safe decorator pattern established by PR #12016.
📊 Validated against 3 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 3e2c276 · What is Autotest? · Any feedback? Reach out in #autotest
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
| import io.grpc.StatusRuntimeException; | ||
| import java.util.BitSet; | ||
| import java.util.function.Function; | ||
| import org.checkerframework.checker.nullness.compatqual.NonNullDecl; |
There was a problem hiding this comment.
Yes. I have a hard time with IDEA importing various null check libraries... I have seen the one from javax, findbugs, JSpecify, Jetbrains, in addition to this one... I changed for javax like the other ones.
It happens when I asked it to add the annotations for various overrides. I guess it picks the one that are already in the classpath. And as various instrumentations have various classpath, they might have their own null check libraries which take priority.
I had a look at the codebase and I found some in context work too 😅
mcculls
left a comment
There was a problem hiding this comment.
Various comments (mostly around the wide variety of different @NonNulls being imported) but nothing blocking
What Does This Do
This PR is a follow up of #11998 that applies to more signals:
BaseDecoratorafterStart()onPeerConnection()- no delegation, only locked as no overrideHttpClientDecoratoronRequest()onResponse()HttpServerDecoratoronRequest()onResponseStatus()onResponse()Additionally, document null type and safety for the base decorators.
Motivation
The goal is to simplify how write instrumentations, and make the dumb way safe.
Additional Notes
This part is part of stack PRs:
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]