-
Notifications
You must be signed in to change notification settings - Fork 901
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Instrument cassandra executeReactive method #6441
Instrument cassandra executeReactive method #6441
Conversation
|
1000fdc
to
139043d
Compare
thx @SimoneGiusso!
we probably don't want to drop support for earlier cassandra 4.x versions, see our reasoning under the "Javaagent instrumentation" section here: instead, go ahead and create a new module |
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). |
139043d
to
634755c
Compare
...brary/src/main/java/io/opentelemetry/instrumentation/cassandra/v4_0/CassandraSingletons.java
Outdated
Show resolved
Hide resolved
…sandraTelemetry & CassandraTelemetryBuilder files
instrumentation/cassandra/cassandra-4.14/javaagent/build.gradle.kts
Outdated
Show resolved
Hide resolved
.../main/java/io/opentelemetry/instrumentation/cassandra/v4_0/CassandraAttributesExtractor.java
Outdated
Show resolved
Hide resolved
...ibrary/src/main/java/io/opentelemetry/instrumentation/cassandra/v4_0/CassandraTelemetry.java
Outdated
Show resolved
Hide resolved
...ibrary/src/main/java/io/opentelemetry/instrumentation/cassandra/v4_0/CassandraTelemetry.java
Outdated
Show resolved
Hide resolved
...ibrary/src/main/java/io/opentelemetry/instrumentation/cassandra/v4_0/CassandraTelemetry.java
Outdated
Show resolved
Hide resolved
...ibrary/src/main/java/io/opentelemetry/instrumentation/cassandra/v4_0/CassandraTelemetry.java
Outdated
Show resolved
Hide resolved
...elemetry/javaagent/instrumentation/cassandra/v4_14/CassandraClientInstrumentationModule.java
Outdated
Show resolved
Hide resolved
.../main/java/io/opentelemetry/javaagent/instrumentation/cassandra/v4_14/TracingCqlSession.java
Outdated
Show resolved
Hide resolved
...src/main/java/io/opentelemetry/instrumentation/cassandra/v4_0/CassandraTelemetryBuilder.java
Outdated
Show resolved
Hide resolved
instrumentation/cassandra/cassandra-4.14/javaagent/src/test/groovy/CassandraClientTest.groovy
Outdated
Show resolved
Hide resolved
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! |
track |
7113aa6
to
926ed7e
Compare
|
1aa92b9
to
128f0f6
Compare
2891ad0
to
128f0f6
Compare
5e1eb0b
to
18a26d3
Compare
18a26d3
to
0ca0549
Compare
46b2e88
to
8841346
Compare
3bc4320
to
5f03349
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @SimoneGiusso,
the change below should get the tests running
febca6e
to
3e8ea44
Compare
public ReactiveResultSet executeReactive(Statement<?> statement) { | ||
return new DefaultReactiveResultSet(() -> originalTracingCqlSession.executeAsync(statement)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the core change to instrument the new reactive method.
…der the new library module (removing 'javaagent' from the package path)
ab7274c
to
74fd803
Compare
Hi @trask and @mateuszrzeszutek. To sum-up: we are instrumenting the We have decided to not modify the current The new entry point for both modules is the
The new Could you please review the PR (I should applied the previous @mateuszrzeszutek's comments)? Fell free to ask questions. Thanks! |
ccf1cd7
to
74fd803
Compare
# Conflicts: # settings.gradle.kts
track |
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. |
hi @SimoneGiusso sorry for the long delay, we will try to get to this in the next week |
…tereactive-cassandra-method
hey @SimoneGiusso! I pushed some changes to your branch:
|
|
||
public abstract class AbstractCassandra44Test extends AbstractCassandraTest { | ||
|
||
// TODO add reactive tests here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you rewrite the couple of reactive groovy tests here in Java?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// TODO can this be rewritten using newer API? | ||
@SuppressWarnings("deprecation") | ||
Statement<?> statement = executionInfo.getStatement(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SimoneGiusso can you look at this to see if there's a better way to do this in 4.4+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They made ExecutionInfo
compatible with any Request
type, and the current deprecated method just do a cast. We can do the same.
it would be great to add a README for the new library instrumentation, see #6947, but we can merge without it too |
c7e7735
to
8593fe4
Compare
thx @SimoneGiusso! |
Happy to contribute. Thank you for your help and reviews @trask! |
It follows the issue I opened some days ago.
The
executeReactive
method use the same processor used byexecuteAsync
(see here) and wrap the callback in theDefaultReactiveResultSet
publisher.Here I'm simply overriding the
executeReactive
method doing the same thing: call the already instrumentedexecuteAsync
method and wrapping the callback using theDefaultReactiveResultSet
publisher.I did an upgrade of the-> Cassandra-4.4 is enough.java-driver-core
library to haveTracingCqlSession.java
extending theReactiveSession
. I have to probably rename thecassandra-4.0
module incassandra-4.14
but I'll let you confirm this.