Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions src/Sentry/IScopeObserver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ public interface IScopeObserver
/// </summary>
public void SetTrace(SentryId traceId, SpanId parentSpanId);

/// <summary>
/// Sets the environment.
/// </summary>
Comment thread
sentry-warden[bot] marked this conversation as resolved.
public void SetEnvironment(string? environment);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thought: interface vs abstract class

Making the ScopeObserver an interface, rather than an abstract class, is a design decision that is haunting us now: we continuously add new (non-default) members to it in minor versions, which is breaking user code when implementing their own ScopeObserver ... arguably the ScopeObserver it a more niche use case .. but source breaking nonetheless.

I wonder if an abstract class would have alleviated this problem, with which we could add new virtual members with an empty implementation, that user code might then override, but does not have to.

Changing this interface into an abstract class now would be even more unexpectedly breaking. But something to consider for every public interface that we expose in the future.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We could make the change in the next major version... I think it's a good suggestion. We can mock/substitute abstract classes just as well as interfaces.

@bitsandfoxes bitsandfoxes Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I understand the "technically source breaking" argument but we have this discussion every time I make changes to the interface. The thing should go being internal in the next major and we'd be done with it.

For what it's worth we're not really breaking user code. This is a hypothetical. The ScopeObserver is public because when we added it we had no other mechanism to make it available to sentry-unity and not because it's some load-bearing functionality we're expecting users to use in the first place.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can / should we make IScopeObserver internal,
since we're exposing this feature to user code:

/// <summary>
/// A scope set outside of Sentry SDK. If set, the global parameters from the SDK's scope will be sent to the observed scope.<br/>
/// NOTE: EnableScopeSync must be set true for the scope to be synced.
/// </summary>
public IScopeObserver? ScopeObserver { get; set; }
/// <summary>
/// If true, the SDK's scope will be synced with the observed scope.
/// </summary>
public bool EnableScopeSync { get; set; }

Users might today already provide their own Scope-Observer.

The AndroidScopeObserver and CocoaScopeObserver do wrap any Options/User-provided IScopeObserver.
But the NativeScopeObserver does not, and just overwrites any user-provided IScopeObserver.

So we do have a bit of an incomplete story here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The wrapping is happening because you guys asked me to respect any potentially added ScopeObserver since it is public. Fair enough. But that should not be a precedence of us having to support this or not change it. We have a very limited set of hooks that we want to support on the SDK, right? Where users are supposed to filter, mutate or drop data/events, i.e. in Init and all those Before_X callbacks, the X_Samplers. The ScopeObserver is an outlier built out of necessity.
Like what problem is this supposed to solve for the user? How is the user supposed to use this? They want to receive a callback when they are setting something (but not everything) on the scope?

We can continue weighing the pros and cons of dropping it in #4529

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sounds good ... I actually didn't quite intend to spawn off a heavy discussion on this ... just wanted to note, that for the next public interface we design, we should consider making it an abstract class if it's going to "evolve"


/// <summary>
/// Adds an attachment.
/// </summary>
Expand Down
9 changes: 9 additions & 0 deletions src/Sentry/Internal/ScopeObserver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,15 @@ public void SetTrace(SentryId traceId, SpanId parentSpanId)

public abstract void SetTraceImpl(SentryId traceId, SpanId parentSpanId);

public void SetEnvironment(string? environment)
{
_options.DiagnosticLogger?.Log(SentryLevel.Debug,
"{0} Scope Sync - Setting Environment e:\"{1}\"", null, _name, environment);
SetEnvironmentImpl(environment);
}

public abstract void SetEnvironmentImpl(string? environment);

public void AddAttachment(SentryAttachment attachment)
{
_options.DiagnosticLogger?.Log(SentryLevel.Debug,
Expand Down
12 changes: 12 additions & 0 deletions src/Sentry/Platforms/Android/AndroidScopeObserver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,18 @@ public void SetTrace(SentryId traceId, SpanId parentSpanId)
}
}

public void SetEnvironment(string? environment)
{
try
{
// TODO: Missing corresponding scope-level functionality on the Android SDK
}
finally
{
_innerObserver?.SetEnvironment(environment);
}
}

