feat: User.Id can now be overriden (set to null) in Global mode#5039
feat: User.Id can now be overriden (set to null) in Global mode#5039jamescrosswell wants to merge 11 commits intomainfrom
Conversation
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog. Breaking Changes 🛠
Features ✨
Fixes 🐛
Dependencies ⬆️Deps
Other
🤖 This preview updates automatically when you update the PR. |
| } | ||
| 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. |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
Could this be a behavior breaking change?
Would this be something for .NET 11 / v7?
| ] | ||
| }, | ||
| { | ||
| Message: Starting BackpressureMonitor. |
There was a problem hiding this comment.
These were never really part of the test anyway (which is to ensure all the default integrations get registered)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed ... let's open an issue for the .NET 11 / v7 project.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5039 +/- ##
==========================================
+ Coverage 74.01% 74.07% +0.06%
==========================================
Files 499 502 +3
Lines 18065 18121 +56
Branches 3518 3525 +7
==========================================
+ Hits 13370 13423 +53
- Misses 3836 3838 +2
- Partials 859 860 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adjusts how User.Id is populated from InstallationId when Global Mode is enabled, so that applications can explicitly clear/override the value (e.g., set it to null) without it being re-applied during event enrichment.
Changes:
- Add a new
GlobalRootScopeIntegrationdefault integration that setsscope.User.Idfromoptions.InstallationIdat startup (Global Mode only). - Update
Enricherto not apply theInstallationIdfallback in Global Mode (so user overrides aren’t re-written). - Update/extend tests and Verify snapshots to account for the new default integration and changed behavior.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/Sentry/Integrations/GlobalRootScopeIntegration.cs |
New integration to set User.Id on the root scope in Global Mode. |
src/Sentry/Internal/Enricher.cs |
Stops applying InstallationId fallback when Global Mode is enabled. |
src/Sentry/SentryOptions.cs |
Registers the new integration as a default integration. |
test/Sentry.Tests/Integrations/GlobalRootScopeIntegrationTests.cs |
Adds tests covering the new integration and updated enricher behavior. |
test/Sentry.Tests/SentryClientTests.cs |
Adjusts tests around fallback User.Id behavior. |
test/Sentry.Tests/SentryOptionsTests.verify.cs + *.verified.txt |
Narrows snapshot scope to integration-registration logs and updates expected output. |
src/Sentry/PlatformAbstractions/FrameworkInfo.cs |
Adds an additional .NET Framework release key mapping for 4.8.1. |
| #if NET8_0_OR_GREATER | ||
| | DefaultIntegrations.SystemDiagnosticsMetricsIntegration | ||
| #endif | ||
| | DefaultIntegrations.GlobalRootScopeIntegration | ||
| ; |
There was a problem hiding this comment.
GlobalRootScopeIntegration is now enabled by default, but unlike the other default integrations there’s no public Disable* method to let consumers turn it off. Consider adding a DisableGlobalRootScopeIntegration() (or similar) for parity and to give users a supported opt-out.
There was a problem hiding this comment.
Arguably it's not an 'integration'... it's just some behaviour we want on all global root scopes (there's no need for an opt-out). We could move it out of the integrations list and bake the logic into the Hub constructor instead.
Otherwise I think it's OK to simply omit the disable integration method for this one. @Flash0ver thoughts?
There was a problem hiding this comment.
We also don't offer a DisableAutoSessionTrackingIntegration opt-out for DefaultIntegrations.AutoSessionTrackingIntegration.
Not sure if by design or an oversight.
But for all other DefaultIntegrations, there is a Disable* opt-out.
Perhaps a question that helps to decide is:
- Should this emit a diagnostic log?
sentry-dotnet/src/Sentry/Internal/Hub.cs
Lines 100 to 108 in f50b360
On the other hand, we don't really have any "logic" in Hub.ctor other than:
sentry-dotnet/src/Sentry/Internal/Hub.cs
Lines 77 to 81 in f50b360
... I am not answering the question ...
If we are happy that this emits a Debug-Log,
and if the Debug-Log does not necessarily indicate to the user that this is behavior that can be opted-out of,
but instead just gives more insights into the system and debuggability that Global-Mode is enabled and the SDK applies some default behavior,
then I am slightly leaning towards the ISdkIntegration.
If otherwise this could cause more confusion to the user than informational benefit, I guess then the location to invoke the ConfigureScope is after:
sentry-dotnet/src/Sentry/Internal/Hub.cs
Lines 77 to 81 in f50b360
Although I am currently ever so slightly preferring the ISdkIntegration-based approach, I am very overrulable here, considering my feedback/thoughts.
| // 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; | ||
| } | ||
|
|
There was a problem hiding this comment.
With this change, the InstallationId fallback for User.Id is no longer applied when IsGlobalModeEnabled is true. That means scenarios that rely on Enricher without going through Hub initialization/integrations (e.g., apps constructing SentryClient directly) will no longer get the previous default User.Id behavior in global mode. If that behavior needs to remain, consider an alternative that still allows explicit clearing (e.g., apply the fallback when creating the initial Scope/root scope, or introduce an explicit option/flag indicating whether the fallback should be applied).
| // 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.
I don't think there's a scenario where someone uses a SentryClient independent of the Hub... so this isn't a real concern.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Thank you for the PR! |
| { | ||
| #if NET5_0_OR_GREATER | ||
| Skip.If(System.OperatingSystem.IsAndroid() || System.OperatingSystem.IsIOS(), | ||
| "On mobile, User.Id is set by GlobalRootScopeIntegration at startup, not the enricher."); |
There was a problem hiding this comment.
suggestion: use nameof
For "go to declaration"-ability.
| "On mobile, User.Id is set by GlobalRootScopeIntegration at startup, not the enricher."); | |
| $"On mobile, User.Id is set by {nameof(GlobalRootScopeIntegration)} at startup, not the enricher."); |
| // In global mode the userid gets set at app startup via the GlobalRootScopeIntegration, rather than by an | ||
| // enricher during capture... so this functionality in SentryClient only works when IsGlobalModeEnabled is false | ||
| Skip.If(System.OperatingSystem.IsAndroid() || System.OperatingSystem.IsIOS(), | ||
| "On mobile, User.Id is set by GlobalRootScopeIntegration at startup, not the enricher."); |
There was a problem hiding this comment.
suggestion: use nameof
For easier navigation in a code editor.
| "On mobile, User.Id is set by GlobalRootScopeIntegration at startup, not the enricher."); | |
| $"On mobile, User.Id is set by {nameof(GlobalRootScopeIntegration)} at startup, not the enricher."); |
| // In global mode the userid gets set at app startup via the GlobalRootScopeIntegration, rather than by an | ||
| // enricher during capture... so this functionality in SentryClient only works when IsGlobalModeEnabled is false |
There was a problem hiding this comment.
suggestion (nit pick): move comment
Move comment between // Arrange and _fixture.SentryOptions.IsGlobalModeEnabled = false;,
so that this test is in sync with CaptureTransaction_UserIsNull_SetsFallbackUserId.
There was a problem hiding this comment.
Agreed ... let's open an issue for the .NET 11 / v7 project.
| return; | ||
| } | ||
|
|
||
| hub.ConfigureScope(scope => scope.User.Id ??= options.InstallationId); |
There was a problem hiding this comment.
question: compliant with new PII rules?
See https://develop.sentry.dev/sdk/foundations/client/data-collection/.
I haven't checked the spec yet ... currently in the draft phase ... need to go though it.
I just wanted to make sure we don't paint ourselves into a corner when - later this year - we implement the new PII rules via a new dataCollection-Options.
| } | ||
| 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. |
There was a problem hiding this comment.
Could this be a behavior breaking change?
Would this be something for .NET 11 / v7?
| {533320, "4.8.1"}, | ||
| {533325, "4.8.1"} | ||
| {533325, "4.8.1"}, | ||
| {533509, "4.8.1"} |
There was a problem hiding this comment.
question: how did you notice this?
CI just started to fail when a new servicing update is applied to the runners?
There was a problem hiding this comment.
I think I ran the unit tests on my Windows box which is running the latest version of Windows and service packs, which broke the tests. Presumably those are now hitting the CI runners.
| #if NET8_0_OR_GREATER | ||
| | DefaultIntegrations.SystemDiagnosticsMetricsIntegration | ||
| #endif | ||
| | DefaultIntegrations.GlobalRootScopeIntegration | ||
| ; |
There was a problem hiding this comment.
We also don't offer a DisableAutoSessionTrackingIntegration opt-out for DefaultIntegrations.AutoSessionTrackingIntegration.
Not sure if by design or an oversight.
But for all other DefaultIntegrations, there is a Disable* opt-out.
Perhaps a question that helps to decide is:
- Should this emit a diagnostic log?
sentry-dotnet/src/Sentry/Internal/Hub.cs
Lines 100 to 108 in f50b360
On the other hand, we don't really have any "logic" in Hub.ctor other than:
sentry-dotnet/src/Sentry/Internal/Hub.cs
Lines 77 to 81 in f50b360
... I am not answering the question ...
If we are happy that this emits a Debug-Log,
and if the Debug-Log does not necessarily indicate to the user that this is behavior that can be opted-out of,
but instead just gives more insights into the system and debuggability that Global-Mode is enabled and the SDK applies some default behavior,
then I am slightly leaning towards the ISdkIntegration.
If otherwise this could cause more confusion to the user than informational benefit, I guess then the location to invoke the ConfigureScope is after:
sentry-dotnet/src/Sentry/Internal/Hub.cs
Lines 77 to 81 in f50b360
Although I am currently ever so slightly preferring the ISdkIntegration-based approach, I am very overrulable here, considering my feedback/thoughts.
| integration.Register(hub, options); | ||
|
|
||
| // Assert | ||
| hub.DidNotReceive().ConfigureScope(Arg.Any<Action<Scope>>()); |
There was a problem hiding this comment.
suggestion: test consistency
Here, we check whether an invocation occurred or not.
But the following two tests check scope.User.Id.
Could we here do a similar setup with the Scope and assert that,
so that we are consistent and indeed test correlated behavior.
In other words ... could we
- also create a
Scope - call
hub.SubstituteConfigureScope(scope); - and then assert that the
scope.User.Idis null
And also - to be consistent the other way around to - add to the other tests an assertion that the invocation did occur ... to have consistent assertions:
hub.Received(1).ConfigureScope(Arg.Any<Action<Scope>>());
Resolves #4172