-
-
Notifications
You must be signed in to change notification settings - Fork 241
fix: fetch default attributes from Scope first
#5376
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 all commits
cc9de07
925a20a
f8f932a
6a90a32
d2b5eb5
c6268d0
b4821a3
ce47fbb
55b2d5d
52db5d3
8efd196
184ea53
a0babfe
4db9f38
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 | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -144,8 +144,35 @@ public SentryUser User | |||||||||||
| /// <inheritdoc /> | ||||||||||||
| public string? Distribution { get; set; } | ||||||||||||
|
|
||||||||||||
| private string? _environment; | ||||||||||||
|
|
||||||||||||
| /// <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); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // TransactionName is kept for legacy purposes because | ||||||||||||
| // SentryEvent still makes use of it. | ||||||||||||
|
|
@@ -288,6 +315,10 @@ internal Scope(SentryOptions? options, SentryPropagationContext? propagationCont | |||||||||||
| { | ||||||||||||
| Options = options ?? new SentryOptions(); | ||||||||||||
| PropagationContext = new SentryPropagationContext(propagationContext); | ||||||||||||
|
|
||||||||||||
| // Skip scope sync with backing field. The native SDKs already received the environment set on the options. | ||||||||||||
| _environment = Options.Environment; | ||||||||||||
| Release = Options.Release; | ||||||||||||
|
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. PushScope ignores custom ReleaseMedium Severity Initializing each scope’s Additional Locations (1)Reviewed by Cursor Bugbot for commit 55b2d5d. Configure here.
Collaborator
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. Not an immediate solution (would require a major version bump) but maybe reinforces this idea: In the interim, it seems like Options should only be used to populate the root scope. Any scope that gets cloned after that should probably just apply itself (not the options) to the new clone. Kind of opinionated, but can anyone see a reason to do it otherwise?
Contributor
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. Then ... I believe ... we have some unintended behavior! By setting
So SentrySdk.Init(options =>
{
options.Release = "options-release";
});
SentrySdk.ConfigureScope(static scope =>
{
scope.Release = "scope-release";
});
var scope = SentrySdk.PushScope();
SentrySdk.ConfigureScope(static scope =>
{
Console.WriteLine(scope.Release); //options-release
});Perhaps we need a facility, where we have a
Collaborator
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. TBH it's a bit weird for stuff like We seem to be using the scope as a vehicle to apply these properties to things that are Note
So maybe we go the opposite direction? When And in the next major, we see if we can remove these from the Scope entirely. They should probably never have been added there. @bitsandfoxes thoughts 🤔
Flash0ver marked this conversation as resolved.
Contributor
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. question: Do we need similar treatment for |
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // For testing. Should explicitly require SentryOptions. | ||||||||||||
|
|
@@ -404,9 +435,9 @@ public void Clear() | |||||||||||
| Request = new(); | ||||||||||||
| Contexts.Clear(); | ||||||||||||
| User = new(); | ||||||||||||
| Release = default; | ||||||||||||
| Release = Options.Release; | ||||||||||||
| Distribution = default; | ||||||||||||
| Environment = default; | ||||||||||||
| Environment = Options.Environment; // Restore to keep in sync with native | ||||||||||||
| TransactionName = default; | ||||||||||||
| Transaction = default; | ||||||||||||
| Fingerprint = Array.Empty<string>(); | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -63,6 +63,10 @@ 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(); | ||||||||||||||||
| options.Release = options.SettingLocator.GetRelease(); | ||||||||||||||||
|
Collaborator
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. 👍🏻 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. Release init blocks mobile defaultsHigh Severity Calling Reviewed by Cursor Bugbot for commit a0babfe. Configure here.
Contributor
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. confirmed: This does now apply sentry-dotnet/src/Sentry/Platforms/Native/CFunctions.cs Lines 89 to 93 in cc9de07
But, it changes existing behavior for Android
and also iOS / Mac Catalyst
Do we intend this change in behavior for Android/Apple? |
||||||||||||||||
|
|
||||||||||||||||
| // Initialize native platform SDKs here | ||||||||||||||||
| if (options.InitNativeSdks) | ||||||||||||||||
| { | ||||||||||||||||
|
|
||||||||||||||||


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.
issue: don't edit
CHANGELOG.mdmanuallySorry for the confusion I've caused ... I meant to add a Changelog Entry indirectly via
### Changelog Entryin the PR description (https://craft.sentry.dev/configuration/#custom-changelog-entries-from-pr-descriptions).This way we can have "great changelog entries" while avoiding merge conflicts when editing
CHANGELOG.mdmanually.We just merged new guidance: #5369