chore(deps): update Native SDK to v0.14.0#5189
chore(deps): update Native SDK to v0.14.0#5189github-actions[bot] wants to merge 3 commits intomainfrom
Conversation
| @@ -1 +1 @@ | |||
| Subproject commit 87f25f2a4b1c7bd5edc0b4dfd5ee463ffc4cf2d8 | |||
| Subproject commit c0e5f0705da3853ff548c7ece77d639a20e1d8f5 | |||
There was a problem hiding this comment.
Bug: Bumping sentry-native enables metrics and logs by default, but the .NET SDK doesn't propagate settings like options.EnableMetrics = false to disable them at the native layer.
Severity: MEDIUM
Suggested Fix
In src/Sentry/Platforms/Native/CFunctions.cs, call sentry_options_set_enable_metrics(cOptions, 0) when options.EnableMetrics is false. Also, introduce a new option in the .NET SDK to control native structured logs and call sentry_options_set_enable_logs(cOptions, 0) based on that setting, mirroring the existing pattern for disabling other native features.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: modules/sentry-native#L1
Potential issue: The update to `sentry-native` v0.14.0 enables metrics and structured
logs by default at the native layer. However, the .NET SDK does not propagate user
settings to disable these features. For instance, if a user sets `options.EnableMetrics
= false`, the native layer will still collect and send metrics because the `Init()`
method in `src/Sentry/Platforms/Native/CFunctions.cs` does not call
`sentry_options_set_enable_metrics`. Similarly, there is no option provided in the .NET
SDK to disable structured logs from the native layer, which are now on by default. This
is a regression that ignores user configuration.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
In sentry-dotnet, we will enable Logs per default in #5183 with the next major (v7) end of the year.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5189 +/- ##
==========================================
- Coverage 74.20% 74.09% -0.11%
==========================================
Files 501 506 +5
Lines 18113 18247 +134
Branches 3521 3564 +43
==========================================
+ Hits 13440 13520 +80
- Misses 3811 3857 +46
- Partials 862 870 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| sentry_options_set_enable_logs(cOptions, options.EnableLogs ? 1 : 0); | ||
| sentry_options_set_enable_metrics(cOptions, options.EnableMetrics ? 1 : 0); |
There was a problem hiding this comment.
following up on: #5189 (comment)
thought: now I am second guessing myself
Ok ... I'm quite undecisive on this one ... here comes the bikeshedding:
If we sync these two options, shouldn't we then actually sync all options that from a "set intersection"?
Which then would be a behavioral breaking change ... wouldn't it?
My current thought is:
Do we instead only want to avoid the breaking change of changing the sentry-native semantics of disable-per-default to enable-per-default in a minor release of the .NET SDK?
On the other hand, we already suffered a breaking change in 0.13.5 of sentry-native where Metrics got enabled per default and we didn't change anything in the sentry-dotnet side.
And another thought:
Am I overthinking this?
Are the sentry-native APIs even exposed to the .NET consumer ... so would it even matter?
sentry-native has no "default-integration" for neither Logs nor Metrics (does it?), so if the APIs aren't called then nothing will happen, shall we then just leave the defaults of sentry-native as they are.
And instead in the next major release of sentry-dotnet forward all Options that appear in both down to sentry-native and then keep future options and changes of the default values in sync?
There was a problem hiding this comment.
Let's keep them consistent - so if someone does not enable logs in the .NET SDK they don't get logs (from any of the SDKs) and the same for metrics.
If that's not what we're doing now, let's change it. That's not a breaking change - it's just fixing something that's broken... It is a change in behaviour (from broken to fixed).
There was a problem hiding this comment.
OK ... then let's bring the sync of Logs & Metrics options back,
and follow-up with PRs to pass-through more/all options we can.
This reverts commit de27031.
Bumps modules/sentry-native from 0.13.8 to 0.14.0.
Auto-generated by a dependency updater.
Changelog
0.14.0
Breaking / Important behavior changes:
0.13.5and is now documented as part of the0.14.0behavior. Applications that do not want to send metrics must explicitly opt out withsentry_options_set_enable_metrics(options, false). (#1609)0.13.9and is now documented as part of the0.14.0behavior. Applications that do not want to capture structured logs must explicitly opt out withsentry_options_set_enable_logs(options, false). (#1673)0.13.9
Features:
sentry_options_set_enable_logs(options, false). (#1673)crashpad_wait_for_uploadflag. (#1679, crashpad#152)sentry_options_set_enable_large_attachments. (#1545)Fixes:
shutdown_timeoutoption in the daemon. (#1691)