-
Notifications
You must be signed in to change notification settings - Fork 283
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
Add Vert.x 4 Redis client instrumentation #6233
Add Vert.x 4 Redis client instrumentation #6233
Conversation
b027924
to
2472020
Compare
There are some API changes in redis client from v3 to v4
So we need to instrument this method as well but we cannot wrap the response handler like we were doing in the vertx3 as we do not have the actual handler yet (set later using If we do not wrap the handler and just close the parent continuation, spans if any started in the response handler of redis query are not attached to the parent span due to which the @nayeem-kamal @bm1549 @PerfectSlayer Do you have any suggestions about what can be done for instrumenting methods returning |
Hello @akshaypatidar1999 About handling the if (clientScope != null) {
responseFuture = responseFuture.compose(
response -> {
clientScope.close();
return Future.succeededFuture(response);
},
throwable -> {
DECORATE.onError(clientScope, throwable);
return Future.failedFuture(throwable);
});
} Make sure to update its @Advice.Return(readOnly = false) Future<Response> responseFuture, |
Thanks @PerfectSlayer for your response. I am currently using If we use |
Sorry @akshaypatidar1999, you are right. My proposal will only take care of span duration, but any span created from a composed future based on the result won't be related to the parent span. To achieve such thing, we need to capture a As the vert.x Pair<Boolean, AgentScope.Continuation> pair =
InstrumentationContext.get(Request.class, Pair.class).get(request);
AgentScope.Continuation continuation = pair.getRight();
DECORATE.logging("Parent continuation", continuation);
if (clientScope != null) {
Promise<Response> promise = Promise.promise();
responseFuture.onComplete(event -> {
AgentScope scope = null;
try {
// Close client scope and span
if (!event.succeeded()) {
DECORATE.onError(clientScope, event.cause());
}
clientScope.close();
clientScope.span().finish();
// Activate parent continuation and trigger promise completion
if (continuation != null) {
scope = continuation.activate();
}
if (event.succeeded()) {
promise.complete(event.result());
} else {
promise.fail(event.cause());
}
} finally {
// Deactivate parent continuation
if (scope != null) {
scope.close();
}
}
});
responseFuture = promise.future();
} Creating a new promise should ensure to properly close the parent span continuation after any composed Note: I think you can also simplify the |
11bfb3d
to
a2a9ce3
Compare
Thanks, @PerfectSlayer, using I am using If we use Let me know if I am missing something. |
6ccd3be
to
27a9ace
Compare
apply from: "$rootDir/gradle/java.gradle" | ||
|
||
dependencies { | ||
compileOnly group: 'io.vertx', name: 'vertx-redis-client', version: '3.9.0' |
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.
Looks like this stub is using Vert.X Redis client 3.9, not 4. Isn’t an issue?
Does the stub project need to be copied or can it be a shared module between 3.9 an 4 instrumentation?
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.
It can be a shared module between 3.9 and 4 instrumentation, vertx 4 just has an extra method
@Override
public Request arg(boolean arg) {
return null;
}
dd-java-agent/instrumentation/vertx-redis-client-4.0/build.gradle
Outdated
Show resolved
Hide resolved
throws Throwable { | ||
ContextStore<Request, Pair> ctxt = InstrumentationContext.get(Request.class, Pair.class); | ||
Pair<Boolean, AgentScope.Continuation> pair = ctxt.get(request); | ||
if (pair != null && pair.hasLeft() && pair.getLeft() != null && pair.getLeft()) { |
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.
pair.hasLeft()
already covers pair.getLeft() != null
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.
Glad to see you made it work 👍
In order to merge the instrumentation, I think we will need to work on deduplicating code between the 3.9 and 4+ Vert.X Redis client instrumentations. Most of the code is similar so it would be great to create a common module for those parts, maybe be for the tests too.
We might also need to rename the package of the original 3.9 instrumentation, maybe introduce name aliases with version number of both instrumentations.
For all this kind of changes, we are looking for someone internally to help you with them and contribute too, so we can push your PR to the last miles to get it to our quality requirements (code maintenance, tests). cc @bm1549
About using a Pair
with the InstrumentationContext
, I would rather having two different context instances, one with the Boolean
, another with the Continuation
if they are not related to each other.
A last question for you could be: are you aware of another methods to instrument in the Redis Client 4 to get it fully covered, or do you think we got everything everyone could need?
There are three ways to interact with Redis Client 4 i.e I will be happy to make the changes to meet the quality requirements. About using a Pair with the InstrumentationContext, I also would have preferred two different context instances but we need them for the same @Override
public Map<String, String> contextStore() {
Map<String, String> contextStores = new HashMap<>();
contextStores.put("io.vertx.redis.client.Command", UTF8BytesString.class.getName());
contextStores.put("io.vertx.redis.client.Request", "datadog.trace.api.Pair");
contextStores.put("io.vertx.redis.client.RedisConnection", "io.vertx.core.net.SocketAddress");
return contextStores;
} |
Sorry, you were right. You can only have one type according a specific key (per It consists in using a specific instance of
You can create a such noop /** A marker to denote already instrumented Request (as a noop continuation to fit into the ContextStore). */
private static AgentScope.Continuation INSTRUMENTED_REQUEST_MARKER = AgentTracer.NoopAgentScope.INSTANCE.capture(); |
I had a quick chat with @amarziali Yesterday to explain him the context, what's remaining to get it merge and hand him over the PR. So don't be surprised if you start seeing commits pushed to the PR from him 😉 |
Thanks @PerfectSlayer |
Hi @akshaypatidar1999 I'm working on refactoring and removing some code duplication. Some of the duplicated advices for the 4.x will probably be removed since the advices from 3.9 can still be applied (with some slight modification). I'm finalising the refactoring (that can take still some little time) and adding tests as well for the new future based methods. So please don't be surprised if you notice that some of the classes on the 4.0 module will be deleted. |
Signed-off-by: Akshay Patidar <akshaypatidar1999@gmail.com>
Signed-off-by: Akshay Patidar <akshaypatidar1999@gmail.com>
Signed-off-by: Akshay Patidar <akshaypatidar1999@gmail.com>
Signed-off-by: Akshay Patidar <akshaypatidar1999@gmail.com>
Signed-off-by: Akshay Patidar <akshaypatidar1999@gmail.com>
b69a47d
to
5af94c7
Compare
@akshaypatidar1999 I finally managed to shrink everything in a module since the most of the advices were compatible with the 4.x and adapted the advice you provided for the future for the 4.x code. I added test suites for 3.9.0, 4.0.0, and 4.x (not testing latest 3.x since the branch is not alive). Also remove Flaky annotation (hence fixing the tests) since otherwise they were not run as mandatory on the CI. The work is pretty ready. @PerfectSlayer when you can review I'm fine with the PR thanks |
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.
Looks great! Nice improvement 👍
Just one comment about a FIXME
.
....9/src/main/java/datadog/trace/instrumentation/vertx_redis_client/RedisFutureSendAdvice.java
Show resolved
Hide resolved
thanks for the contribution @akshaypatidar1999 (and @PerfectSlayer for the review) |
What Does This Do
Adds instrumentation for Vert.x4 Redis Client
Fixes #6230
Motivation
We are migrating our applications to Vert.x 4 and are not getting any spans for redis
Additional Notes
Jira ticket: [PROJ-IDENT]