DRAFT: APM AI TOOLKIT New Integration: Feign Client Instrumentation#10579
DRAFT: APM AI TOOLKIT New Integration: Feign Client Instrumentation#10579
Conversation
Add instrumentation for OpenFeign (feign-core) HTTP client library. Covers both sync (Client) and async (AsyncClient) execution paths with distributed tracing context propagation. Files: - FeignClientInstrumentation: ByteBuddy type/method matchers for Client.execute - FeignAsyncClientInstrumentation: Matchers for AsyncClient.execute - FeignClientAdvice: Sync span lifecycle (enter/exit) - FeignAsyncClientAdvice: Async span lifecycle via CompletableFuture - FeignDecorator: HttpClientDecorator with operation names and tags - FeignHeadersInjectAdapter: CarrierSetter for trace context propagation - FeignClientTest: Spock tests using HttpClientTest base (70 tests pass) Generated by APM Instrumentation Toolkit (anubis) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| import java.util.concurrent.CompletableFuture; | ||
| import net.bytebuddy.asm.Advice; | ||
|
|
||
| public class FeignAsyncClientAdvice { |
There was a problem hiding this comment.
Nice to see the advice separated out into its own class, rather than nested inside the instrumentation class. This makes it more clear the advice is actually decoupled from the instrumentation declaration.
(basically the bytecode here is lifted and patched into the target method, this class is never actually linked and run)
|
|
||
| @Advice.OnMethodEnter(suppress = Throwable.class) | ||
| public static AgentScope onEnter(@Advice.Argument(0) final Request request) { | ||
| final int callDepth = CallDepthThreadLocalMap.incrementCallDepth(Client.class); |
There was a problem hiding this comment.
Interesting to see it use CallDepthThreadLocalMap - this will stop additional spans being created when requests are reentrant (i.e. loop back into itself) - not always required, but easy enough to add from the start.
Note CallDepthThreadLocalMap can be tricky to use - if you get it wrong and have a path where the thread doesn't reset it when the request is unwound then it can mean a thread will not create spans, because it thinks it is still in a request. The use here looks good.
| return activateSpan(span); | ||
| } | ||
|
|
||
| @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) |
There was a problem hiding this comment.
+1 for use of onThrowable = Throwable.class so the advice is always run, even when the patched method throws an exception (important to clean up the CallDepthThreadLocalMap, and suppress = Throwable.class to stop instrumentation errors from leaking out.
| DECORATE.beforeFinish(span); | ||
| } finally { | ||
| span.finish(); | ||
| CallDepthThreadLocalMap.reset(Client.class); |
There was a problem hiding this comment.
Not sure about this use of CallDepthThreadLocalMap.reset - if the future completes on a different thread then it could leave the original thread still thinking that the request is in-progress (from the perspective of its CallDepthThreadLocalMap)
|
|
||
| if (future != null) { | ||
| future.whenComplete( | ||
| (response, error) -> { |
There was a problem hiding this comment.
This compiles, but will result in an invokedynamic opcode in the bytecode where the target of the invokedynamic (the lambda) has not been prepared by the class being patched. In such cases we should either move the lambda use to a helper where it can be setup properly, or convert the lambda to a named class which is then injected as a helper.
Note this is something which we could have a linting rule for, because we've had developers make the same mistake. (And I was also confused why muzzle was failing before I realized that it was caused by the use of a lambda in the advice, which led to a confusing message about the advice class - I'll look into fixing this to make the message clearer.)
| scope.close(); | ||
|
|
||
| if (future != null) { | ||
| future.whenComplete( |
There was a problem hiding this comment.
future.whenComplete will return a new future - at the moment this is thrown away, so it has no effect.
To tell byte-buddy to pass the 'traced' future back to the patched method you would normally use @Advice.Return(readOnly=false) future and then assign the updated value to the future parameter.
Note this is very specific to byte-buddy as normally assigning a new value to a parameter has no side-effect outside of the method, but here the bytecode is patched into the target method so the store opcode actually does have a side-effect.
This is where clear examples of what you should (and shouldn't) do in advice classes would help, because the rules are subtly different to usual Java development.
There was a problem hiding this comment.
It's also where standard ways of wrapping a future to continue and close out a trace would help.
| import net.bytebuddy.description.type.TypeDescription; | ||
| import net.bytebuddy.matcher.ElementMatcher; | ||
|
|
||
| @AutoService(InstrumenterModule.class) |
There was a problem hiding this comment.
Most instrumentations at the moment follow this pattern, but we want to move towards having a single InstrumenterModule per integration which then lists the different instrumentations, in this case the 'sync' and 'async' cases.
| group = "io.github.openfeign" | ||
| module = "feign-core" | ||
| versions = "[10.0.0,)" | ||
| assertInverse = true |
There was a problem hiding this comment.
Nice - this assertInverse is something people often forget :)
| ":dd-java-agent:instrumentation:elasticsearch:elasticsearch-transport:elasticsearch-transport-7.3", | ||
| ":dd-java-agent:instrumentation:elasticsearch:elasticsearch-transport:elasticsearch-transport-common", | ||
| ":dd-java-agent:instrumentation:elasticsearch:elasticsearch-common", | ||
| ":dd-java-agent:instrumentation:feign-core", |
There was a problem hiding this comment.
Note this fails our more recent conventions:
Instrumentation naming convention violations found:
• feign-core
Module name 'feign-core' must end with a version (e.g., '2.0', '3.1.0') or one of: '-common', '-stubs', '-iast'
Naming rules:
1. Module name must end with a version (e.g., '2.0', '3.1') OR one of: '-common', '-stubs', '-iast'
2. Module name must include the parent directory name
Example: 'couchbase/couchbase-2.0' ✓ (contains 'couchbase')
|
Minor notes:
These are trivial though :) |
|
Ideas:
|
4 participants
THIS IS A TEST RUN FOR TRYING TO INSTRUMENT JAVA CODE WITH THE AI TOOLKIT, @mcculls to review
Add instrumentation for OpenFeign (feign-core) HTTP client library. Covers both sync (Client) and async (AsyncClient) execution paths with distributed tracing context propagation.
Files:
Generated by APM AI Integration Toolkit (anubis)
What Does This Do
Motivation
Additional Notes
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]
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.