feat: Add support for Thrift 0.9.1 and later versions #13784
Conversation
|
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.
| pass { | ||
| group.set("org.apache.thrift") | ||
| module.set("libthrift") | ||
| versions.set("[0.9.1,)") |
There was a problem hiding this comment.
usually we also add assertInverse.set(true) to verify that instrumentation does not apply on earlier versions
| var generateThrift = tasks.register<Exec>("generateThrift") { | ||
| group = "build" | ||
| description = "Generate Java code from Thrift IDL files" | ||
| commandLine(thriftExecutable, "--gen", "java", "-out", thriftOutputDir, thriftInputFile) |
There was a problem hiding this comment.
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?
| * 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 { |
There was a problem hiding this comment.
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
| import org.apache.thrift.protocol.TType; | ||
| import org.apache.thrift.transport.TTransport; | ||
|
|
||
| @SuppressWarnings("all") |
There was a problem hiding this comment.
instead of suppressing warnings we prefer to fix the code so that it wouldn't have the warnings
| if (this.isOneway() && message.type != TMessageType.ONEWAY) { | ||
| TMessage onewayMessage = new TMessage(message.name, TMessageType.ONEWAY, message.seqid); | ||
| super.writeMessageBegin(onewayMessage); | ||
| } else { | ||
| super.writeMessageBegin(message); | ||
| } |
There was a problem hiding this comment.
Usually instrumentation should not affect how library works. You need to add a comment that explains why this is needed and why it is okay to do this.
| // TMultiplexedProcessor compatible | ||
| Field field = null; | ||
| try { | ||
| field = TProtocolDecorator.class.getDeclaredField("concreteProtocol"); |
There was a problem hiding this comment.
when we use reflection we always fetch the field/method only once and the reuse it
| @@ -0,0 +1,10 @@ | |||
| plugins { | |||
| id("otel.library-instrumentation") | |||
There was a problem hiding this comment.
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.
| import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor; | ||
| import javax.annotation.Nullable; | ||
|
|
||
| final class ThriftAttributesExtractor implements AttributesExtractor<ThriftRequest, Integer> { |
|
|
||
| import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor; | ||
|
|
||
| public final class ThriftSpanNameExtractor implements SpanNameExtractor<ThriftRequest> { |
There was a problem hiding this comment.
there are semantic conventions that define the name for a RPC span, you should follow them. See RpcSpanNameExtractor.
|
|
||
| @Override | ||
| public String getSystem(ThriftRequest request) { | ||
| return "thrift"; |
There was a problem hiding this comment.
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.
|
@YaoYingLong 兄弟你还在维护这个pull req吗 |
在的 只是最近没有时间顾的上来修改提的修改建议 |
Add support for Thrift 0.9.1 and later versions
#1573
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.