POTEL 48 - Cleanup POTel TODOs (#3705)

* merge

* move from hub to scopes

* restore instrumenter code, do not use otel as instrumenter for potel

* changelog

* reword changelog

* restore multi init tests

* Set origin on span options instead of setting after transaction/span has been created

* restore SentryGestureListenerTracingTest

* respect ignored span origin when starting a child

* cleanup POTel TODOs

* remove another todo

sentry-opentelemetry/sentry-opentelemetry-agentcustomization/src/main/java/io/sentry/opentelemetry/SentryAutoConfigurationCustomizerProvider.java

@@ -55,7 +55,6 @@ public void customize(AutoConfigurationCustomizer autoConfiguration) {
             options.setIgnoredSpanOrigins(SpanUtils.ignoredSpanOriginsForOpenTelemetry());
             options.setSpanFactory(spanFactory);
             final @Nullable SdkVersion sdkVersion = createSdkVersion(options, versionInfoHolder);
-            // TODO [POTEL] is detecting a version mismatch between application and agent possible?
             if (sdkVersion != null) {
               options.setSdkVersion(sdkVersion);
             }

sentry-opentelemetry/sentry-opentelemetry-core/src/main/java/io/sentry/opentelemetry/SentrySampler.java

@@ -16,6 +16,7 @@
 import io.sentry.PropagationContext;
 import io.sentry.SamplingContext;
 import io.sentry.ScopesAdapter;
+import io.sentry.SentryLevel;
 import io.sentry.SentryTraceHeader;
 import io.sentry.SpanId;
 import io.sentry.TracesSamplingDecision;
@@ -123,8 +124,12 @@ public SamplingResult shouldSample(
       }
       return new SentrySamplingResult(parentSamplingDecision);
     } else {
-      // this should never happen and only serve to calm the compiler
-      // TODO [POTEL] log
+      scopes
+          .getOptions()
+          .getLogger()
+          .log(
+              SentryLevel.WARNING,
+              "Encountered a missing parent sampling decision where one was expected.");
       return new SentrySamplingResult(new TracesSamplingDecision(true));
     }
   }

sentry-opentelemetry/sentry-opentelemetry-core/src/main/java/io/sentry/opentelemetry/SentrySpanExporter.java

