Skip to content

Hubs / Scopes Merge 16 - Move client and throwable to span map to scope #3318

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.

Sign up for GitHub

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

Merged
merged 20 commits into from
Apr 19, 2024
Merged

Hubs / Scopes Merge 16 - Move client and throwable to span map to scope #3318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
305baf5
replace hub with scopes
adinauer Mar 27, 2024
95f5e1b
Add Scopes
adinauer Apr 2, 2024
27f2398
Introduce `IScopes` interface.
adinauer Apr 2, 2024
ce3c14f
Replace `IHub` with `IScopes` in core
adinauer Apr 2, 2024
ce615f4
Replace `IHub` with `IScopes` in android core
adinauer Apr 2, 2024
22ddc00
Replace `IHub` with `IScopes` in android integrations
adinauer Apr 2, 2024
305c217
Replace `IHub` with `IScopes` in apollo integrations
adinauer Apr 2, 2024
da927bc
Replace `IHub` with `IScopes` in okhttp integration
adinauer Apr 2, 2024
8279276
Replace `IHub` with `IScopes` in graphql integration
adinauer Apr 2, 2024
9bfc086
Replace `IHub` with `IScopes` in logging integrations
adinauer Apr 2, 2024
b998e50
Replace `IHub` with `IScopes` in more integrations
adinauer Apr 2, 2024
739827a
Replace `IHub` with `IScopes` in OTel integration
adinauer Apr 2, 2024
69f2d63
Replace `IHub` with `IScopes` in Spring 5 / Spring Boot 2 integrations
adinauer Apr 2, 2024
792d482
Replace `IHub` with `IScopes` in Spring 6 / Spring Boot 3 integrations
adinauer Apr 2, 2024
9bcbce6
Replace `IHub` with `IScopes` in samples
adinauer Apr 2, 2024
3f25a4b
Merge branch 'feat/hsm-13-replacements-in-samples' into feat/hubs-sco…
adinauer Apr 2, 2024
d6fb40a
gitscopes -> github
adinauer Apr 2, 2024
7752bcc
Replace ThreadLocal with ScopesStorage
adinauer Apr 4, 2024
1e329c5
Move client and throwable to span map to scope
adinauer Apr 4, 2024
eeac7f3
Merge branch '8.x.x' into feat/hsm-16-move-client-span-map
adinauer Apr 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions sentry/api/sentry.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,6 +660,8 @@ public abstract interface class io/sentry/IScope {
public abstract fun addBreadcrumb (Lio/sentry/Breadcrumb;)V
public abstract fun addBreadcrumb (Lio/sentry/Breadcrumb;Lio/sentry/Hint;)V
public abstract fun addEventProcessor (Lio/sentry/EventProcessor;)V
public abstract fun assignTraceContext (Lio/sentry/SentryEvent;)V
public abstract fun bindClient (Lio/sentry/ISentryClient;)V
public abstract fun clear ()V
public abstract fun clearAttachments ()V
public abstract fun clearBreadcrumbs ()V
Expand Down Expand Up @@ -703,6 +705,7 @@ public abstract interface class io/sentry/IScope {
public abstract fun setPropagationContext (Lio/sentry/PropagationContext;)V
public abstract fun setRequest (Lio/sentry/protocol/Request;)V
public abstract fun setScreen (Ljava/lang/String;)V
public abstract fun setSpanContext (Ljava/lang/Throwable;Lio/sentry/ISpan;Ljava/lang/String;)V
public abstract fun setTag (Ljava/lang/String;Ljava/lang/String;)V
public abstract fun setTransaction (Lio/sentry/ITransaction;)V
public abstract fun setTransaction (Ljava/lang/String;)V
Expand Down Expand Up @@ -1298,6 +1301,8 @@ public final class io/sentry/NoOpScope : io/sentry/IScope {
public fun addBreadcrumb (Lio/sentry/Breadcrumb;)V
public fun addBreadcrumb (Lio/sentry/Breadcrumb;Lio/sentry/Hint;)V
public fun addEventProcessor (Lio/sentry/EventProcessor;)V
public fun assignTraceContext (Lio/sentry/SentryEvent;)V
public fun bindClient (Lio/sentry/ISentryClient;)V
public fun clear ()V
public fun clearAttachments ()V
public fun clearBreadcrumbs ()V
Expand Down Expand Up @@ -1343,6 +1348,7 @@ public final class io/sentry/NoOpScope : io/sentry/IScope {
public fun setPropagationContext (Lio/sentry/PropagationContext;)V
public fun setRequest (Lio/sentry/protocol/Request;)V
public fun setScreen (Ljava/lang/String;)V
public fun setSpanContext (Ljava/lang/Throwable;Lio/sentry/ISpan;Ljava/lang/String;)V
public fun setTag (Ljava/lang/String;Ljava/lang/String;)V
public fun setTransaction (Lio/sentry/ITransaction;)V
public fun setTransaction (Ljava/lang/String;)V
Expand Down Expand Up @@ -1731,6 +1737,8 @@ public final class io/sentry/Scope : io/sentry/IScope {
public fun addBreadcrumb (Lio/sentry/Breadcrumb;)V
public fun addBreadcrumb (Lio/sentry/Breadcrumb;Lio/sentry/Hint;)V
public fun addEventProcessor (Lio/sentry/EventProcessor;)V
public fun assignTraceContext (Lio/sentry/SentryEvent;)V
public fun bindClient (Lio/sentry/ISentryClient;)V
public fun clear ()V
public fun clearAttachments ()V
public fun clearBreadcrumbs ()V
Expand Down Expand Up @@ -1775,6 +1783,7 @@ public final class io/sentry/Scope : io/sentry/IScope {
public fun setPropagationContext (Lio/sentry/PropagationContext;)V
public fun setRequest (Lio/sentry/protocol/Request;)V
public fun setScreen (Ljava/lang/String;)V
public fun setSpanContext (Ljava/lang/Throwable;Lio/sentry/ISpan;Ljava/lang/String;)V
public fun setTag (Ljava/lang/String;Ljava/lang/String;)V
public fun setTransaction (Lio/sentry/ITransaction;)V
public fun setTransaction (Ljava/lang/String;)V
Expand Down
11 changes: 10 additions & 1 deletion sentry/src/main/java/io/sentry/IScope.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,8 +377,17 @@ public interface IScope {
@NotNull
SentryId getLastEventId();

void setClient(final @NotNull ISentryClient client);
void bindClient(final @NotNull ISentryClient client);

@NotNull
ISentryClient getClient();

@ApiStatus.Internal
void assignTraceContext(final @NotNull SentryEvent event);

@ApiStatus.Internal
void setSpanContext(
final @NotNull Throwable throwable,
final @NotNull ISpan span,
final @NotNull String transactionName);
}
9 changes: 8 additions & 1 deletion sentry/src/main/java/io/sentry/NoOpScope.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,10 +256,17 @@ public void setLastEventId(@NotNull SentryId lastEventId) {}
}

@Override
public void setClient(@NotNull ISentryClient client) {}
public void bindClient(@NotNull ISentryClient client) {}

@Override
public @NotNull ISentryClient getClient() {
return NoOpSentryClient.getInstance();
}

@Override
public void assignTraceContext(@NotNull SentryEvent event) {}

@Override
public void setSpanContext(
@NotNull Throwable throwable, @NotNull ISpan span, @NotNull String transactionName) {}
}
52 changes: 51 additions & 1 deletion sentry/src/main/java/io/sentry/Scope.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,18 @@
import io.sentry.protocol.TransactionNameSource;
import io.sentry.protocol.User;
import io.sentry.util.CollectionUtils;
import io.sentry.util.ExceptionUtils;
import io.sentry.util.Objects;
import io.sentry.util.Pair;
import java.lang.ref.WeakReference;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Queue;
import java.util.WeakHashMap;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CopyOnWriteArrayList;
import org.jetbrains.annotations.ApiStatus;
Expand Down Expand Up @@ -85,6 +90,11 @@ public final class Scope implements IScope {

private @NotNull ISentryClient client = NoOpSentryClient.getInstance();

// TODO intended only for global scope
// TODO test for memory leak
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs testing once enough changes have been implemented to check for memory leaks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, we might have to remove the entries that have been sent to Sentry from the map.

But this would have been an issue with Hub as well I think.

private final @NotNull Map<Throwable, Pair<WeakReference<ISpan>, String>> throwableToSpan =
Collections.synchronizedMap(new WeakHashMap<>());

/**
* Scope's ctor
*
Expand All @@ -103,6 +113,7 @@ private Scope(final @NotNull Scope scope) {
this.session = scope.session;
this.options = scope.options;
this.level = scope.level;
this.client = scope.client;
// TODO should we do this? didn't do it for Hub
this.lastEventId = scope.getLastEventId();

Expand Down Expand Up @@ -964,7 +975,7 @@ public void setLastEventId(@NotNull SentryId lastEventId) {
}

@Override
public void setClient(@NotNull ISentryClient client) {
public void bindClient(@NotNull ISentryClient client) {
this.client = client;
}

Expand All @@ -973,6 +984,45 @@ public void setClient(@NotNull ISentryClient client) {
return client;
}

@Override
@ApiStatus.Internal
public void assignTraceContext(final @NotNull SentryEvent event) {
if (options.isTracingEnabled() && event.getThrowable() != null) {
final Pair<WeakReference<ISpan>, String> pair =
throwableToSpan.get(ExceptionUtils.findRootCause(event.getThrowable()));
if (pair != null) {
final WeakReference<ISpan> spanWeakRef = pair.getFirst();
if (event.getContexts().getTrace() == null && spanWeakRef != null) {
final ISpan span = spanWeakRef.get();
if (span != null) {
event.getContexts().setTrace(span.getSpanContext());
}
}
final String transactionName = pair.getSecond();
if (event.getTransaction() == null && transactionName != null) {
event.setTransaction(transactionName);
}
}
}
}

@Override
@ApiStatus.Internal
public void setSpanContext(
final @NotNull Throwable throwable,
final @NotNull ISpan span,
final @NotNull String transactionName) {
Objects.requireNonNull(throwable, "throwable is required");
Objects.requireNonNull(span, "span is required");
Objects.requireNonNull(transactionName, "transactionName is required");
// to match any cause, span context is always attached to the root cause of the exception
final Throwable rootCause = ExceptionUtils.findRootCause(throwable);
// the most inner span should be assigned to a throwable
if (!throwableToSpan.containsKey(rootCause)) {
throwableToSpan.put(rootCause, new Pair<>(new WeakReference<>(span), transactionName));
}
}

/** The IWithTransaction callback */
@ApiStatus.Internal
public interface IWithTransaction {
Expand Down
83 changes: 27 additions & 56 deletions sentry/src/main/java/io/sentry/Scopes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,15 @@
import io.sentry.protocol.SentryTransaction;
import io.sentry.protocol.User;
import io.sentry.transport.RateLimiter;
import io.sentry.util.ExceptionUtils;
import io.sentry.util.HintUtils;
import io.sentry.util.Objects;
import io.sentry.util.Pair;
import io.sentry.util.TracingUtils;
import java.io.Closeable;
import java.io.IOException;
import java.lang.ref.WeakReference;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.WeakHashMap;
import org.jetbrains.annotations.ApiStatus;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
Expand All @@ -40,10 +36,6 @@ public final class Scopes implements IScopes, MetricsApi.IMetricsInterface {
private final @NotNull SentryOptions options;
private volatile boolean isEnabled;
private final @NotNull TracesSampler tracesSampler;

Copy link
Member Author

Choose a reason for hiding this comment

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

Having a separate map per Scopes will break the feature since it's forked more often than hub used to be (at least for non mobile).

// TODO should this go on global scope?
private final @NotNull Map<Throwable, Pair<WeakReference<ISpan>, String>> throwableToSpan =
Collections.synchronizedMap(new WeakHashMap<>());
private final @NotNull TransactionPerformanceCollector transactionPerformanceCollector;
private final @NotNull MetricsApi metricsApi;

Expand Down Expand Up @@ -120,7 +112,10 @@ public boolean isAncestorOf(final @Nullable Scopes otherScopes) {

// TODO add to IScopes interface
public @NotNull Scopes forkedCurrentScope(final @NotNull String creator) {
return new Scopes(scope.clone(), isolationScope, this, options, creator);
IScope clone = scope.clone();
// TODO should use isolation scope
// return new Scopes(clone, isolationScope, this, options, creator);
return new Scopes(clone, clone, this, options, creator);
}

// // TODO in Sentry.init?
Expand Down Expand Up @@ -180,23 +175,7 @@ public boolean isEnabled() {
}

private void assignTraceContext(final @NotNull SentryEvent event) {
if (options.isTracingEnabled() && event.getThrowable() != null) {
final Pair<WeakReference<ISpan>, String> pair =
throwableToSpan.get(ExceptionUtils.findRootCause(event.getThrowable()));
if (pair != null) {
final WeakReference<ISpan> spanWeakRef = pair.getFirst();
if (event.getContexts().getTrace() == null && spanWeakRef != null) {
final ISpan span = spanWeakRef.get();
if (span != null) {
event.getContexts().setTrace(span.getSpanContext());
}
}
final String transactionName = pair.getSecond();
if (event.getTransaction() == null && transactionName != null) {
event.setTransaction(transactionName);
}
}
}
Sentry.getGlobalScope().assignTraceContext(event);
}

private IScope buildLocalScope(
Expand Down Expand Up @@ -691,10 +670,10 @@ public void bindClient(final @NotNull ISentryClient client) {
} else {
if (client != null) {
options.getLogger().log(SentryLevel.DEBUG, "New client bound to scope.");
getDefaultWriteScope().setClient(client);
getDefaultWriteScope().bindClient(client);
} else {
options.getLogger().log(SentryLevel.DEBUG, "NoOp client bound to scope.");
getDefaultWriteScope().setClient(NoOpSentryClient.getInstance());
getDefaultWriteScope().bindClient(NoOpSentryClient.getInstance());
}
}
}
Expand Down Expand Up @@ -871,34 +850,26 @@ public void setSpanContext(
final @NotNull Throwable throwable,
final @NotNull ISpan span,
final @NotNull String transactionName) {
Objects.requireNonNull(throwable, "throwable is required");
Objects.requireNonNull(span, "span is required");
Objects.requireNonNull(transactionName, "transactionName is required");
// to match any cause, span context is always attached to the root cause of the exception
final Throwable rootCause = ExceptionUtils.findRootCause(throwable);
// the most inner span should be assigned to a throwable
if (!throwableToSpan.containsKey(rootCause)) {
throwableToSpan.put(rootCause, new Pair<>(new WeakReference<>(span), transactionName));
}
}

// TODO this seems unused
@Nullable
SpanContext getSpanContext(final @NotNull Throwable throwable) {
Objects.requireNonNull(throwable, "throwable is required");
final Throwable rootCause = ExceptionUtils.findRootCause(throwable);
final Pair<WeakReference<ISpan>, String> pair = this.throwableToSpan.get(rootCause);
if (pair != null) {
final WeakReference<ISpan> spanWeakRef = pair.getFirst();
if (spanWeakRef != null) {
final ISpan span = spanWeakRef.get();
if (span != null) {
return span.getSpanContext();
}
}
}
return null;
}
Sentry.getGlobalScope().setSpanContext(throwable, span, transactionName);
}

// // TODO this seems unused
// @Nullable
// SpanContext getSpanContext(final @NotNull Throwable throwable) {
// Objects.requireNonNull(throwable, "throwable is required");
// final Throwable rootCause = ExceptionUtils.findRootCause(throwable);
// final Pair<WeakReference<ISpan>, String> pair = this.throwableToSpan.get(rootCause);
// if (pair != null) {
// final WeakReference<ISpan> spanWeakRef = pair.getFirst();
// if (spanWeakRef != null) {
// final ISpan span = spanWeakRef.get();
// if (span != null) {
// return span.getSpanContext();
// }
// }
// }
// return null;
// }

@Override
public @Nullable ISpan getSpan() {
Expand Down