Add thrift javaagent instrumentation #8370

tkyun123 · 2023-04-26T17:14:44Z

For personal needs, I'm trying to trace my thrift application, and I finally Implemented a general thrift javaagent instrumentation , I'd like to share with those who hava same need.

I hava completed javaagent part, and I will complete tests within the next few weeks

linux-foundation-easycla · 2023-04-26T17:14:48Z

The committers listed above are authorized under a signed CLA.

✅ login: tkyun123 (b6a981e, e67e1b5, e1faef5, 06b8573, 73d8a8a, a69b185, 1c0c3a0, cebb9ae, 2ab2b74, 203e1de, 085e840, 056e927, 1af6a28, 59ef216, 02768ee)
✅ login: laurit / name: Lauri Tulmin (3f346a9)

mateuszrzeszutek

Hey @tkyun123,
Is this PR ready for review? Or perhaps did you want to open a draft PR?

mateuszrzeszutek · 2023-04-27T11:32:54Z

conventions/src/main/kotlin/otel.java-conventions.gradle.kts

@@ -71,7 +71,7 @@ tasks.withType<JavaCompile>().configureEach {
          "-Xlint:-options",

          // Fail build on any warning
-          "-Werror"
+//          "-Werror"


There's several "debug" changed in this PR you'll need to revert before it actually can receive approval; this is one of them.

mateuszrzeszutek · 2023-04-27T11:34:17Z

settings.gradle.kts

@@ -509,7 +509,7 @@ hideFromDependabot(":instrumentation:vibur-dbcp-11.0:javaagent")
 hideFromDependabot(":instrumentation:vibur-dbcp-11.0:library")
 hideFromDependabot(":instrumentation:vibur-dbcp-11.0:testing")
 hideFromDependabot(":instrumentation:wicket-8.0:javaagent")
-
+hideFromDependabot(":instrumentation:thrift:javaagent")


By convention, we put the minimum supported library version in the module name; in case of thrift it should be :instrumentation:thrift-0.14:javaagent

mateuszrzeszutek · 2023-04-27T11:35:55Z

instrumentation/thrift/javaagent/build.gradle.kts

+}
+
+dependencies {
+  implementation("org.apache.thrift:libthrift:0.14.1")


implementation will result in the library being included inside the javaagent, which probably is not what you're aiming for. You can use library instead (our own "syntactic sugar" configuration that's equivalent to compileOnly):

Suggested change

implementation("org.apache.thrift:libthrift:0.14.1")

library("org.apache.thrift:libthrift:0.14.1")

tkyun123 · 2023-04-28T04:08:37Z

thank for your advice，I will check and revert my debug changes，now this PR is not ready yet，I still need some time to finish all the test cases

tkyun123 · 2023-05-10T13:32:46Z

tests completed，please review

laurit · 2023-05-10T13:51:24Z

closing and reopening to trigger checks

laurit · 2023-05-11T12:56:23Z

@tkyun123 the first thing you should work on is to get the build passing

laurit · 2023-05-11T12:58:14Z

build.gradle.kts

@@ -98,3 +98,17 @@ if (gradle.startParameter.taskNames.any { it.equals("listTestsInPartition") }) {
    }
  }
 }
