Aerospike client instrumentation #9836

Abhishekkr3003 · 2023-11-09T19:54:10Z

Resolves #6578

linux-foundation-easycla · 2023-11-09T19:54:14Z

The committers listed above are authorized under a signed CLA.

✅ login: Abhishekkr3003 / name: Abhishek Kumar (b4af69a, a30b6d7, e82bdcb, 14f1109, 7c17dfc, 58f1b46, faf4a92, 226b9e1, 7f84218, ecdd747, afda86d, 5059c8e, a03903c, ec950dc, 5da5b5b, 4133b58, d1818c1, 3a3e509, 93da28f, c34b20d, 72c35de, 0ef5f40, 79765c5, 453c2b9, 3b6ab9b, cfc0227, 7d2f3a5, cc92b86)

laurit · 2023-11-14T11:55:10Z

...onv/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/db/AerospikeMetrics.java

+import java.util.concurrent.TimeUnit;
+import java.util.logging.Logger;
+
+public final class AerospikeMetrics implements OperationListener {


Usually we place framework specific instrumentation classes with the framework instrumentation not here

Moved it to the library in framework instrumentation.

laurit · 2023-11-14T11:55:26Z

instrumentation/aerospike-client/aerospike-client-7.1/javaagent/build.gradle.kts

+
+  compileOnly("com.google.auto.value:auto-value-annotations")
+  annotationProcessor("com.google.auto.value:auto-value")
+  testInstrumentation(project(":instrumentation:aerospike-client:aerospike-client-7.1:javaagent"))


should be added automatically

Yes, changed it to use the framework instrumentation library.

laurit · 2023-11-14T11:56:15Z

instrumentation/aerospike-client/aerospike-client-7.1/javaagent/build.gradle.kts

+  pass {
+    group.set("com.aerospike")
+    module.set("aerospike-client")
+    versions.set("[7.1.0,)")


usually we also add assertInverse.set(true) to verify that the instrumentation does not pass on older versions

Added, assertInverse

...pentelemetry/javaagent/instrumentation/aerospike/v7_1/AerospikeClientAttributeExtractor.java

laurit · 2023-11-14T12:00:54Z

...test/java/io/opentelemetry/javaagent/instrumentation/aerospike/v7_1/AerospikeClientTest.java

+import org.testcontainers.containers.wait.strategy.Wait;
+
+@SuppressWarnings("deprecation") // until old http semconv are dropped in 2.0
+class AerospikeClientTest {


could you also add tests for the metrics

...a/io/opentelemetry/javaagent/instrumentation/aerospike/v7_1/AsyncHandlerInstrumentation.java

laurit · 2023-11-14T12:04:37Z

...a/io/opentelemetry/javaagent/instrumentation/aerospike/v7_1/AsyncHandlerInstrumentation.java

+          request.setStatus(Status.FAILURE);
+        }
+        requestContext.endSpan(AersopikeSingletons.instrumenter(), context, request, throwable);
+        Scope scope = context.makeCurrent();


activating scope to just close it isn't useful, you can delete this

Yes, cleaned this

laurit · 2023-11-14T12:07:37Z

...a/io/opentelemetry/javaagent/instrumentation/aerospike/v7_1/AsyncCommandInstrumentation.java

+    @Advice.OnMethodExit(suppress = Throwable.class)
+    public static void onExit(
+        @Advice.This Command command,
+        @Advice.Local("otelAerospikeRequest") AerospikeRequest request,


@Advice.Local is usually used to pass variables from enter advice to exit advice. Just using it on exit advice doesn't make sense, you could just as well just have a local inside the advice method.

Refactored it as well.

…ibutes and formatting change

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

instrumentation/aerospike-client/aerospike-client-7.1/library/build.gradle.kts

@@ -0,0 +1,11 @@
+plugins {
+  id("otel.library-instrumentation")


By library instrumentation we mean an instrumentation that can be used without javaagent which this module is not. In my opinion you should move the classes from this module to the javaagent module.

Thanks for clarifying! I have moved the metrics to javaagent module.

laurit · 2023-12-14T13:14:07Z

...ry/src/main/java/io/opentelemetry/instrumentation/aerospike/v7_1/AerospikeMetricsAdvice.java

+final class AerospikeMetricsAdvice {
+  private AerospikeMetricsAdvice() {}
+
+  @SuppressWarnings("deprecation") // until old http semconv are dropped in 2.0


old http semconv has been removed from the agent, you can also remove it

Alright, removed it.

laurit · 2023-12-14T15:46:50Z

...rary/src/test/java/io/opentelemetry/instrumentation/aerospike/v7_1/AerospikeMetricsTest.java

+  @SuppressWarnings("deprecation")
+  // until old http semconv are dropped in 2.0
+  void collectsMetrics() {
+    InMemoryMetricReader metricReader = InMemoryMetricReader.create();


Testing metrics this way is a bit unusual, it would be easier to combine traces test with metrics like in

opentelemetry-java-instrumentation/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java

Line 709 in 30ddf6a

testing.waitAndAssertMetrics(

Yeah, indeed. Merged metrics tests with traces tests.

… old semconv

laurit · 2024-01-30T12:09:14Z

.../java/io/opentelemetry/javaagent/instrumentation/aerospike/v7_1/AerospikeRequestContext.java

+    contextThreadLocal.remove();
+  }
+
+  @SuppressWarnings("unchecked")


is the suppress warnings really necessary?

laurit · 2024-01-30T14:55:06Z

instrumentation/aerospike-client/aerospike-client-7.1/javaagent/build.gradle.kts

+  pass {
+    group.set("com.aerospike")
+    module.set("aerospike-client")
+    versions.set("[4.4.9,)")


I guess you want this instrumentation to work starting from 7.1.0 and don't care about earlier versions? The idea here is to have the muzzle version the same as the earliest supported version to avoid the situation where instrumentation applies to some earlier version but does not work correctly. You could bring the minimum version down to 5.0.0 where tests seem to pass with a small modification or I guess you could artificially restrict the version where the instrumentation works with something like

opentelemetry-java-instrumentation/instrumentation/jedis/jedis-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jedis/v4_0/JedisInstrumentationModule.java

Lines 24 to 27 in 0337158

@Override

public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {

return hasClassesNamed("redis.clients.jedis.CommandArguments");

}

where you enable the instrumentation only when a class that was added in 7.1.0 is present (of course this only works when such a class exists).

laurit · 2024-01-30T14:56:20Z

...telemetry/javaagent/instrumentation/aerospike/v7_1/AerospikeClientInstrumentationModule.java

+  @Override
+  public boolean isIndyModule() {
+    return false;
+  }


isIndyModule is an experimental feature you don't need to add this method unless you know that what is done in this instrumentation fails with indy for some reason

laurit · 2024-01-30T15:01:59Z

...va/io/opentelemetry/javaagent/instrumentation/aerospike/v7_1/SyncCommandInstrumentation.java

+    }
+
+    @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
+    public static AerospikeRequestContext stopSpan(


I think you should return void instead of AerospikeRequestContext, the return value isn't used

laurit · 2024-01-30T15:05:01Z

...va/io/opentelemetry/javaagent/instrumentation/aerospike/v7_1/SyncCommandInstrumentation.java

+      }
+      context = AersopikeSingletons.instrumenter().start(parentContext, request);
+      scope = context.makeCurrent();
+      return AerospikeRequestContext.attach(request, context);


I think it would make sense to skip the ThreadLocal when you don't really need it. Perhaps have a method that just creates the request context? Or just use instrumenter().end(...) or have a static version of requestContext.endSpan(...) so you could call it without the request context.

laurit · 2024-01-30T16:05:56Z

...main/java/io/opentelemetry/javaagent/instrumentation/aerospike/v7_1/AersopikeSingletons.java

+            .addAttributesExtractor(NetworkAttributesExtractor.create(netAttributesGetter))
+            .addOperationMetrics(AerospikeMetrics.get());
+    if (InstrumentationConfig.get()
+        .getBoolean("otel.instrumentation.aerospike.experimental-span-attributes", false)) {


this flag should be document in README.md like in https://github.com/Abhishekkr3003/opentelemetry-java-instrumentation/blob/aerospike-client-instrumentation/instrumentation/grpc-1.6/README.md

laurit · 2024-01-30T16:15:17Z

...a/io/opentelemetry/javaagent/instrumentation/aerospike/v7_1/AsyncCommandInstrumentation.java

+      VirtualField<Command, AerospikeRequestContext> virtualField =
+          VirtualField.find(Command.class, AerospikeRequestContext.class);
+      AerospikeRequestContext requestContext = virtualField.get(command);
+      if (requestContext != null) {


if it is really possible that the command under construction already has a request attached then it would be better to add a comment here how this could happen

laurit · 2024-01-30T16:19:00Z

...a/io/opentelemetry/javaagent/instrumentation/aerospike/v7_1/AsyncCommandInstrumentation.java

+      if (!instrumenter().shouldStart(parentContext, request)) {
+        return;
+      }
+      Context context = instrumenter().start(parentContext, request);


I get the feeling that constructor of the command object isn't a good instrumentation point. Perhaps EventLoop.execute(...) would be better? When you need to use thread local to pass context then ideally you should be able set it at the start of the method and clear it at then end.

laurit · 2024-01-30T16:22:52Z

...a/io/opentelemetry/javaagent/instrumentation/aerospike/v7_1/AsyncHandlerInstrumentation.java

+        AerospikeRequest request = requestContext.getRequest();
+        Context context = requestContext.getContext();
+        if (throwable == null) {
+          request.setStatus(Status.SUCCESS);


my understanding is that otel generally favors leaving status unset when operation was successful.

laurit · 2024-01-30T16:30:51Z

...test/java/io/opentelemetry/javaagent/instrumentation/aerospike/v7_1/AerospikeClientTest.java

+    eventPolicy.commandsPerEventLoop = 2;
+    EventLoops eventLoops = new NioEventLoops(eventPolicy, eventLoopSize);
+    clientPolicy.eventLoops = eventLoops;
+    clientPolicy.maxConnsPerNode = 11;


For me the test fails to start with

AerospikeClientTest > initializationError FAILED com.aerospike.client.AerospikeException: Error -1: asyncMaxConnsPerNode 11 must be >= event loop count 16 at app//com.aerospike.client.cluster.Cluster.<init>(Cluster.java:340) at app//com.aerospike.client.AerospikeClient.<init>(AerospikeClient.java:294) at app//com.aerospike.client.AerospikeClient.<init>(AerospikeClient.java:243) at app//io.opentelemetry.javaagent.instrumentation.aerospike.v7_1.AerospikeClientTest.setupSpec(AerospikeClientTest.java:71)

Abhishekkr3003 · 2024-11-26T20:36:51Z

@laurit, I have updated this PR based on your previous comments.

laurit · 2024-11-27T10:22:11Z

conventions/.kotlin/errors/errors-1732624750398.log

@@ -0,0 +1,4 @@
+kotlin version: 2.0.20


remove this file

laurit · 2024-11-27T10:22:30Z

instrumentation/aerospike-client/README.md

+| System property                                               | Type    | Default | Description                                                          |
+|---------------------------------------------------------------|---------|---------|----------------------------------------------------------------------|
+| `otel.instrumentation.aerospike.experimental-span-attributes` | Boolean | `false` | Enable the capture of experimental Aerospike client span attributes. |
+| `otel.instrumentation.aerospike.experimental-metrics`         | Boolean | `false` | Enable the recording of experimental Aerospike client metrics        |


laurit · 2024-11-27T10:27:16Z

instrumentation/aerospike-client/javaagent/build.gradle.kts

+}
+
+dependencies {
+  library("com.aerospike:aerospike-client:8.0.0")


What version do you want to support? Here you use 8, but in package name you use 7. Muzzle passes for 4, would that work?
Since the latest version seems to require java 21 you need to do something like

opentelemetry-java-instrumentation/instrumentation/spring/spring-kafka-2.7/javaagent/build.gradle.kts

Lines 83 to 87 in c6a6fa6

if (latestDepTest) {

otelJava {

minJavaVersionSupported.set(JavaVersion.VERSION_17)

}

}

laurit · 2024-11-27T11:05:27Z

...telemetry/javaagent/instrumentation/aerospike/v7_0/AerospikeClientInstrumentationModule.java

+
+  @Override
+  public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
+    return hasClassesNamed("com.aerospike.client.IAerospikeClient")


hasClassesNamed accepts an array of strings so you could have one call to hasClassesNamed that lists all the classes. Usually you'd use hasClassesNamed here to avoid running the instrumentation on old version of library and here you'd test that a class that was added in the new version is present. Looking at the list of classes makes me wonder why you are doing this and whether this is needed at all.

laurit · 2024-11-27T11:12:50Z

Could you also update https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/supported-libraries.md

jaydeluca · 2024-11-27T10:30:38Z

...main/java/io/opentelemetry/javaagent/instrumentation/aerospike/v7_0/AersopikeSingletons.java

+import io.opentelemetry.javaagent.instrumentation.aerospike.v7_0.internal.AerospikeRequest;
+import io.opentelemetry.javaagent.instrumentation.aerospike.v7_0.metrics.AerospikeMetrics;
+
+public final class AersopikeSingletons {


Suggested change

public final class AersopikeSingletons {

public final class AerospikeSingletons {

laurit · 2024-11-27T13:42:42Z

instrumentation/aerospike-client/javaagent/build.gradle.kts

+  if (latestDepTest) {
+    library("com.aerospike:aerospike-client:+")
+  } else {
+    library("com.aerospike:aerospike-client:7.1.0")
+  }


just library("com.aerospike:aerospike-client:7.1.0") would be enough, when latest dep tests are run version for library dependencies is automatically changed to latest

Abhishekkr3003 requested a review from a team November 9, 2023 19:54

Abhishekkr3003 added 2 commits November 10, 2023 01:48

adds aerospike instrumentation

faf4a92

minor refactoring and adds testInstrumentation

a03903c

Abhishekkr3003 force-pushed the aerospike-client-instrumentation branch from a39cf52 to a03903c Compare November 9, 2023 20:20

Abhishekkr3003 and others added 5 commits November 10, 2023 04:12

changes muzzle fail and test case container startup check

ecdd747

checkstyle complaint package name update

226b9e1

Merge branch 'main' into aerospike-client-instrumentation

58f1b46

changes tests to include server close

ec950dc

refactors tests

14f1109

laurit reviewed Nov 14, 2023

View reviewed changes

Abhishekkr3003 added 4 commits November 16, 2023 02:21

removes unuseful scopes, local variables and refactors metrics

4133b58

adds aerospike metrics test and adds check for experimental-span-attr…

b4af69a

…ibutes and formatting change

adds muzzle assert inverse

d1818c1

removes :test file

e82bdcb

laurit reviewed Dec 14, 2023

View reviewed changes

Abhishekkr3003 added 3 commits December 30, 2023 23:53

Merge branch 'main' into aerospike-client-instrumentation

7c17dfc

Merge branch 'main' into aerospike-client-instrumentation

5da5b5b

refactors metrics into javaagent and updates metrics test and removes…

5059c8e

… old semconv

Abhishekkr3003 force-pushed the aerospike-client-instrumentation branch from bd4a7f8 to 5059c8e Compare December 31, 2023 13:56

updates build.gradle

93da28f

Abhishekkr3003 force-pushed the aerospike-client-instrumentation branch from f99bcdb to 93da28f Compare December 31, 2023 15:41

laurit reviewed Jan 30, 2024

View reviewed changes

Abhishekkr3003 added 5 commits February 10, 2024 10:02

Merge branch 'main' into aerospike-client-instrumentation

a30b6d7

minor refactoring

7f84218

Merge branch 'main' into aerospike-client-instrumentation

afda86d

restructure the modules to v7.0.0

0ef5f40

adds readme for system properties

3a3e509

github-actions bot requested a review from theletterf February 11, 2024 16:50

Merge branch 'main' into aerospike-client-instrumentation

7d2f3a5

Abhishekkr3003 requested a review from a team as a code owner November 26, 2024 12:56

refactors aerospike module

cc92b86

Abhishekkr3003 force-pushed the aerospike-client-instrumentation branch from 7db1a8e to 4bb06f2 Compare November 26, 2024 15:06

github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Nov 26, 2024

Abhishekkr3003 added 2 commits November 26, 2024 20:44

fixes errors

453c2b9

Merge branch 'main' into aerospike-client-instrumentation

cfc0227

Abhishekkr3003 force-pushed the aerospike-client-instrumentation branch from 4bb06f2 to cfc0227 Compare November 26, 2024 15:14

Abhishekkr3003 and others added 2 commits November 26, 2024 21:12

applies spotless fixes

3b6ab9b

refactoring module a level up

c34b20d

laurit reviewed Nov 27, 2024

View reviewed changes

conventions/.kotlin/errors/errors-1732624750398.log Outdated

@@ -0,0 +1,4 @@

kotlin version: 2.0.20

Copy link

Contributor

laurit Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this file

laurit reviewed Nov 27, 2024

View reviewed changes

jaydeluca reviewed Nov 27, 2024

View reviewed changes

fixes muzzle, refactors module, updates classLoadMatcher and added doc

72c35de

laurit reviewed Nov 27, 2024

View reviewed changes

removes library version check

79765c5

	@Override
	public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
	return hasClassesNamed("redis.clients.jedis.CommandArguments");
	}

	\| `otel.instrumentation.aerospike.experimental-metrics` \| Boolean \| `false` \| Enable the recording of experimental Aerospike client metrics \|
	\| `otel.instrumentation.aerospike.experimental-metrics` \| Boolean \| `false` \| Enable the recording of experimental Aerospike client metrics. \|

	if (latestDepTest) {
	otelJava {
	minJavaVersionSupported.set(JavaVersion.VERSION_17)
	}
	}

	public final class AersopikeSingletons {
	public final class AerospikeSingletons {

Aerospike client instrumentation #9836

Are you sure you want to change the base?

Aerospike client instrumentation #9836

Conversation

Abhishekkr3003 commented Nov 9, 2023

linux-foundation-easycla bot commented Nov 9, 2023 • edited Loading

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

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

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

Choose a reason for hiding this comment

Abhishekkr3003 commented Nov 26, 2024

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

laurit commented Nov 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Nov 9, 2023 •

edited

Loading