@@ -46,16 +46,12 @@
 
 public final class SentrySpanExporter implements SpanExporter {
   private volatile boolean stopped = false;
-  // TODO [POTEL] should we clear out old finished spans after a while?
   private final List<SpanData> finishedSpans = new CopyOnWriteArrayList<>();
   private final @NotNull SentryWeakSpanStorage spanStorage = SentryWeakSpanStorage.getInstance();
   private final @NotNull SpanDescriptionExtractor spanDescriptionExtractor =
       new SpanDescriptionExtractor();
   private final @NotNull IScopes scopes;
 
-  // TODO [POTEL] should we also ignore "process.command_args"
-  // (`ResourceAttributes.PROCESS_COMMAND_ARGS`)?
-  // As these are apparently so long that information that is added after it is lost
   private final @NotNull List<String> attributeKeysToRemove =
       Arrays.asList(
           InternalSemanticAttributes.IS_REMOTE_PARENT.getKey(),
@@ -205,7 +201,6 @@ private void createAndFinishSpanForOtelSpan(
             spanData.getParentSpanId());
     final @NotNull SentryDate startDate = new SentryLongDate(spanData.getStartEpochNanos());
     final @NotNull SpanOptions spanOptions = new SpanOptions();
-    // TODO [POTEL] op and description might have been overriden
     final @NotNull io.sentry.SpanContext spanContext =
         parentSentrySpan
             .getSpanContext()
@@ -219,8 +214,6 @@ private void createAndFinishSpanForOtelSpan(
       spanContext.setSamplingDecision(sentrySpanMaybe.getSamplingDecision());
       spanOptions.setOrigin(sentrySpanMaybe.getSpanContext().getOrigin());
     } else {
-      // TODO [POTEL] Check if we want to use `instrumentationScopeInfo.name` and append it to
-      // `auto.otel`
       spanOptions.setOrigin(TRACE_ORIGIN);
     }
 
@@ -258,15 +251,6 @@ private void transferSpanDetails(
         targetSpan.setData(entry.getKey(), entry.getValue());
       }
 
-      // TODO [POTEL] this is not an OtelSpanWrapper since it's created with default span factory
-      //      if (sentryChildSpan instanceof OtelSpanWrapper) {
-      //        final @NotNull OtelSpanWrapper sentryChildSpanWrapper = (OtelSpanWrapper)
-      // sentryChildSpan;
-      //        final @NotNull Map<String, MeasurementValue> measurements =
-      // sentrySpan.getMeasurements();
-      //        sentryChildSpanWrapper.addAllMeasurements(measurements);
-      //      }
-
       final @NotNull Map<String, String> tags = sourceSpan.getTags();
       for (Map.Entry<String, String> entry : tags.entrySet()) {
         targetSpan.setTag(entry.getKey(), entry.getValue());
@@ -319,7 +303,6 @@ private void transferSpanDetails(
       baggage = spanContext.getBaggage();
     }
 
-    // TODO [POTEL] parentSamplingDecision?
     final @NotNull TransactionContext transactionContext =
         new TransactionContext(new SentryId(traceId), sentrySpanId, parentSpanId, null, baggage);
 
@@ -445,7 +428,6 @@ private void createOrUpdateSpanNodeAndRefs(
   private SpanStatus mapOtelStatus(
       final @NotNull SpanData otelSpanData, final @NotNull ISpan sentrySpan) {
     final @Nullable SpanStatus existingStatus = sentrySpan.getStatus();
-    // TODO [POTEL] do we want the unknown error check here?
     if (existingStatus != null && existingStatus != SpanStatus.UNKNOWN_ERROR) {
       return existingStatus;
     }

sentry-opentelemetry/sentry-opentelemetry-extra/api/sentry-opentelemetry-extra.api

@@ -22,8 +22,6 @@ public final class io/sentry/opentelemetry/OtelSpanFactory : io/sentry/ISpanFact
 	public fun <init> ()V
 	public fun createSpan (Lio/sentry/IScopes;Lio/sentry/SpanOptions;Lio/sentry/SpanContext;Lio/sentry/ISpan;)Lio/sentry/ISpan;
 	public fun createTransaction (Lio/sentry/TransactionContext;Lio/sentry/IScopes;Lio/sentry/TransactionOptions;Lio/sentry/TransactionPerformanceCollector;)Lio/sentry/ITransaction;
-	public fun retrieveCurrentSpan (Lio/sentry/IScope;)Lio/sentry/ISpan;
-	public fun retrieveCurrentSpan (Lio/sentry/IScopes;)Lio/sentry/ISpan;
 }
 
 public final class io/sentry/opentelemetry/OtelSpanWrapper : io/sentry/ISpan {

sentry-opentelemetry/sentry-opentelemetry-extra/src/main/java/io/sentry/opentelemetry/OtelSpanFactory.java

@@ -8,7 +8,6 @@
 import io.opentelemetry.api.trace.Tracer;
 import io.opentelemetry.context.Context;
 import io.sentry.Baggage;
-import io.sentry.IScope;
 import io.sentry.IScopes;
 import io.sentry.ISpan;
 import io.sentry.ISpanFactory;
@@ -78,8 +77,6 @@ public final class OtelSpanFactory implements ISpanFactory {
       final @NotNull SpanContext spanContext) {
     final @NotNull String name = spanContext.getOperation();
     final @NotNull SpanBuilder spanBuilder = getTracer().spanBuilder(name);
-    // TODO [POTEL] If performance is disabled, can we use otel.SamplingDecision.RECORD_ONLY to
-    // still allow otel to be used for tracing
     if (parentSpan == null) {
       final @NotNull SentryId traceId = spanContext.getTraceId();
       final @Nullable SpanId parentSpanId = spanContext.getParentSpanId();
@@ -102,11 +99,6 @@ public final class OtelSpanFactory implements ISpanFactory {
         final @NotNull Span wrappedSpan = Span.wrap(otelSpanContext);
         spanBuilder.setParent(Context.root().with(wrappedSpan));
       }
-    } else {
-      if (parentSpan instanceof OtelSpanWrapper) {
-        // TODO [POTEL] retrieve context from span
-        //      spanBuilder.setParent()
-      }
     }
 
     // note: won't go through propagators
@@ -152,18 +144,6 @@ public final class OtelSpanFactory implements ISpanFactory {
     return sentrySpan;
   }
 
-  // TODO [POTEL] consider removing this method
-  @Override
-  public @Nullable ISpan retrieveCurrentSpan(IScopes scopes) {
-    return scopes.getSpan();
-  }
-
-  // TODO [POTEL] consider removing this method
-  @Override
-  public @Nullable ISpan retrieveCurrentSpan(IScope scope) {
-    return scope.getSpan();
-  }
-
   private @NotNull Tracer getTracer() {
     return GlobalOpenTelemetry.getTracer(
         "sentry-instrumentation-scope-name", "sentry-instrumentation-scope-version");

sentry-opentelemetry/sentry-opentelemetry-extra/src/main/java/io/sentry/opentelemetry/OtelSpanWrapper.java

@@ -57,18 +57,14 @@ public final class OtelSpanWrapper implements ISpan {
    * OtelSpanWrapper} and indirectly back to {@link io.opentelemetry.sdk.trace.data.SpanData} via
    * {@link Span}. Also see {@link SentryWeakSpanStorage}.
    */
-  private final @NotNull WeakReference<ReadWriteSpan> span; // TODO [POTEL] bootstrap proxy
+  private final @NotNull WeakReference<ReadWriteSpan> span;
 
   private final @NotNull SpanContext context;
-  //  private final @NotNull SpanOptions options;
   private final @NotNull Contexts contexts = new Contexts();
   private @Nullable String transactionName;
   private @Nullable TransactionNameSource transactionNameSource;
   private final @Nullable Baggage baggage;
 
-  // TODO [POTEL]
-  //  private @Nullable SpanFinishedCallback spanFinishedCallback;
-
   private final @NotNull Map<String, Object> data = new ConcurrentHashMap<>();
   private final @NotNull Map<String, MeasurementValue> measurements = new ConcurrentHashMap<>();
 
@@ -362,13 +358,6 @@ public void setMeasurement(@NotNull String name, @NotNull Number value) {
       return;
     }
     this.measurements.put(name, new MeasurementValue(value, null));
-
-    // TODO [POTEL] can't set on transaction
-    // We set the measurement in the transaction, too, but we have to check if this is the root span
-    // of the transaction, to avoid an infinite recursion
-    //    if (transaction.getRoot() != this) {
-    //      transaction.setMeasurementFromChild(name, value);
-    //    }
   }
 
   @Override
@@ -385,13 +374,6 @@ public void setMeasurement(
       return;
     }
     this.measurements.put(name, new MeasurementValue(value, unit.apiName()));
-
-    // TODO [POTEL] can't set on transaction
-    // We set the measurement in the transaction, too, but we have to check if this is the root span
-    // of the transaction, to avoid an infinite recursion
-    //    if (transaction.getRoot() != this) {
-    //      transaction.setMeasurementFromChild(name, value, unit);
-    //    }
   }
 
   @Override
@@ -430,7 +412,6 @@ public void setContext(@NotNull String key, @NotNull Object context) {
 
   @Override
   public @NotNull Contexts getContexts() {
-    // TODO [POTEL] only works for root span atm
     return contexts;
   }
 

sentry-opentelemetry/sentry-opentelemetry-extra/src/main/java/io/sentry/opentelemetry/OtelTransactionSpanForwarder.java

@@ -94,7 +94,6 @@ public OtelTransactionSpanForwarder(final @NotNull OtelSpanWrapper rootSpan) {
 
   @Override
   public void finish() {
-    // TODO [POTEL] should this finish all spans?
     rootSpan.finish();
   }
 
@@ -225,7 +224,6 @@ public boolean isNoOp() {
 
   @Override
   public @NotNull List<io.sentry.Span> getSpans() {
-    // TODO [POTEL]
     return new ArrayList<>();
   }
 
@@ -257,7 +255,6 @@ public boolean isNoOp() {
 
   @Override
   public @NotNull SentryId getEventId() {
-    // TODO [POTEL]
     return new SentryId();
   }
 
@@ -268,14 +265,11 @@ public boolean isNoOp() {
   }
 
   @Override
-  public void scheduleFinish() {
-    // TODO [POTEL]
-  }
+  public void scheduleFinish() {}
 
   @Override
   public void forceFinish(
       @NotNull SpanStatus status, boolean dropIfNoChildren, @Nullable Hint hint) {
-    // TODO [POTEL]
     rootSpan.finish(status);
   }
 
@@ -285,7 +279,6 @@ public void finish(
       @Nullable SentryDate timestamp,
       boolean dropIfNoChildren,
       @Nullable Hint hint) {
-    // TODO [POTEL]
     rootSpan.finish(status, timestamp);
   }
 

sentry/api/sentry.api

@@ -375,8 +375,6 @@ public final class io/sentry/DefaultSpanFactory : io/sentry/ISpanFactory {
 	public fun <init> ()V
 	public fun createSpan (Lio/sentry/IScopes;Lio/sentry/SpanOptions;Lio/sentry/SpanContext;Lio/sentry/ISpan;)Lio/sentry/ISpan;
 	public fun createTransaction (Lio/sentry/TransactionContext;Lio/sentry/IScopes;Lio/sentry/TransactionOptions;Lio/sentry/TransactionPerformanceCollector;)Lio/sentry/ITransaction;
-	public fun retrieveCurrentSpan (Lio/sentry/IScope;)Lio/sentry/ISpan;
-	public fun retrieveCurrentSpan (Lio/sentry/IScopes;)Lio/sentry/ISpan;
 }
 
 public final class io/sentry/DefaultTransactionPerformanceCollector : io/sentry/TransactionPerformanceCollector {
@@ -1022,8 +1020,6 @@ public abstract interface class io/sentry/ISpan {
 public abstract interface class io/sentry/ISpanFactory {
 	public abstract fun createSpan (Lio/sentry/IScopes;Lio/sentry/SpanOptions;Lio/sentry/SpanContext;Lio/sentry/ISpan;)Lio/sentry/ISpan;
 	public abstract fun createTransaction (Lio/sentry/TransactionContext;Lio/sentry/IScopes;Lio/sentry/TransactionOptions;Lio/sentry/TransactionPerformanceCollector;)Lio/sentry/ITransaction;
-	public abstract fun retrieveCurrentSpan (Lio/sentry/IScope;)Lio/sentry/ISpan;
-	public abstract fun retrieveCurrentSpan (Lio/sentry/IScopes;)Lio/sentry/ISpan;
 }
 
 public abstract interface class io/sentry/ITransaction : io/sentry/ISpan {
@@ -1675,8 +1671,6 @@ public final class io/sentry/NoOpSpanFactory : io/sentry/ISpanFactory {
 	public fun createSpan (Lio/sentry/IScopes;Lio/sentry/SpanOptions;Lio/sentry/SpanContext;Lio/sentry/ISpan;)Lio/sentry/ISpan;
 	public fun createTransaction (Lio/sentry/TransactionContext;Lio/sentry/IScopes;Lio/sentry/TransactionOptions;Lio/sentry/TransactionPerformanceCollector;)Lio/sentry/ITransaction;
 	public static fun getInstance ()Lio/sentry/NoOpSpanFactory;
-	public fun retrieveCurrentSpan (Lio/sentry/IScope;)Lio/sentry/ISpan;
-	public fun retrieveCurrentSpan (Lio/sentry/IScopes;)Lio/sentry/ISpan;
 }
 
 public final class io/sentry/NoOpTransaction : io/sentry/ITransaction {

sentry/src/main/java/io/sentry/CombinedScopeView.java

@@ -150,7 +150,6 @@ public void setRequest(@Nullable Request request) {
 
   @Override
   public @NotNull List<String> getFingerprint() {
-    // TODO [HSM] should these be merged?
     final @Nullable List<String> current = scope.getFingerprint();
     if (!current.isEmpty()) {
       return current;

sentry/src/main/java/io/sentry/DefaultSpanFactory.java

@@ -22,19 +22,8 @@ public final class DefaultSpanFactory implements ISpanFactory {
       final @NotNull SpanContext spanContext,
       @Nullable ISpan parentSpan) {
     if (parentSpan == null) {
-      // TODO [POTEL] We could create a transaction here
       return NoOpSpan.getInstance();
     }
     return parentSpan.startChild(spanContext, spanOptions);
   }
-
-  @Override
-  public @Nullable ISpan retrieveCurrentSpan(final IScopes scopes) {
-    return scopes.getSpan();
-  }
-
-  @Override
-  public @Nullable ISpan retrieveCurrentSpan(final IScope scope) {
-    return scope.getSpan();
-  }
 }

sentry/src/main/java/io/sentry/ISpanFactory.java

@@ -19,10 +19,4 @@ ISpan createSpan(
       @NotNull SpanOptions spanOptions,
       @NotNull SpanContext spanContext,
       @Nullable ISpan parentSpan);
-
-  @Nullable
-  ISpan retrieveCurrentSpan(IScopes scopes);
-
-  @Nullable
-  ISpan retrieveCurrentSpan(IScope scope);
 }

sentry/src/main/java/io/sentry/NoOpSpanFactory.java

@@ -32,14 +32,4 @@ public static NoOpSpanFactory getInstance() {
       @Nullable ISpan parentSpan) {
     return NoOpSpan.getInstance();
   }
-
-  @Override
-  public @Nullable ISpan retrieveCurrentSpan(IScopes scopes) {
-    return NoOpSpan.getInstance();
-  }
-
-  @Override
-  public @Nullable ISpan retrieveCurrentSpan(IScope scope) {
-    return NoOpSpan.getInstance();
-  }
 }

sentry/src/main/java/io/sentry/Scopes.java

@@ -827,8 +827,6 @@ public void flush(long timeoutMillis) {
       final @NotNull TransactionContext transactionContext,
       final @NotNull TransactionOptions transactionOptions) {
     Objects.requireNonNull(transactionContext, "transactionContext is required");
-    // TODO [POTEL] what if span is already running and someone calls startTransaction?
-
     transactionContext.setOrigin(transactionOptions.getOrigin());
 
     ITransaction transaction;
@@ -841,7 +839,6 @@ public void flush(long timeoutMillis) {
       transaction = NoOpTransaction.getInstance();
     } else if (SpanUtils.isIgnored(
         getOptions().getIgnoredSpanOrigins(), transactionContext.getOrigin())) {
-      // TODO [POTEL] may not have been set yet?
       getOptions()
           .getLogger()
           .log(
@@ -926,7 +923,7 @@ public void setSpanContext(
           .getLogger()
           .log(SentryLevel.WARNING, "Instance is disabled and this 'getSpan' call is a no-op.");
     } else {
-      return getOptions().getSpanFactory().retrieveCurrentSpan(getCombinedScopeView());
+      return getCombinedScopeView().getSpan();
     }
     return null;
   }

sentry/src/main/java/io/sentry/Sentry.java

@@ -1046,10 +1046,7 @@ public static void endSession() {
     if (globalHubMode && Platform.isAndroid()) {
       return getCurrentScopes().getTransaction();
     } else {
-      return getCurrentScopes()
-          .getOptions()
-          .getSpanFactory()
-          .retrieveCurrentSpan(getCurrentScopes());
+      return getCurrentScopes().getSpan();
     }
   }
 

sentry/src/main/java/io/sentry/SentryClient.java

@@ -139,7 +139,6 @@ private boolean shouldApplyScopeData(final @NotNull CheckIn event, final @NotNul
       }
     }
 
-    // TODO [HSM] EventProcessors from options are always executed after those from scopes
     event = processEvent(event, hint, options.getEventProcessors());
 
     if (event != null) {

sentry/src/main/java/io/sentry/SentryOptions.java

@@ -2803,7 +2803,6 @@ private void addPackageInfo() {
 
   @ApiStatus.Internal
   public @NotNull ISpanFactory getSpanFactory() {
-    // TODO [POTEL] use a util for checking if OTel is active or similar
     return spanFactory;
   }