-
-
Notifications
You must be signed in to change notification settings - Fork 230
feat: User.Id can now be overriden (set to null) in Global mode #5039
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.
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
28e9845
1b15d0f
7b2e38d
7a7d0f6
9907a6c
2663ae0
abca943
8c2d66b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| namespace Sentry.Integrations; | ||
|
|
||
| internal class GlobalRootScopeIntegration : ISdkIntegration | ||
| { | ||
| public void Register(IHub hub, SentryOptions options) | ||
| { | ||
| if (!options.IsGlobalModeEnabled) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| hub.ConfigureScope(scope => scope.User.Id ??= options.InstallationId); | ||
| } | ||
jamescrosswell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,7 +12,7 @@ internal class Enricher | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| private readonly Lazy<Runtime> _runtimeLazy = new(() => | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| var current = PlatformAbstractions.SentryRuntime.Current; | ||||||||||||||||||||||
| var current = SentryRuntime.Current; | ||||||||||||||||||||||
| return new Runtime | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| Name = current.Name, | ||||||||||||||||||||||
|
|
@@ -36,7 +36,7 @@ public void Apply(IEventLike eventLike) | |||||||||||||||||||||
| if (!eventLike.Contexts.ContainsKey(OperatingSystem.Type)) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| // RuntimeInformation.OSDescription is throwing on Mono 5.12 | ||||||||||||||||||||||
| if (!PlatformAbstractions.SentryRuntime.Current.IsMono()) | ||||||||||||||||||||||
| if (!SentryRuntime.Current.IsMono()) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| #if NETFRAMEWORK | ||||||||||||||||||||||
| // RuntimeInformation.* throws on .NET Framework on macOS/Linux | ||||||||||||||||||||||
|
|
@@ -58,9 +58,8 @@ public void Apply(IEventLike eventLike) | |||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // SDK | ||||||||||||||||||||||
| // SDK Name/Version might have be already set by an outer package | ||||||||||||||||||||||
| // e.g: ASP.NET Core can set itself as the SDK | ||||||||||||||||||||||
| // e.g.: ASP.NET Core can set itself as the SDK | ||||||||||||||||||||||
| if (eventLike.Sdk.Version is null && eventLike.Sdk.Name is null) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| eventLike.Sdk.Name = Constants.SdkName; | ||||||||||||||||||||||
|
|
@@ -92,7 +91,12 @@ public void Apply(IEventLike eventLike) | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| eventLike.User.IpAddress ??= DefaultIpAddress; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| eventLike.User.Id ??= _options.InstallationId; | ||||||||||||||||||||||
| // Set by the GlobalRootScopeIntegration In global mode so that it can be overridden by the user. | ||||||||||||||||||||||
jamescrosswell marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||
| // In non-global mode (e.g. ASP.NET Core) the enricher sets it here as a fallback. | ||||||||||||||||||||||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Flash0ver arguably we could remove this code entirely... not sure if we actually want to set the same user id for all ASP.NET requests (where no user was logged in). @bruno-garcia any thoughts from your side? |
||||||||||||||||||||||
| if (!_options.IsGlobalModeEnabled) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| eventLike.User.Id ??= _options.InstallationId; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
||||||||||||||||||||||
| // Set by the GlobalRootScopeIntegration In global mode so that it can be overridden by the user. | |
| // In non-global mode (e.g. ASP.NET Core) the enricher sets it here as a fallback. | |
| if (!_options.IsGlobalModeEnabled) | |
| { | |
| eventLike.User.Id ??= _options.InstallationId; | |
| } | |
| // User.Id can be set by the GlobalRootScopeIntegration in global mode or by user code. | |
| // The enricher still provides the InstallationId-based fallback here, but it will not override | |
| // any value that was already set. | |
| eventLike.User.Id ??= _options.InstallationId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's a scenario where someone uses a SentryClient independent of the Hub... so this isn't a real concern.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -220,6 +220,11 @@ internal IEnumerable<ISdkIntegration> Integrations | |
| } | ||
| #endif | ||
|
|
||
| if ((_defaultIntegrations & DefaultIntegrations.GlobalRootScopeIntegration) != 0) | ||
| { | ||
| yield return new GlobalRootScopeIntegration(); | ||
| } | ||
|
|
||
| foreach (var integration in _integrations) | ||
| { | ||
| yield return integration; | ||
|
|
@@ -1362,6 +1367,7 @@ public SentryOptions() | |
| #if NET8_0_OR_GREATER | ||
| | DefaultIntegrations.SystemDiagnosticsMetricsIntegration | ||
| #endif | ||
| | DefaultIntegrations.GlobalRootScopeIntegration | ||
| ; | ||
|
Comment on lines
1367
to
1371
|
||
|
|
||
| #if ANDROID | ||
|
|
@@ -1829,6 +1835,7 @@ internal enum DefaultIntegrations | |
| #if NET8_0_OR_GREATER | ||
| SystemDiagnosticsMetricsIntegration = 1 << 7, | ||
| #endif | ||
| GlobalRootScopeIntegration = 1 << 8, | ||
| } | ||
|
|
||
| internal void SetupLogging() | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,145 @@ | ||||||||||||||||
| using Sentry.Integrations; | ||||||||||||||||
|
|
||||||||||||||||
| namespace Sentry.Tests.Integrations; | ||||||||||||||||
|
|
||||||||||||||||
| public class GlobalRootScopeIntegrationTests | ||||||||||||||||
| { | ||||||||||||||||
| [Fact] | ||||||||||||||||
| public void Register_GlobalModeEnabled_SetsInstallationIdOnRootScope() | ||||||||||||||||
| { | ||||||||||||||||
| // Arrange | ||||||||||||||||
| var options = new SentryOptions | ||||||||||||||||
| { | ||||||||||||||||
| Dsn = ValidDsn, | ||||||||||||||||
| IsGlobalModeEnabled = true, | ||||||||||||||||
| AutoSessionTracking = false | ||||||||||||||||
| }; | ||||||||||||||||
|
|
||||||||||||||||
| var hub = Substitute.For<IHub>(); | ||||||||||||||||
| var integration = new GlobalRootScopeIntegration(); | ||||||||||||||||
|
|
||||||||||||||||
| // Act | ||||||||||||||||
| integration.Register(hub, options); | ||||||||||||||||
|
|
||||||||||||||||
| // Assert | ||||||||||||||||
| hub.Received(1).ConfigureScope(Arg.Any<Action<Scope>>()); | ||||||||||||||||
|
||||||||||||||||
| hub.Received(1).ConfigureScope(Arg.Any<Action<Scope>>()); | |
| hub.Received(1).ConfigureScope(Arg.Do<Action<Scope>>(configure => | |
| { | |
| var scope = new Scope(options); | |
| configure(scope); | |
| Assert.Equal(options.InstallationId, scope.User?.Id); | |
| })); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,4 @@ | ||
| [ | ||
| { | ||
| Message: Initializing Hub for Dsn: '{0}'., | ||
| Args: [ | ||
| https://d4d82fc1c2c4032a83f3a29aa3a3aff@fake-sentry.io:65535/2147483647 | ||
| ] | ||
| }, | ||
| { | ||
| Message: Starting BackpressureMonitor. | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These were never really part of the test anyway (which is to ensure all the default integrations get registered) |
||
| }, | ||
| { | ||
| Message: Registering integration: '{0}'., | ||
| Args: [ | ||
|
|
@@ -37,5 +28,11 @@ | |
| Args: [ | ||
| SentryDiagnosticListenerIntegration | ||
| ] | ||
| }, | ||
| { | ||
| Message: Registering integration: '{0}'., | ||
| Args: [ | ||
| GlobalRootScopeIntegration | ||
| ] | ||
| } | ||
| ] | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could potentially set more stuff here. There are lots of scope properties that never change and which we currently set on the scope every time we capture an event. These are all candidates, I think:
sentry-dotnet/src/Sentry/Scope.cs
Lines 498 to 517 in f50b360
More of a performance improvement but might result in a subtle behavioural change for some users (perhaps code that checks/expects things to not be set in certain circumstances and sets them conditionally on this basis ). To be safe, perhaps we delay a change like that until the next major release.