+
+allprojects {


You should remove this

laurit · 2023-05-11T13:00:53Z

...src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/AbstractProtocolWrapper.java

+import org.apache.thrift.protocol.TProtocolDecorator;
+
+public abstract class AbstractProtocolWrapper extends TProtocolDecorator {
+  public static final String OIJ_THRIFT_FIELD_NAME = "OIJ_FIELD_NAME";


this doesn't seem to be used anywhere

laurit · 2023-05-11T13:05:26Z

...src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/AbstractProtocolWrapper.java

+
+public abstract class AbstractProtocolWrapper extends TProtocolDecorator {
+  public static final String OIJ_THRIFT_FIELD_NAME = "OIJ_FIELD_NAME";
+  public static final short OIJ_THRIFT_FIELD_ID = 3333;


OIJ_THRIFT_FIELD_ID is a weird name , what does OIJ stand for? I assume that this is the id for the field that you use for context propagation? If so it would be nice to have a comment. Also would it make sense to use a larger number than 3333? Would that reduce the risk of colliding with an existing field? Would it make sense to make this configurable?

Or does it come from https://github.com/opentracing-contrib/java-thrift/blob/2fdf7c55309150fb9e3b06ebf773e37f80968d1b/src/main/java/io/opentracing/thrift/SpanProtocol.java#L52 ?

Yes. Make it configurable with a default value
May make sense

laurit · 2023-05-11T13:06:03Z

...src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/AbstractProtocolWrapper.java

+  public static final String OIJ_THRIFT_FIELD_NAME = "OIJ_FIELD_NAME";
+  public static final short OIJ_THRIFT_FIELD_ID = 3333;
+
+  public AbstractProtocolWrapper(TProtocol protocol) {


Does this class have any other purpose besides containing OIJ_THRIFT_FIELD_ID field?

No，just for containing this field

laurit · 2023-05-11T13:12:16Z

...rc/main/java/io/opentelemetry/javaagent/instrumentation/thrift/ClientOutProtocolWrapper.java

+import org.apache.thrift.protocol.TMessage;
+import org.apache.thrift.protocol.TProtocol;
+
+@SuppressWarnings({"serial"})


Is this @SuppressWarnings needed, this class does not seem to be serializable?

…-instrumentation

tkyun123 · 2023-05-12T00:23:01Z

I‘m confused about this muzzle check result，I‘m sure I do nothing to this instrumentation, How could I fix this

laurit · 2023-05-12T06:05:11Z

I‘m confused about this muzzle check result，I‘m sure I do nothing to this instrumentation, How could I fix this

delete https://github.com/tkyun123/opentelemetry-java-instrumentation/blob/085e840266fc3b6c5a0140f75874bbcea476ccac/build.gradle.kts#L102-L114

laurit · 2023-05-19T10:52:30Z

@opentelemetrybot update

trask · 2023-05-19T17:25:30Z

@opentelemetrybot help

opentelemetrybot · 2023-05-19T17:25:44Z

Available commands:

@opentelemetrybot spotless - runs ./gradlew spotlessApply
@opentelemetrybot license - runs ./gradlew generateLicenseReport
@opentelemetrybot apidiff - runs ./gradlew jApiCmp
@opentelemetrybot update - updates branch with merge commit
@opentelemetrybot rerun - re-runs failed checks (NOT IMPLEMENTED YET)
@opentelemetrybot help - displays available commands

trask · 2023-05-19T17:26:13Z

@opentelemetrybot update

trask · 2023-05-19T17:34:02Z

ah, it looks like our automation doesn't work when the PR is from a branch named main

@tkyun123 can you merge upstream/main into your PR, that will likely resolve the test failures

breedx-splk · 2023-06-06T16:30:53Z

@tkyun123 did you see @trask's comment about merging main into your fork?

Merge Main

tkyun123 · 2023-06-07T05:00:46Z

I am sorry for the late reply. Now I have mergd main into my PR

maokitty · 2023-06-20T02:43:10Z

...main/java/io/opentelemetry/javaagent/instrumentation/thrift/ThriftClientInstrumentation.java

+  @Override
+  public void transform(TypeTransformer transformer) {
+    transformer.applyAdviceToMethod(
+        isMethod().and(named("sendBase")).and(isProtected()),


It is possible that the sendbase method is public, should we remove this rule?

maokitty · 2023-06-20T02:46:21Z

...main/java/io/opentelemetry/javaagent/instrumentation/thrift/ThriftClientInstrumentation.java

+    transformer.applyAdviceToMethod(
+        isMethod().and(named("receiveBase")),
+        ThriftClientInstrumentation.class.getName() + "$ClientReceiveAdvice");
+    transformer.applyAdviceToMethod(


There are two constructors for TServiceClient. One of them takes two arguments

maokitty · 2023-06-20T11:46:47Z

...src/main/java/io/opentelemetry/javaagent/instrumentation/thrift/ServerInProtocolWrapper.java

+          return readFieldBegin();
+        }
+        context = serverInstrumenter().start(clientContext, request);
+        serverInstrumenter().end(context, request, 0, null);


This line is redundant

andrew-lozoya · 2024-09-16T18:49:12Z

Where is this PR at?

tkyun123 added 2 commits April 26, 2023 14:49

Add thrift plugin

b6a981e

check style

e67e1b5

tkyun123 requested a review from a team April 26, 2023 17:14

mateuszrzeszutek reviewed Apr 27, 2023

View reviewed changes

revert debug changes

e1faef5

trask marked this pull request as draft April 28, 2023 19:02

tkyun123 added 3 commits May 9, 2023 17:58

add test

06b8573

complete test

73d8a8a

change module name

a69b185

tkyun123 marked this pull request as ready for review May 10, 2023 13:32

laurit closed this May 10, 2023

laurit reopened this May 10, 2023

check style

1c0c3a0

laurit closed this May 11, 2023

laurit reopened this May 11, 2023

laurit and others added 2 commits May 11, 2023 14:15

Merge branch 'main' into main

3f346a9

check style

cebb9ae

laurit reviewed May 11, 2023

View reviewed changes

tkyun123 added 3 commits May 11, 2023 23:06

check style

2ab2b74

Merge branch 'main' of https://github.com/tkyun123/opentelemetry-java…

203e1de

…-instrumentation

Update opentelemetry-instrumentation-annotations.txt

085e840

tkyun123 added 2 commits May 12, 2023 15:58

Update build.gradle.kts

056e927

check style

1af6a28

Merge pull request #1 from open-telemetry/main

59ef216

Merge Main

replace Deprecated class

02768ee

maokitty reviewed Jun 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add thrift javaagent instrumentation #8370

Add thrift javaagent instrumentation #8370

tkyun123 commented Apr 26, 2023

linux-foundation-easycla bot commented Apr 26, 2023 •

edited

Loading

mateuszrzeszutek left a comment

mateuszrzeszutek Apr 27, 2023

mateuszrzeszutek Apr 27, 2023

mateuszrzeszutek Apr 27, 2023

tkyun123 commented Apr 28, 2023

tkyun123 commented May 10, 2023

laurit commented May 10, 2023

laurit commented May 11, 2023

laurit May 11, 2023

laurit May 11, 2023

laurit May 11, 2023

laurit May 11, 2023

tkyun123 May 11, 2023

tkyun123 May 11, 2023

laurit May 11, 2023

tkyun123 May 11, 2023

laurit May 11, 2023

tkyun123 commented May 12, 2023

laurit commented May 12, 2023

laurit commented May 19, 2023

trask commented May 19, 2023

opentelemetrybot commented May 19, 2023

trask commented May 19, 2023

trask commented May 19, 2023 •

edited

Loading

breedx-splk commented Jun 6, 2023

tkyun123 commented Jun 7, 2023

maokitty Jun 20, 2023

maokitty Jun 20, 2023

maokitty Jun 20, 2023

andrew-lozoya commented Sep 16, 2024

	implementation("org.apache.thrift:libthrift:0.14.1")
	library("org.apache.thrift:libthrift:0.14.1")

Add thrift javaagent instrumentation #8370

Are you sure you want to change the base?

Add thrift javaagent instrumentation #8370

Conversation

tkyun123 commented Apr 26, 2023

linux-foundation-easycla bot commented Apr 26, 2023 • edited Loading

mateuszrzeszutek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkyun123 commented Apr 28, 2023

tkyun123 commented May 10, 2023

laurit commented May 10, 2023

laurit commented May 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkyun123 commented May 12, 2023

laurit commented May 12, 2023

laurit commented May 19, 2023

trask commented May 19, 2023

opentelemetrybot commented May 19, 2023

trask commented May 19, 2023

trask commented May 19, 2023 • edited Loading

breedx-splk commented Jun 6, 2023

tkyun123 commented Jun 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-lozoya commented Sep 16, 2024

linux-foundation-easycla bot commented Apr 26, 2023 •

edited

Loading

trask commented May 19, 2023 •

edited

Loading