feature: prevent span duplicates (#22)

* feature: prevent span duplicates

* refactor: sonarcloud issues

* prevent duplicate GitHub workflow

* revert last commit

* refactor: add null-check

* refactor: replace exception with return value

* test: add util tests

* fix test

* refactor: use ReadableSpan instead of SdkSpan

* fix gradle group

* apply spotless

* exclude our classes from transformation

* push docker images for every branch

* update workflow

* try fix workflow

* fix image tag format

* apply requested changes

.github/workflows/cr-master.yml → .github/workflows/ci-test-and-push-docker.yml

@@ -1,11 +1,11 @@
-# This workflow is triggered on pushes to the master branch.
-# It builds the agent and creates a multi-arch image for it.
+# This workflow builds the agent and creates a multi-arch image for it.
 # The image is then pushed to Docker Hub.
 #
-# The workflow consists of three jobs:
-# 1. build-agent: Builds the agent and uploads the resulting JAR as an artifact.
-# 2. build-image: Builds a Docker image for multiple platforms with the matrix strategy.
-# 3. merge: Merges the images for the different platforms into a manifest list and pushes it to Docker Hub.
+# The workflow consists of the following jobs:
+# 1. build-agent-jar: Builds the agent and uploads the resulting JAR as an artifact.
+# 2. dependency-submission: Generate a dependency graph.
+# 2. build-docker-images: Builds a Docker image for multiple platforms with the matrix strategy.
+# 3. merge-images: Merges the images for the different platforms into a manifest list and pushes it to Docker Hub.
 
 # To make the build faster, we use a matrix strategy to build the image for multiple platforms in parallel.
 # The build of the first job is copied over to the image build jobs, so that the application build is only done once.
@@ -14,18 +14,16 @@
 # For more information about how to build multi-arch images and advanced settings with Docker Buildx in GitHub actions, see:
 # https://docs.docker.com/build/ci/github-actions/multi-platform/
 
-name: Master Branch Continuous Release
+name: Branch Continuous Integration
 
 on:
   push:
-    branches:
-    - master
 
 env:
   REGISTRY_IMAGE: inspectit/inspectit-gepard-agent
 
 jobs:
-  build-agent:
+  build-agent-jar:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v4
@@ -56,9 +54,26 @@ jobs:
           name: agent-artifact
           path: inspectit-gepard-agent/build/libs/inspectit-gepard-agent.jar
 
-  build-image:
+  dependency-submission:
     runs-on: ubuntu-latest
-    needs: build-agent
+    permissions:
+      contents: write
+    steps:
+      - uses: actions/checkout@v4
+      - name: Set up JDK 17
+        uses: actions/setup-java@v4
+        with:
+          java-version: '17'
+          distribution: 'temurin'
+
+      # Generates and submits a dependency graph, enabling Dependabot Alerts for all project dependencies.
+      # See: https://github.com/gradle/actions/blob/main/dependency-submission/README.md
+      - name: Generate and submit dependency graph
+        uses: gradle/actions/dependency-submission@417ae3ccd767c252f5661f1ace9f835f9654f2b5 # v3.1.0
+
+  build-docker-images:
+    runs-on: ubuntu-latest
+    needs: build-agent-jar
     strategy:
       fail-fast: false
       matrix:
@@ -124,10 +139,9 @@ jobs:
           if-no-files-found: error
           retention-days: 1
 
-  merge:
+  merge-images:
     runs-on: ubuntu-latest
-    needs:
-      - build-image
+    needs: build-docker-images
     steps:
       - name: Download digests
         uses: actions/download-artifact@v4

inspectit-gepard-agent/build.gradle

@@ -19,7 +19,7 @@ plugins {
     id "com.palantir.docker" version "0.36.0"
 }
 
-group 'rocks.inspectit.gepard.agent'
+group 'rocks.inspectit.gepard'
 version = "0.0.1-SNAPSHOT"
 
 sourceCompatibility = "17"

inspectit-gepard-agent/src/main/java/rocks/inspectit/gepard/agent/instrumentation/hook/action/SpanAction.java

@@ -1,54 +1,52 @@
 /* (C) 2024 */
 package rocks.inspectit.gepard.agent.instrumentation.hook.action;
 
-import io.opentelemetry.api.OpenTelemetry;
 import io.opentelemetry.api.trace.Span;
 import io.opentelemetry.api.trace.Tracer;
 import io.opentelemetry.context.Context;
 import java.util.Objects;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import rocks.inspectit.gepard.agent.instrumentation.hook.action.exception.CouldNotCloseSpanScopeException;
+import rocks.inspectit.gepard.agent.instrumentation.hook.action.util.SpanUtil;
 import rocks.inspectit.gepard.agent.internal.otel.OpenTelemetryAccessor;
 
 /** This action contains the logic to start and end a {@link Span}. */
 public class SpanAction {
-
-  private static final String INSTRUMENTATION_SCOPE_NAME = "inspectit-gepard";
-
-  private final OpenTelemetry openTelemetry;
-
-  public SpanAction() {
-    this.openTelemetry = OpenTelemetryAccessor.getOpenTelemetry();
-  }
+  private static final Logger log = LoggerFactory.getLogger(SpanAction.class);
 
   /**
    * Starts a new {@link Span}. Should be called before {@link SpanAction#endSpan}.
    *
    * @param spanName the name of the span
-   * @return the scope of the started span
+   * @return the scope of the started span or null, if the current span has the same name
    */
   public AutoCloseable startSpan(String spanName) {
-    Span.current().getSpanContext();
+    // In the future, we still might want to set some attributes in the current span
+    if (SpanUtil.spanAlreadyExists(spanName)) {
+      log.debug("Span '{}' already exists at the moment. No new span will be started", spanName);
+      return null;
+    }
 
-    Tracer tracer = openTelemetry.getTracer(INSTRUMENTATION_SCOPE_NAME);
+    Tracer tracer = OpenTelemetryAccessor.getTracer();
     Span span = tracer.spanBuilder(spanName).setParent(Context.current()).startSpan();
     return span.makeCurrent();
   }
 
   /**
    * Ends the current span and closes its scope. Should be called after {@link
-   * SpanAction#startSpan}.
+   * SpanAction#startSpan}. If the scope is null, we won't do anything.
    *
    * @param spanScope the scope of the span, which should be finished
    */
   public void endSpan(AutoCloseable spanScope) {
-    Span current = Span.current();
+    if (Objects.isNull(spanScope)) return;
 
-    if (Objects.nonNull(spanScope)) {
-      try {
-        spanScope.close();
-      } catch (Exception e) {
-        throw new CouldNotCloseSpanScopeException(e);
-      }
+    Span current = Span.current();
+    try {
+      spanScope.close();
+    } catch (Exception e) {
+      throw new CouldNotCloseSpanScopeException(e);
     }
     current.end();
   }

inspectit-gepard-agent/src/main/java/rocks/inspectit/gepard/agent/instrumentation/hook/action/util/SpanUtil.java

@@ -0,0 +1,27 @@
+/* (C) 2024 */
+package rocks.inspectit.gepard.agent.instrumentation.hook.action.util;
+
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.sdk.trace.ReadableSpan;
+
+/** Util class to access span data, like the name or attributes. */
+public class SpanUtil {
+
+  private SpanUtil() {}
+
+  /**
+   * Checks, if the current span uses the provided span name. This check might be necessary to
+   * prevent span duplicates. For example, we would like to create a span for a method, which is
+   * already recorded by OpenTelemetry. In this case, we should not create a new span.
+   *
+   * @param spanName the name of the span with the format 'SimpleClassName.methodName', for instance
+   *     'SpanUtil.spanAlreadyExists'
+   * @return true, if the current span uses the provided span name
+   */
+  public static boolean spanAlreadyExists(String spanName) {
+    Span span = Span.current();
+
+    if (span instanceof ReadableSpan readableSpan) return spanName.equals(readableSpan.getName());
+    return false;
+  }
+}

inspectit-gepard-agent/src/main/java/rocks/inspectit/gepard/agent/internal/otel/OpenTelemetryAccessor.java

@@ -3,25 +3,23 @@
 
 import io.opentelemetry.api.GlobalOpenTelemetry;
 import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.api.trace.Tracer;
 
 /**
- * Singleton to access the {@link OpenTelemetry} instance. We use this accessor, because according
- * to the documentation of {@link GlobalOpenTelemetry}, the get() method should only be called once
- * during the application.
+ * Singleton to access the OpenTelemetry API. We use this accessor, because according to the
+ * documentation of {@link GlobalOpenTelemetry}, the get() method should only be called once during
+ * the application.
  */
 public class OpenTelemetryAccessor {
 
+  /** The instrumentation scope name we use for our spans and metrics */
+  public static final String INSTRUMENTATION_SCOPE_NAME = "inspectit-gepard";
+
+  /** Our global OpenTelemetry instance */
   private static OpenTelemetry openTelemetry;
 
   private OpenTelemetryAccessor() {}
 
-  /**
-   * @return the global {@link OpenTelemetry} instance
-   */
-  public static OpenTelemetry getOpenTelemetry() {
-    return openTelemetry;
-  }
-
   /**
    * Sets the global {@link OpenTelemetry} instance for inspectIT. This will allow us to create
    * traces or metrics. Should only be called once.
@@ -31,4 +29,11 @@ public static OpenTelemetry getOpenTelemetry() {
   public static void setOpenTelemetry(OpenTelemetry otel) {
     openTelemetry = otel;
   }
+
+  /**
+   * @return the tracer to create spans
+   */
+  public static Tracer getTracer() {
+    return openTelemetry.getTracer(INSTRUMENTATION_SCOPE_NAME);
+  }
 }

inspectit-gepard-agent/src/main/java/rocks/inspectit/gepard/agent/transformation/TransformationManager.java

@@ -1,9 +1,11 @@
 /* (C) 2024 */
 package rocks.inspectit.gepard.agent.transformation;
 
-import static net.bytebuddy.matcher.ElementMatchers.any;
+import static net.bytebuddy.matcher.ElementMatchers.*;
 
 import net.bytebuddy.agent.builder.AgentBuilder;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.matcher.ElementMatcher;
 import rocks.inspectit.gepard.agent.instrumentation.state.InstrumentationState;
 
 /** Responsible component for setting up class transformation for instrumentation */
@@ -34,7 +36,16 @@ public AgentBuilder modify(AgentBuilder agentBuilder) {
     DynamicTransformer transformer = new DynamicTransformer(instrumentationState);
     InspectitListener listener = new InspectitListener();
 
-    // In the future, we might add a white- or black-list for types
-    return agentBuilder.type(any()).transform(transformer).with(listener);
+    return agentBuilder.type(typeMatcher()).transform(transformer).with(listener);
+  }
+
+  /**
+   * Defines all types, which should (or should not) be transformed. We don't want to transform our
+   * own classes.
+   *
+   * @return the type matcher for transformation
+   */
+  private ElementMatcher.Junction<TypeDescription> typeMatcher() {
+    return not(nameStartsWith("rocks.inspectit.gepard.agent"));
   }
 }

inspectit-gepard-agent/src/test/java/rocks/inspectit/gepard/agent/instrumentation/hook/action/util/SpanUtilTest.java

@@ -0,0 +1,61 @@
+/* (C) 2024 */
+package rocks.inspectit.gepard.agent.instrumentation.hook.action.util;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import io.opentelemetry.api.GlobalOpenTelemetry;
+import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.context.Scope;
+import io.opentelemetry.sdk.OpenTelemetrySdk;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+class SpanUtilTest {
+
+  private Tracer tracer;
+
+  private final String spanName = "SpanUtilTest.method";
+
+  @BeforeEach
+  void beforeEach() {
+    GlobalOpenTelemetry.resetForTest();
+    OpenTelemetry openTelemetry = OpenTelemetrySdk.builder().buildAndRegisterGlobal();
+    tracer = openTelemetry.getTracer("inspectit-gepard");
+  }
+
+  @Test
+  void shouldReturnTrueWhenSpanExists() throws Exception {
+    Span span = tracer.spanBuilder(spanName).startSpan();
+    Scope scope = span.makeCurrent();
+
+    boolean exists = SpanUtil.spanAlreadyExists(spanName);
+
+    assertTrue(exists);
+
+    scope.close();
+    span.end();
+  }
+
+  @Test
+  void shouldReturnFalseWhenNoSpanExists() throws Exception {
+    boolean exists = SpanUtil.spanAlreadyExists(spanName);
+
+    assertFalse(exists);
+  }
+
+  @Test
+  void shouldReturnFalseWhenOtherSpanExists() throws Exception {
+    Span span = tracer.spanBuilder("dummy").startSpan();
+    Scope scope = span.makeCurrent();
+
+    boolean exists = SpanUtil.spanAlreadyExists(spanName);
+
+    assertFalse(exists);
+
+    scope.close();
+    span.end();
+  }
+}