Instrument cassandra executeReactive method by SimoneGiusso · Pull Request #6441 · open-telemetry/opentelemetry-java-instrumentation

SimoneGiusso · 2022-08-08T06:21:28Z

It follows the issue I opened some days ago.

The executeReactive method use the same processor used by executeAsync (see here) and wrap the callback in the DefaultReactiveResultSet publisher.

Here I'm simply overriding the executeReactive method doing the same thing: call the already instrumented executeAsync method and wrapping the callback using the DefaultReactiveResultSet publisher.

I did an upgrade of the java-driver-core library to have TracingCqlSession.java extending the ReactiveSession. I have to probably rename the cassandra-4.0 module in cassandra-4.14 but I'll let you confirm this. -> Cassandra-4.4 is enough.

linux-foundation-easycla · 2022-08-08T06:21:30Z

The committers listed above are authorized under a signed CLA.

✅ login: SimoneGiusso / name: Simone Giusso (cf9763c)

trask · 2022-08-08T15:36:25Z

thx @SimoneGiusso!

I have to probably rename the cassandra-4.0 module in cassandra-4.14 but I'll let you confirm this.

we probably don't want to drop support for earlier cassandra 4.x versions, see our reasoning under the "Javaagent instrumentation" section here:
https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/VERSIONING.md#dropping-support-for-older-library-versions

instead, go ahead and create a new module cassandra-4.14 that contains just the reactive instrumentation. it's a little odd to have overlapping instrumentation modules (4.0 will still apply to 4.14), but we have a couple of other cases like this (e.g. mongo) and it's the best approach we have come up with to handle this kind of case

SimoneGiusso · 2022-08-08T20:16:44Z