public void AddAttachment(SentryAttachment attachment)
{
try
Expand Down
12 changes: 12 additions & 0 deletions src/Sentry/Platforms/Cocoa/CocoaScopeObserver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,18 @@ public void SetTrace(SentryId traceId, SpanId parentSpanId)
}
}

public void SetEnvironment(string? environment)
{
try
{
SentryCocoaSdk.ConfigureScope(scope => scope.SetEnvironment(environment));
}
finally
{
_innerObserver?.SetEnvironment(environment);
}
}

public void AddAttachment(SentryAttachment attachment)
{
try
Expand Down
3 changes: 3 additions & 0 deletions src/Sentry/Platforms/Native/CFunctions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,9 @@ internal static string GetCacheDirectory(SentryOptions options)
[DllImport("sentry-native")]
internal static extern void sentry_set_trace(string traceId, string parentSpanId);

[DllImport("sentry-native")]
internal static extern void sentry_set_environment(string? environment);

Comment thread
bitsandfoxes marked this conversation as resolved.
internal static Dictionary<long, DebugImage> LoadDebugImages(IDiagnosticLogger? logger)
{
// It only makes sense to load them once because they're cached on the native side anyway. We could force
Expand Down
3 changes: 3 additions & 0 deletions src/Sentry/Platforms/Native/NativeScopeObserver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ public override void SetUserImpl(SentryUser user)
public override void SetTraceImpl(SentryId traceId, SpanId parentSpanId) =>
C.sentry_set_trace(traceId.ToString(), parentSpanId.ToString());

public override void SetEnvironmentImpl(string? environment) =>
C.sentry_set_environment(environment);

public override void AddAttachmentImpl(SentryAttachment attachment)
{
// TODO: Missing corresponding functionality on the Native SDK
Expand Down
33 changes: 31 additions & 2 deletions src/Sentry/Scope.cs
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,35 @@ public SentryUser User
/// <inheritdoc />
public string? Distribution { get; set; }

private string? _environment;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nitpick: could use field keyword instead of explicit backing field

If we don't use the _environment backing field anywhere outside of the Environment property accessors, with C# 14.0, we could remove that explicit backing field, and use "field backed properties" instead.

See https://learn.microsoft.com/dotnet/csharp/whats-new/csharp-14#the-field-keyword


/// <inheritdoc />
public string? Environment { get; set; }
public string? Environment
{
get => _environment;
set
{
if (_environment == value)
{
return;
}

if (value is null)
{
Options.LogWarning("Environment cannot be null. Reverting to default value from the options.");
_environment = Options.Environment;
}
else
{
_environment = value;
}

if (Options is { EnableScopeSync: true, ScopeObserver: { } observer })
{
observer.SetEnvironment(_environment);
}
}
}
Comment thread
bitsandfoxes marked this conversation as resolved.

// TransactionName is kept for legacy purposes because
// SentryEvent still makes use of it.
Expand Down Expand Up @@ -288,6 +315,8 @@ internal Scope(SentryOptions? options, SentryPropagationContext? propagationCont
{
Options = options ?? new SentryOptions();
PropagationContext = new SentryPropagationContext(propagationContext);

_environment = Options.Environment;
}

// For testing. Should explicitly require SentryOptions.
Expand Down Expand Up @@ -406,7 +435,7 @@ public void Clear()
User = new();
Release = default;
Distribution = default;
Environment = default;
Environment = Options.Environment; // Restore to keep in sync with native
TransactionName = default;
Transaction = default;
Fingerprint = Array.Empty<string>();
Expand Down
3 changes: 3 additions & 0 deletions src/Sentry/SentrySdk.cs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ internal static IHub InitHub(SentryOptions options)
#pragma warning restore 0162
#pragma warning restore CS0162 // Unreachable code detected

// This happens before the native SDKs get initialized
options.Environment = options.SettingLocator.GetEnvironment();

// Initialize native platform SDKs here
if (options.InitNativeSdks)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ namespace Sentry
void AddAttachment(Sentry.SentryAttachment attachment);
void AddBreadcrumb(Sentry.Breadcrumb breadcrumb);
void ClearAttachments();
void SetEnvironment(string? environment);
void SetExtra(string key, object? value);
void SetTag(string key, string value);
void SetTrace(Sentry.SentryId traceId, Sentry.SpanId parentSpanId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ namespace Sentry
void AddAttachment(Sentry.SentryAttachment attachment);
void AddBreadcrumb(Sentry.Breadcrumb breadcrumb);
void ClearAttachments();
void SetEnvironment(string? environment);
void SetExtra(string key, object? value);
void SetTag(string key, string value);
void SetTrace(Sentry.SentryId traceId, Sentry.SpanId parentSpanId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ namespace Sentry
void AddAttachment(Sentry.SentryAttachment attachment);
void AddBreadcrumb(Sentry.Breadcrumb breadcrumb);
void ClearAttachments();
void SetEnvironment(string? environment);
void SetExtra(string key, object? value);
void SetTag(string key, string value);
void SetTrace(Sentry.SentryId traceId, Sentry.SpanId parentSpanId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ namespace Sentry
void AddAttachment(Sentry.SentryAttachment attachment);
void AddBreadcrumb(Sentry.Breadcrumb breadcrumb);
void ClearAttachments();
void SetEnvironment(string? environment);
void SetExtra(string key, object? value);
void SetTag(string key, string value);
void SetTrace(Sentry.SentryId traceId, Sentry.SpanId parentSpanId);
Expand Down Expand Up @@ -2029,4 +2030,4 @@ public static class SentryExceptionExtensions
public static void AddSentryContext(this System.Exception ex, string name, System.Collections.Generic.IReadOnlyDictionary<string, object> data) { }
public static void AddSentryTag(this System.Exception ex, string name, string value) { }
public static void SetSentryMechanism(this System.Exception ex, string type, string? description = null, bool? handled = default, bool? terminal = default) { }
}
}
76 changes: 76 additions & 0 deletions test/Sentry.Tests/ScopeTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -696,6 +696,82 @@ public void AddBreadcrumb_ObserverExist_ObserverAddsBreadcrumbIfEnabled(bool obs
observer.Received(expectedCount).AddBreadcrumb(Arg.Is(breadcrumb));
}

[Theory]
[InlineData(true)]
[InlineData(false)]
public void SetEnvironment_ObserverExist_ObserverSetsEnvironmentIfEnabled(bool observerEnable)
{
// Arrange
var observer = Substitute.For<IScopeObserver>();
var scope = new Scope(new SentryOptions
{
ScopeObserver = observer,
EnableScopeSync = observerEnable
});
const string expectedEnvironment = "staging";
var expectedCount = observerEnable ? 1 : 0;

// Act
scope.Environment = expectedEnvironment;

// Assert
observer.Received(expectedCount).SetEnvironment(Arg.Is(expectedEnvironment));
}

[Fact]
public void SetEnvironment_Null_EnvironmentSetToOptionEnvironment()
{
// Arrange
const string expectedEnvironment = "staging";
var scope = new Scope(new SentryOptions { Environment = expectedEnvironment });

// Act
scope.Environment = null;

// Assert
scope.Environment.Should().Be(expectedEnvironment);
}

[Fact]
public void SetEnvironment_Null_ObserverReceivesOptionEnvironment()
{
// Arrange
const string expectedEnvironment = "staging";
var observer = Substitute.For<IScopeObserver>();
var scope = new Scope(new SentryOptions
{
ScopeObserver = observer,
EnableScopeSync = true,
Environment = expectedEnvironment
});
observer.ClearReceivedCalls();

// Act
scope.Environment = null;

// Assert
observer.Received(1).SetEnvironment(Arg.Is(expectedEnvironment));
}

[Fact]
public void SetEnvironment_SameValue_ObserverNotifiedOnce()
{
// Arrange
var observer = Substitute.For<IScopeObserver>();
var scope = new Scope(new SentryOptions
{
ScopeObserver = observer,
EnableScopeSync = true
});

// Act
scope.Environment = "staging";
scope.Environment = "staging";

// Assert
observer.Received(1).SetEnvironment(Arg.Is("staging"));
}

[Fact]
public void Filtered_tags_are_not_set()
{
Expand Down
Loading