feat: Add support for Thrift 0.9.1 and later versions by YaoYingLong · Pull Request #13784 · open-telemetry/opentelemetry-java-instrumentation

YaoYingLong · 2025-04-28T11:24:43Z

Add support for Thrift 0.9.1 and later versions

Special Note：Although 51 files were added with a total of 16,937 lines of code, 12 of them are test-related files containing 14,960 lines of code, while the actual instrumentation adaptation code is only around 1,900 lines. Among the test-related code, approximately 9,000 lines were generated using the Thrift client based on IDL files for testing purposes. Due to the limitations of Thrift client code generation, these generated codes were pre-generated and placed in the project's test directory.

SylvainJuge · 2025-04-28T12:27:13Z

In order to minimize the amount of code, do you think it would be possible to re-generate the generated code on build instead of adding it to git ? The likely challenge here would be being able to maintain those over time, in particular it would really be useful to be able to re-generate and update those every time the thrift version changes, or even generating and testing multiple versions.

…be regenerated during the build process.

laurit · 2025-05-13T16:12:19Z

+  pass {
+    group.set("org.apache.thrift")
+    module.set("libthrift")
+    versions.set("[0.9.1,)")


usually we also add assertInverse.set(true) to verify that instrumentation does not apply on earlier versions

laurit · 2025-05-13T16:35:28Z

+var generateThrift = tasks.register<Exec>("generateThrift") {
+  group = "build"
+  description = "Generate Java code from Thrift IDL files"
+  commandLine(thriftExecutable, "--gen", "java", "-out", thriftOutputDir, thriftInputFile)


Binaries shouldn't be really added to the source code repository, there is even a scorecard check for that https://github.com/ossf/scorecard/blob/cd152cb6742c5b8f2f3d2b5193b41d9c50905198/docs/checks.md#binary-artifacts Also since the project needs to build on at least windows, linux and mac just having one binary wouldn't work. Currently code generation seems to fail https://scans.gradle.com/s/73advrdsyn2fk/console-log/task/:instrumentation:thrift:thrift-0.9.1:javaagent:generateThrift?anchor=1&page=1 I would have hoped that the compiler binaries would be available for download like for the gRPC but that doesn't seem to be the case.
I think it would be best to go back to adding the generated java files to the repo for now. We can work on automating the generation later when other issues have been sorted. One way we could do the generation would be to have the thrift compiler in a docker image and run that image during the build similarly to how semconv repo runs weaver https://github.com/open-telemetry/semantic-conventions-java/blob/36743b068e1d3bbff129b528a340a63fb544e390/build.gradle.kts#L86 An example of such image can be found from https://github.com/reddit/thrift-compiler/pkgs/container/thrift-compiler If we can't find an existing image for the version we need we can build it ourself. @open-telemetry/java-instrumentation-approvers do you have any suggestions? Would having the thrift compiler in docker image be worth the effort?

Then I will temporarily add the generated Java files to the repository, or temporarily remove the test code.

laurit · 2025-05-13T16:42:30Z

+ * Note that the 8888th field of record is reserved for transporting trace header. Because Thrift
+ * doesn't support to transport metadata.
+ */
+public abstract class AbstractProtocolWrapper extends TProtocolDecorator {


this code seems to originate from skywalking https://github.com/apache/skywalking-java/blob/9e80a4c85ba895083e892f2024611a27846c3c5f/apm-sniffer/apm-sdk-plugin/thrift-plugin/src/main/java/org/apache/skywalking/apm/plugin/thrift/wrapper/AbstractProtocolWrapper.java#L22 you should point to the origin of the code like in

opentelemetry-java-instrumentation/instrumentation/camel-2.20/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachecamel/ActiveContextManager.java

Line 6 in 2e5c12b

// Includes work from:

laurit · 2025-05-14T06:01:32Z

+import org.apache.thrift.protocol.TType;
+import org.apache.thrift.transport.TTransport;
+
+@SuppressWarnings("all")


instead of suppressing warnings we prefer to fix the code so that it wouldn't have the warnings

laurit · 2025-05-14T06:36:52Z

+        // TMultiplexedProcessor compatible
+        Field field = null;
+        try {
+          field = TProtocolDecorator.class.getDeclaredField("concreteProtocol");


when we use reflection we always fetch the field/method only once and the reuse it

laurit · 2025-05-14T07:37:08Z

@@ -0,0 +1,10 @@
+plugins {
+  id("otel.library-instrumentation")


By library instrumentation we mean instrumentation that can be applied without the agent. This module does not seem to contain a library instrumentation in that sense. Consider merging it into the javaagent module or if you intend to add instrumentations for more versions that could benefit from shared code you could rename this module from library to javaagent too.

This is the shared code for adding instrumentation for more versions, so I have renamed the module to javaagent.

laurit · 2025-05-14T07:37:33Z

+import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
+import javax.annotation.Nullable;
+
+final class ThriftAttributesExtractor implements AttributesExtractor<ThriftRequest, Integer> {


empty class

laurit · 2025-05-14T07:38:25Z

+
+import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor;
+
+public final class ThriftSpanNameExtractor implements SpanNameExtractor<ThriftRequest> {


there are semantic conventions that define the name for a RPC span, you should follow them. See RpcSpanNameExtractor.

laurit · 2025-05-14T07:42:31Z

+
+  @Override
+  public String getSystem(ThriftRequest request) {
+    return "thrift";


You should also create a pr to add thrift to rpc.system values in https://github.com/open-telemetry/semantic-conventions/blob/main/docs/rpc/rpc-spans.md. See open-telemetry/semantic-conventions@7c1aa90 for inspiration. It is possible that the value should be apache_thrift instead of thrift but I'll leave this for the semantic conventions folks to decide.

fatekingsama · 2025-06-02T17:07:51Z

@YaoYingLong 兄弟你还在维护这个pull req吗

YaoYingLong · 2025-06-09T07:03:02Z

@YaoYingLong 兄弟你还在维护这个pull req吗

在的只是最近没有时间顾的上来修改提的修改建议

…uggestions.

beztebya666 · 2026-02-13T15:11:23Z

Any updates?

beztebya666 · 2026-02-13T15:27:59Z

@YaoYingLong ?

github-actions · 2026-03-03T23:34:23Z

This PR has been labeled as stale due to lack of activity and needing author feedback. It will be automatically closed if there is no further activity over the next 7 days.

YaoYingLong · 2026-03-10T11:59:32Z

Any updates?

Optimized according to Laurit's suggestions.

…re/thrift-0.9.1

…butesGetter parameters.

…ate and populate the .fossa.yml file.

YaoYingLong · 2026-03-17T03:04:28Z

@beztebya666 @laurit Hello, both of you,

Thank you for your previous suggestions! I have made the necessary changes based on your feedback and have also fixed all the failing test cases. Could you please help review it again? Thank you so much!

MileaRobertStefan · 2026-04-03T10:27:29Z

Any updates on this?

beztebya666 · 2026-04-03T13:49:47Z

@beztebya666 @laurit Hello, both of you,

Thank you for your previous suggestions! I have made the necessary changes based on your feedback and have also fixed all the failing test cases. Could you please help review it again? Thank you so much!

@laurit need to see this

Copilot

Pull request overview

Adds OpenTelemetry Javaagent instrumentation for Apache Thrift (targeting Thrift 0.9.1+) by introducing a shared thrift-common library-instrumentation module and a thrift-0.9.1 javaagent module, and wiring these new projects into the build and FOSSA scanning configuration.

Changes:

Introduces instrumentation/thrift/thrift-common helpers (request model, header propagation helpers, RPC + network attribute getters, instrumenter factory).
Adds instrumentation/thrift/thrift-0.9.1 ByteBuddy instrumentations and protocol wrappers for client/server span creation, context propagation, and async handling.
Registers the new Gradle modules in settings.gradle.kts and adds new FOSSA Gradle targets.

Reviewed changes

Copilot reviewed 36 out of 36 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
settings.gradle.kts	Adds Thrift modules to the Gradle build (but currently drops `zio-http-3.0` include).
.fossa.yml	Adds FOSSA targets for the new Thrift modules (but currently drops `zio-http-3.0` target).
instrumentation/thrift/thrift-common/javaagent/build.gradle.kts	Declares dependencies for the shared Thrift instrumentation helpers.
instrumentation/thrift/thrift-common/javaagent/src/main/java/io/opentelemetry/instrumentation/thrift/common/ThriftRequest.java	Defines a request carrier for span naming/attributes and context propagation.
instrumentation/thrift/thrift-common/javaagent/src/main/java/io/opentelemetry/instrumentation/thrift/common/ThriftInstrumenterFactory.java	Centralizes client/server `Instrumenter` construction (RPC + network attrs, metrics, propagators).
instrumentation/thrift/thrift-common/javaagent/src/main/java/io/opentelemetry/instrumentation/thrift/common/ThriftSpanNameExtractor.java	Derives span names from service + method.
instrumentation/thrift/thrift-common/javaagent/src/main/java/io/opentelemetry/instrumentation/thrift/common/ThriftSpanStatusExtractor.java	Maps Thrift status/error to span status.
instrumentation/thrift/thrift-common/javaagent/src/main/java/io/opentelemetry/instrumentation/thrift/common/ThriftRpcAttributesGetter.java	Supplies RPC semantic convention attributes (`rpc.system/service/method`).
instrumentation/thrift/thrift-common/javaagent/src/main/java/io/opentelemetry/instrumentation/thrift/common/ThriftHeaderGetter.java	Extracts propagation headers from the Thrift request carrier.
instrumentation/thrift/thrift-common/javaagent/src/main/java/io/opentelemetry/instrumentation/thrift/common/ThriftHeaderSetter.java	Injects propagation headers into the Thrift request carrier.
instrumentation/thrift/thrift-common/javaagent/src/main/java/io/opentelemetry/instrumentation/thrift/common/SocketAccessor.java	Extracts underlying `Socket` from various Thrift `TTransport` implementations.
instrumentation/thrift/thrift-common/javaagent/src/main/java/io/opentelemetry/instrumentation/thrift/common/RequestScopeContext.java	Stores request + context + optional scope for lifecycle management.
instrumentation/thrift/thrift-common/javaagent/src/main/java/io/opentelemetry/instrumentation/thrift/common/client/MethodAccessor.java	Reflection-based helpers for method return-type discovery and protocol field access.
instrumentation/thrift/thrift-common/javaagent/src/main/java/io/opentelemetry/instrumentation/thrift/common/client/VirtualFields.java	Stores async callback associations via `VirtualField`.
instrumentation/thrift/thrift-common/javaagent/src/main/java/io/opentelemetry/instrumentation/thrift/common/client/ThriftClientNetworkAttributesGetter.java	Supplies network peer attributes for client spans.
instrumentation/thrift/thrift-common/javaagent/src/main/java/io/opentelemetry/instrumentation/thrift/common/server/ThriftServerNetworkAttributesGetter.java	Supplies network peer attributes for server spans.
instrumentation/thrift/thrift-0.9.1/javaagent/build.gradle.kts	Declares javaagent plugin usage and muzzle range for Thrift 0.9.1+.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/ThriftSingletons.java	Provides singleton instrumenters for client/server.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/AbstractProtocolWrapper.java	Base protocol decorator and reserved field constants for trace header transport.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/AsyncMethodCallbackWrapper.java	Ends spans on async completion/error.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/client/ThriftClientCommonInstrumentationModule.java	Registers client-side type instrumentations.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/client/ThriftClientInstrumentation.java	Instruments sync clients: wraps protocols and ends spans on send/receive.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/client/ThriftAsyncClientInstrumentation.java	Instruments async clients: wraps protocol factory to inject tracing.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/client/ClientProtocolFactoryWrapper.java	Wraps `TProtocolFactory` to return tracing protocol decorators.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/client/ClientOutProtocolWrapper.java	Client protocol wrapper that starts spans and injects trace headers via reserved field.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/client/ThriftAsyncMethodCallInstrumentation.java	Captures async callbacks and associates them with method calls via `VirtualField`.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/client/ThriftAsyncWriteArgsInstrumentation.java	Adjusts async oneway behavior and wires request scope to callbacks.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/server/ThriftServerInstrumentationModule.java	Registers server-side type instrumentations.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/server/ThriftServerInstrumentation.java	Wraps server protocol factory on server construction.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/server/ServerProtocolFactoryWrapper.java	Produces server-side tracing protocol wrapper instances.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/server/ServerInProtocolWrapper.java	Server protocol wrapper that extracts headers and starts server spans.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/server/ThriftFrameBufferInstrumentation.java	Captures transport on non-blocking servers to support socket extraction.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/server/ThriftAsyncProcessInstrumentation.java	Wraps async result handlers to end spans on callback completion.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/server/ThriftBaseProcessorInstrumentation.java	Ends server spans around sync processor `process()` execution.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/server/ThriftBaseAsyncProcessorInstrumentation.java	Ends server spans for async processor execution paths.
instrumentation/thrift/thrift-0.9.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/v0_9_1/server/ThriftMutiplexedProcessorInstrumentation.java	Handles `TMultiplexedProcessor` end-of-request behavior.

Copilot · 2026-04-03T23:32:06Z

 include(":instrumentation:xxl-job:xxl-job-common:javaagent")
 include(":instrumentation:xxl-job:xxl-job-common:testing")
 include(":instrumentation:zio:zio-2.0:javaagent")
-include(":instrumentation:zio:zio-http-3.0:javaagent")
+include(":instrumentation:thrift:thrift-0.9.1:javaagent")
+include(":instrumentation:thrift:thrift-common:javaagent")


settings.gradle.kts no longer includes the :instrumentation:zio:zio-http-3.0:javaagent project, but that module still exists in the repo (e.g., instrumentation/zio/zio-http-3.0/...) and is referenced from docs/instrumentation-list.yaml. This will stop it from being built/published and may break expectations; please re-add the include (and keep the Thrift includes as well).

Copilot · 2026-04-03T23:32:07Z

-      path: ./
-      target: ':instrumentation:zio:zio-http-3.0:javaagent'
    - type: gradle
      path: ./


.fossa.yml removed the Gradle target for :instrumentation:zio:zio-http-3.0:javaagent, but the module still exists and is listed in docs/instrumentation-list.yaml. Please restore the removed FOSSA target entry so license scanning continues to cover this instrumentation.

Suggested change

path: ./

path: ./

target: ':instrumentation:zio:zio-http-3.0:javaagent'

- type: gradle

path: ./

Copilot · 2026-04-03T23:32:07Z

+          return field;
+        }
+        Context context = serverInstrumenter().start(parentContext, request);
+        this.requestScopeContext = RequestScopeContext.create(request, null, context);


When the trace header is found in readFieldBegin(), the server span is started but the returned Context is never made current (scope is stored as null). This means downstream work during request processing won't see the server context. Consider calling context.makeCurrent() here too and storing the resulting Scope in RequestScopeContext (mirroring the readMessageEnd() path).

Suggested change

this.requestScopeContext = RequestScopeContext.create(request, null, context);

Scope scope = context.makeCurrent();

this.requestScopeContext = RequestScopeContext.create(request, scope, context);

In an asynchronous non-blocking scenario, the related logic will be executed in a separate thread. During thread switching, context.makeCurrent() will be executed once. If context.makeCurrent() is executed again at this point, it will result in duplicate executions of context.makeCurrent(), which may lead to issues.

Copilot · 2026-04-03T23:32:07Z

+  public static Set<String> voidMethodNames(String serviceName) {
+    Set<String> methodNames = SERVICE_NAME_METHOD_NAMES.getOrDefault(serviceName, new HashSet<>());
+    if (!methodNames.isEmpty()) {
+      return methodNames;
+    }


voidMethodNames() re-runs reflection every time for services that have no void-returning methods because it treats an empty cached set as “not cached” (if (!methodNames.isEmpty())). This can add unnecessary overhead on hot paths. Consider caching based on containsKey(serviceName) (or using computeIfAbsent) so an empty result is still memoized; also consider using a concurrent map since this cache is accessed from multiple threads in the agent.

Copilot · 2026-04-03T23:32:08Z

+  public TProtocol getProtocol(TTransport transport) {
+    TProtocol protocol = this.delegate.getProtocol(transport);
+    if (protocol instanceof ClientOutProtocolWrapper) {
+      if (transport != null) {
+        ((ClientOutProtocolWrapper) protocol).updateTransport(this.transport);
+      }
+      ((ClientOutProtocolWrapper) protocol).setServiceName(this.serviceName);
+      return protocol;
+    }
+    protocol = new ClientOutProtocolWrapper(protocol, this.serviceName, null);
+    if (transport != null) {
+      ((ClientOutProtocolWrapper) protocol).updateTransport(this.transport);
+    }


getProtocol(TTransport transport) ignores its transport parameter and instead calls updateTransport(this.transport). If the factory is ever reused with a different transport, the wrapper will retain the wrong transport/socket info. Please update the wrapper using the method parameter (and/or drop the stored transport field if it’s not needed).

The passed-in TTransport might be wrapped multiple times, and only the TTransport provided in the constructor is the original one that contains the socket.

Copilot · 2026-04-03T23:32:08Z

+public final class ThriftMutiplexedProcessorInstrumentation implements TypeInstrumentation {
+
+  @Override
+  public ElementMatcher<TypeDescription> typeMatcher() {
+    return named("org.apache.thrift.TMultiplexedProcessor");
+  }


Class name ThriftMutiplexedProcessorInstrumentation has a typo (“Mutiplexed” vs “Multiplexed”), while it instruments org.apache.thrift.TMultiplexedProcessor. Renaming to ThriftMultiplexedProcessorInstrumentation would improve discoverability and consistency (and update the module’s typeInstrumentations() list accordingly).

Copilot · 2026-04-03T23:32:08Z

+public final class ThriftClientNetworkAttributesGetter
+    implements ServerAttributesGetter<ThriftRequest>,
+        NetworkAttributesGetter<ThriftRequest, Integer> {
+
+  @Nullable
+  @Override
+  public InetSocketAddress getNetworkPeerInetSocketAddress(
+      ThriftRequest request, @Nullable Integer integer) {


ThriftClientNetworkAttributesGetter implements ServerAttributesGetter, but does not override getServerAddress() / getServerPort(). As a result, ServerAttributesExtractor.create(netClientAttributesGetter) in ThriftInstrumenterFactory.clientInstrumenter() is effectively a no-op. Either implement server address/port extraction (e.g., from the socket) or remove the ServerAttributesExtractor registration to avoid confusion and extra work.

Copilot · 2026-04-03T23:32:09Z

+import javax.annotation.Nullable;
+
+public enum ThriftHeaderGetter implements TextMapGetter<ThriftRequest> {
+  INSTANCE;
+
+  @Override
+  public Iterable<String> keys(ThriftRequest request) {


TextMapGetter.keys() does not handle a null carrier, while get() does. Many TextMapGetter implementations in this repo accept @Nullable and return an empty list when the carrier is null to avoid NPEs during propagation. Consider aligning keys() with that pattern.

Suggested change

import javax.annotation.Nullable;

public enum ThriftHeaderGetter implements TextMapGetter<ThriftRequest> {

INSTANCE;

@Override

public Iterable<String> keys(ThriftRequest request) {

import java.util.Collections;

import javax.annotation.Nullable;

public enum ThriftHeaderGetter implements TextMapGetter<ThriftRequest> {

INSTANCE;

@Override

public Iterable<String> keys(@Nullable ThriftRequest request) {

if (request == null) {

return Collections.emptyList();

}

Copilot · 2026-04-03T23:32:09Z

+public final class ThriftSpanNameExtractor implements SpanNameExtractor<ThriftRequest> {
+  @Override
+  public String extract(ThriftRequest request) {
+    if (request.getServiceName() == null || request.getMethodName() == null) {


ThriftRequest.getMethodName() is declared non-null, but ThriftSpanNameExtractor.extract() still checks request.getMethodName() == null. This is inconsistent and can hide actual contract violations (AutoValue will throw if methodName is null). Either make methodName nullable end-to-end or remove the null check and rely on the non-null contract.

Suggested change

if (request.getServiceName() == null || request.getMethodName() == null) {

if (request.getServiceName() == null) {

Copilot · 2026-04-03T23:32:09Z

+    transformer.applyAdviceToMethod(
+        isConstructor().and(takesArguments(2)),
+        ThriftClientInstrumentation.class.getName() + "$ConstructorTowAdvice");
+


Typo in advice class name: ConstructorTowAdvice should be ConstructorTwoAdvice (or similar). Since these names are used in stack traces and debugging, correcting the typo will make troubleshooting easier.

laurit · 2026-04-04T10:45:59Z

One issue with this PR is that it doesn't include tests. There original PR did include them but it also included a ton of generated code. Ideally code generation would happen during the build. I'm not familiar with thrift toolchain so I don't know whether it is easy (e.g. for protobuf there is a gradle plugin that runs protoc). If tools need to be installed manually one option we could try would be to test for the presence of the tools and skip the build or tests when the necessary tools are not present and the CI environment variable is not set. In ci workflow these tools could be installed. Anyway having tests in the PR even if the include generated code would be preferable to having no tests.
Secondly at first glance the instrumentation looks complicated. There is an opentracing instrumentation for thrift https://github.com/opentracing-contrib/java-thrift that looks simpler. It is a manual instrumentation library not an agent instrumentation. Idk whether the complexity rises from wiring this up for the agent or something else. Does anybody know what this instrumentation, that originates from skywalking, offers over the opentracing one?

beztebya666 · 2026-04-04T10:52:34Z

this is all turning into a very complex PR... we just wanted thrift support for this java agent..

is there an easier way? @laurit @YaoYingLong

I'll be honest, in 24 days this PR will be exactly a year old

…ses.

…re/thrift-0.9.1

YaoYingLong · 2026-04-07T10:50:35Z

One issue with this PR is that it doesn't include tests. There original PR did include them but it also included a ton of generated code. Ideally code generation would happen during the build. I'm not familiar with thrift toolchain so I don't know whether it is easy (e.g. for protobuf there is a gradle plugin that runs protoc). If tools need to be installed manually one option we could try would be to test for the presence of the tools and skip the build or tests when the necessary tools are not present and the CI environment variable is not set. In ci workflow these tools could be installed. Anyway having tests in the PR even if the include generated code would be preferable to having no tests. Secondly at first glance the instrumentation looks complicated. There is an opentracing instrumentation for thrift https://github.com/opentracing-contrib/java-thrift that looks simpler. It is a manual instrumentation library not an agent instrumentation. Idk whether the complexity rises from wiring this up for the agent or something else. Does anybody know what this instrumentation, that originates from skywalking, offers over the opentracing one?

@laurit @beztebya666

Regarding the implementation mentioned in https://github.com/opentracing-contrib/java-thrift, I took a closer look. Since Thrift is based on a layer 4 protocol, it inherently supports synchronous blocking, synchronous non-blocking, and asynchronous non-blocking modes. The SpanProcessor used on the server side is an implementation of Thrift's TProcessor, which indicates that it can only support synchronous blocking and synchronous non-blocking scenarios, but not asynchronous non-blocking scenarios (whether on the client or server side). The asynchronous non-blocking mode uses TAsyncProcessor + TNonblockingServer or TThreadedSelectorServer.

The fields exported in https://github.com/opentracing-contrib/java-thrift are actually quite limited and only support Thrift versions 0.13 and above. In this PR, network-related fields have been added for compatibility, supporting versions 0.9.1 and above, including differences in the oneway mechanism between versions 0.9.1, 0.9.2, and 0.9.3. Additionally, to ensure compatibility with asynchronous scenarios, the logic appears relatively complex. If only synchronous scenarios are considered and the interception for asynchronous scenarios is removed, the code would become much simpler. The implementation approach is actually the same as in java-thrift.

Regarding the tests, there are no available Docker images for version 0.9.x, so for now, only version 0.12 images can be used. Additionally, the previously removed test cases have been added back, with some simplifications and optimizations made.

laurit · 2026-04-07T11:57:07Z

@YaoYingLong thanks for the explanation. Just wondering if testing older versions is going to be a pain then perhaps we shouldn't bother with them at all? Would that simplify the instrumentation?

YaoYingLong · 2026-04-07T13:19:09Z

@YaoYingLong thanks for the explanation. Just wondering if testing older versions is going to be a pain then perhaps we shouldn't bother with them at all? Would that simplify the instrumentation?

@laurit

Because some of our applications use version 0.9.x, we have implemented compatibility for this version. However, if you think it’s unnecessary, I can remove this part of the compatibility logic and only support relatively higher versions. This would simplify the compatibility logic for ONEWAY messages, but it won’t significantly simplify the code or reduce its complexity because the asynchronous logic is the same in higher versions and version 0.9.1. The primary cause of complexity lies in the asynchronous logic, which is used quite frequently.

If you want me to simplify the compatibility logic for ONEWAY messages, please let me know, and I will make the adjustments.

laurit · 2026-04-07T14:36:51Z

Because some of our applications use version 0.9.x, we have implemented compatibility for this version.

Lets try to keep the support for old versions then.

trask · 2026-04-30T21:10:12Z

Thanks @YaoYingLong! Closing in favor of #18405

feat: Add support for Thrift 0.9.1 and later versions

4ecbd76

YaoYingLong requested a review from a team as a code owner April 28, 2025 11:24

feat: Adjust the Thrift service-related code required for testing to …

937e1ee

…be regenerated during the build process.

laurit reviewed May 14, 2025

View reviewed changes

YaoYingLong added 2 commits November 20, 2025 20:13

feat: Adjust and optimize the code according to the PR optimization s…

2fe16f7

…uggestions.

Merge branch 'main' into feature/thrift-0.9.1

5b88f26

breedx-splk added the needs author feedback Waiting for additional feedback from the author label Feb 24, 2026

github-actions Bot added the stale label Mar 3, 2026

github-actions Bot removed needs author feedback Waiting for additional feedback from the author stale labels Mar 10, 2026

YaoYingLong added 8 commits March 10, 2026 20:13

Merge branch 'main' into feature/thrift-0.9.1

91e7b06

feat: temporarily remove the test code.

966066e

Merge remote-tracking branch 'origin/feature/thrift-0.9.1' into featu…

e3f243d

…re/thrift-0.9.1

feat: Optimize the compilation issue caused by the change in RpcAttri…

fefbf83

…butesGetter parameters.

feat: Merge Thrift 0.9.1 and 0.9.3.

a31f02c

feat: Run ./gradlew generateFossaConfiguration to automatically gener…

8d2bb68

…ate and populate the .fossa.yml file.

feat: remove ThriftServletInstrumentation.

f91074b

Merge branch 'main' into feature/thrift-0.9.1

d66db2f

trask requested a review from Copilot April 3, 2026 23:25

Copilot started reviewing on behalf of trask April 3, 2026 23:25 View session

Copilot AI reviewed Apr 3, 2026

View reviewed changes

YaoYingLong added 2 commits April 7, 2026 18:46

feat: Optimize the revision suggestions for AI review，and add test ca…

9066d31

…ses.

Merge remote-tracking branch 'origin/feature/thrift-0.9.1' into featu…

1673651

…re/thrift-0.9.1

otelbot-java-instrumentation Bot added the test native This label can be applied to PRs to trigger them to run native tests label Apr 7, 2026

Merge branch 'main' into feature/thrift-0.9.1

5dfd1e1

saurabhdes mentioned this pull request Apr 20, 2026

Add support for thrift #1573

Closed

laurit added 5 commits April 22, 2026 15:20

Merge branch 'main' into feature/thrift-0.9.1

744f050

fix thrift source generation when bash isn't available

794194e

make indy ready

e6ec343

restore removed module

e3ae169

revert

12e2fd8

laurit mentioned this pull request Apr 29, 2026

Add instrumentation for thrift #18405

Merged

trask mentioned this pull request Apr 30, 2026

Pull Request Dashboard #18435

Open

trask closed this Apr 30, 2026


		import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor;

		public final class ThriftSpanNameExtractor implements SpanNameExtractor<ThriftRequest> {

	this.requestScopeContext = RequestScopeContext.create(request, null, context);
	Scope scope = context.makeCurrent();
	this.requestScopeContext = RequestScopeContext.create(request, scope, context);

	if (request.getServiceName() == null \|\| request.getMethodName() == null) {
	if (request.getServiceName() == null) {

Conversation

YaoYingLong commented Apr 28, 2025

Uh oh!

SylvainJuge commented Apr 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fatekingsama commented Jun 2, 2025

Uh oh!

YaoYingLong commented Jun 9, 2025

Uh oh!

beztebya666 commented Feb 13, 2026

Uh oh!

beztebya666 commented Feb 13, 2026

Uh oh!

github-actions Bot commented Mar 3, 2026

Uh oh!

YaoYingLong commented Mar 10, 2026

Uh oh!

YaoYingLong commented Mar 17, 2026

Uh oh!

MileaRobertStefan commented Apr 3, 2026

Uh oh!

beztebya666 commented Apr 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!