I see. I'll probably able to do this change in two weeks, when I'll be back from vacations (I won't bring my laptop with me).

…sandraTelemetry & CassandraTelemetryBuilder files

trask · 2022-10-23T02:16:47Z

hi @SimoneGiusso! just checking in to see if you are still working on this? (looks like a great addition)

definitely let us know if you have any questions, thx!

hansh0801 · 2022-11-14T05:38:57Z

track

linux-foundation-easycla · 2022-11-14T20:56:22Z

The committers listed above are authorized under a signed CLA.

✅ login: SimoneGiusso / name: Simone Giusso (634755c, f59e4d7, 128f0f6, 926ed7e)

linux-foundation-easycla · 2022-11-14T21:16:35Z

The committers listed above are authorized under a signed CLA.

✅ login: SimoneGiusso / name: Simone Giusso (1aa92b9)

trask

hey @SimoneGiusso,

the change below should get the tests running

SimoneGiusso · 2022-11-27T16:59:39Z

+  public ReactiveResultSet executeReactive(Statement<?> statement) {
+    return new DefaultReactiveResultSet(() -> originalTracingCqlSession.executeAsync(statement));
+  }


This is the core change to instrument the new reactive method.

…der the new library module (removing 'javaagent' from the package path)

SimoneGiusso · 2022-11-27T18:29:50Z

Hi @trask and @mateuszrzeszutek.

To sum-up: we are instrumenting the executeReactive method (introduced in Cassandra 4.4). It consists in "simply" enrich the current *.v4_0.TracingCqlSession already implemented and add a new test for this method.

We have decided to not modify the current cassandra-4.0 module but to create a new cassandra-4.4 module. For this reason a new testing module has been created which groups all the common tests between these two modules (i.e. all the tests in the current cassandra-4.0).

The new entry point for both modules is the CassandraTelemetry class (following the @mateuszrzeszutek's suggestions). Two distinct classes are needed, one for each module, to create two distinct TracingCqlSession objects (see CassandraTelemetry.wrap(). Inheritance here has been used to:

Set the instrumenter's name.
Share the code to create the instrumenter (and avoid duplicating package private classes such as CassandraAttributesExtractor).

The new TracingCqlSession simply wrap the original TracingCqlSession and enables tracing of the executeReactive method. In other words cassandra-4.4 is offering what is already offered by cassandra-4.0, with the addition of this executeReactive method.

Could you please review the PR (I should applied the previous @mateuszrzeszutek's comments)? Fell free to ask questions.

Thanks!

# Conflicts: # settings.gradle.kts

hansh0801 · 2022-12-23T09:22:08Z

track

SimoneGiusso · 2023-01-18T12:32:33Z

Hi, @trask @mateuszrzeszutek. Did you have some time to check this PR? I think we are close to merge this branch. All tests are passing and I should have applied almost all your suggestions.

trask · 2023-02-24T04:11:05Z

hi @SimoneGiusso sorry for the long delay, we will try to get to this in the next week

…tereactive-cassandra-method

trask · 2023-02-27T02:43:58Z

hey @SimoneGiusso! I pushed some changes to your branch:

merged in the new cassandra Java tests
added testing for the library instrumentation
changed library instrumentation to target 4.4 instead of 4.0 (we generally want library instrumentation to target the latest, though this does mean now there's some code duplication between the 4.0 and 4.4 implementation, I think this is ok, I don't think we currently have a pattern for sharing code between a later library version and an earlier javaagent version)

trask · 2023-02-27T02:44:36Z

+
+public abstract class AbstractCassandra44Test extends AbstractCassandraTest {
+
+  // TODO add reactive tests here


can you rewrite the couple of reactive groovy tests here in Java?

trask · 2023-02-27T02:46:39Z

+    // TODO can this be rewritten using newer API?
+    @SuppressWarnings("deprecation")
+    Statement<?> statement = executionInfo.getStatement();


@SimoneGiusso can you look at this to see if there's a better way to do this in 4.4+

They made ExecutionInfo compatible with any Request type, and the current deprecated method just do a cast. We can do the same.

8593fe4

trask · 2023-02-27T02:49:51Z

it would be great to add a README for the new library instrumentation, see #6947, but we can merge without it too

trask · 2023-03-08T15:55:09Z

thx @SimoneGiusso!

SimoneGiusso · 2023-03-08T16:13:33Z

Happy to contribute. Thank you for your help and reviews @trask!

SimoneGiusso requested a review from a team August 8, 2022 06:21

SimoneGiusso force-pushed the instrumenting-executereactive-cassandra-method branch 2 times, most recently from 1000fdc to 139043d Compare August 8, 2022 08:20

new cassandra 4.14 module to support reactive tracing

634755c

SimoneGiusso force-pushed the instrumenting-executereactive-cassandra-method branch from 139043d to 634755c Compare August 21, 2022 18:57

fix DuplicateFileCopyingException

f59e4d7

SimoneGiusso commented Aug 21, 2022

View reviewed changes

Comment thread ...brary/src/main/java/io/opentelemetry/instrumentation/cassandra/v4_0/CassandraSingletons.java

Move CassandraSingletons in instrumentation libraries and add new Cas…

128f0f6

…sandraTelemetry & CassandraTelemetryBuilder files

mateuszrzeszutek reviewed Aug 31, 2022

View reviewed changes

SimoneGiusso force-pushed the instrumenting-executereactive-cassandra-method branch from 7113aa6 to 926ed7e Compare November 14, 2022 21:07

SimoneGiusso force-pushed the instrumenting-executereactive-cassandra-method branch 2 times, most recently from 1aa92b9 to 128f0f6 Compare November 14, 2022 22:24

github-actions Bot requested a review from theletterf November 15, 2022 19:36

SimoneGiusso force-pushed the instrumenting-executereactive-cassandra-method branch from 2891ad0 to 128f0f6 Compare November 15, 2022 19:42

Merge branch 'main' into instrumenting-executereactive-cassandra-method

25de63f

SimoneGiusso force-pushed the instrumenting-executereactive-cassandra-method branch from 5e1eb0b to 18a26d3 Compare November 15, 2022 22:07

sync with main branch

0ca0549

SimoneGiusso force-pushed the instrumenting-executereactive-cassandra-method branch from 18a26d3 to 0ca0549 Compare November 15, 2022 22:12

WIP: Fix tests

8841346

SimoneGiusso force-pushed the instrumenting-executereactive-cassandra-method branch from 46b2e88 to 8841346 Compare November 16, 2022 22:46

WIP: use lowest java-driver-core version that supports reactive

5f03349

SimoneGiusso force-pushed the instrumenting-executereactive-cassandra-method branch from 3bc4320 to 5f03349 Compare November 17, 2022 21:17

trask reviewed Nov 22, 2022

View reviewed changes

Comment thread instrumentation/cassandra/testing/build.gradle.kts Outdated

Share code keeping it package protected + clean-up

3e8ea44

SimoneGiusso force-pushed the instrumenting-executereactive-cassandra-method branch from febca6e to 3e8ea44 Compare November 27, 2022 16:56

SimoneGiusso commented Nov 27, 2022

View reviewed changes

Make CompletionStageFunction public and rename package for classes un…

74fd803

…der the new library module (removing 'javaagent' from the package path)

SimoneGiusso force-pushed the instrumenting-executereactive-cassandra-method branch from ab7274c to 74fd803 Compare November 27, 2022 18:11

SimoneGiusso force-pushed the instrumenting-executereactive-cassandra-method branch from ccf1cd7 to 74fd803 Compare November 27, 2022 18:48

Merge branch 'main' into instrumenting-executereactive-cassandra-method

308a11e

# Conflicts: # settings.gradle.kts

trask added 4 commits February 26, 2023 16:16

Merge remote-tracking branch 'upstream/main' into instrumenting-execu…

dc416a8

…tereactive-cassandra-method

Move library to 4.4

aced6ee

more

ed61c54

more

865b337

trask reviewed Feb 27, 2023

View reviewed changes

todo

a924133

trask reviewed Feb 27, 2023

View reviewed changes

Update docs

19225cd

Simone Giusso added 2 commits February 27, 2023 22:44

Rewrite cassandra reactive test in java

2385797

Remove deprecated ExecutionInfo#getStatement method

8593fe4

SimoneGiusso force-pushed the instrumenting-executereactive-cassandra-method branch from c7e7735 to 8593fe4 Compare February 27, 2023 22:26

laurit approved these changes Mar 8, 2023

View reviewed changes

trask merged commit 1a7e0f3 into open-telemetry:main Mar 8, 2023

SimoneGiusso deleted the instrumenting-executereactive-cassandra-method branch March 15, 2023 19:39

SimoneGiusso changed the title ~~Instrumenting cassandra executeReactive method~~ Instrument cassandra executeReactive method Aug 2, 2023


		public abstract class AbstractCassandra44Test extends AbstractCassandraTest {

		// TODO add reactive tests here

Conversation

SimoneGiusso commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla Bot commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trask commented Aug 8, 2022

Uh oh!

SimoneGiusso commented Aug 8, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trask commented Oct 23, 2022

Uh oh!

hansh0801 commented Nov 14, 2022

Uh oh!

linux-foundation-easycla Bot commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla Bot commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trask left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SimoneGiusso Nov 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimoneGiusso commented Nov 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hansh0801 commented Dec 23, 2022

Uh oh!

SimoneGiusso commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trask commented Feb 24, 2023

Uh oh!

trask commented Feb 27, 2023

Uh oh!

trask Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

SimoneGiusso Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

trask Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

SimoneGiusso Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trask commented Feb 27, 2023

Uh oh!

trask commented Mar 8, 2023

Uh oh!

SimoneGiusso commented Mar 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SimoneGiusso commented Aug 8, 2022 •

edited

Loading

linux-foundation-easycla Bot commented Aug 8, 2022 •

edited

Loading

linux-foundation-easycla Bot commented Nov 14, 2022 •

edited

Loading

linux-foundation-easycla Bot commented Nov 14, 2022 •

edited

Loading

SimoneGiusso Nov 27, 2022 •

edited

Loading

SimoneGiusso commented Nov 27, 2022 •

edited

Loading

SimoneGiusso commented Jan 18, 2023 •

edited

Loading

SimoneGiusso Feb 27, 2023 •

edited

Loading