Skip to content
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

Feat: native crash reporting #2887

Merged
merged 38 commits into from
Nov 29, 2023
Merged

Feat: native crash reporting #2887

merged 38 commits into from
Nov 29, 2023

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented Nov 22, 2023

Closes #2770

TODO:

Sample event: https://sentry-sdks.sentry.io/issues/4677655767/events/025a40ef472d40b2fa0c3011bf3c68bf/

Sample log output (second run)

  Debug: Setting native CacheDirectoryPath on Windows: C:\dev\dotnet\.sentry-native
  Debug: Logging enabled with ConsoleDiagnosticLogger and min level: Debug
  Debug: Setting Debug: True
  Debug: Disabling native auto session tracking
  Debug: Setting native CacheDirectoryPath on Windows: C:\dev\dotnet\.sentry-native
  Debug: Setting a native logger
  Debug: Initializing sentry native
   Info: [native] using database path "C:\dev\dotnet\.sentry-native"
  Debug: [native] starting transport
  Debug: [native] starting background worker thread
  Debug: [native] starting backend
  Debug: [native] background worker thread started
  Debug: [native] processing and pruning old runs
  Debug: [native] sending envelope
  Debug: [native] submitting task to background worker thread
  Debug: [native] executing task on worker thread
  Debug: [native] sending request using winhttp to "https://o447951.ingest.sentry.io:443/api/5428537/envelope/": x-sentry-auth:Sentry sentry_key=eb18e953812b41c3aeb042e666fd3b5c, sentry_version=7, sentry_client=sentry.native.dotnet/0.6.7 content-type:application/x-sentry-envelope content-length:10364
  Debug: Native SDK reported: 'crashedLastRun': 'True'
  Debug: Initializing Hub for Dsn: 'https://eb18e953812b41c3aeb042e666fd3b5c@o447951.ingest.sentry.io/5428537'.
  Debug: This looks like a NativeAOT application build.
  Debug: Starting BackgroundWorker.
  Debug: New scope pushed.
  Debug: Registering integration: 'AutoSessionTrackingIntegration'.
  Debug: Registering integration: 'AppDomainUnhandledExceptionIntegration'.
  Debug: Registering integration: 'AppDomainProcessExitIntegration'.
  Debug: Registering integration: 'UnobservedTaskExceptionIntegration'.
  Debug: Registering integration: 'SentryDiagnosticListenerIntegration'.
   Info: DiagnosticSource Integration is disabled because tracing is disabled.
  Debug: Registering integration: 'WinUIUnhandledExceptionIntegration'.
  Debug: BackgroundWorker Started.
  Debug: [native] received response: HTTP/1.1 200 OK Date: Tue, 28 Nov 2023 13:52:24 GMT Via: 1.1 google Content-Length: 41 Content-Type: application/json Server: nginx Vary: origin,access-control-request-method,access-control-request-headers access-control-allow-origin: * access-control-expose-headers: x-sentry-error,x-sentry-rate-limits,retry-after cross-origin-resource-policy: cross-origin x-envoy-upstream-service-time: 0 Strict-Transport-Security: max-age=31536000; includeSubDomains; preload Alt-Svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000
  Debug: [native] request handled in 216ms
  Debug: Triggering a deliberate exception because SentrySdk.CauseCrash(CrashType.Native) was called
   Info: [native] entering signal handler
  Debug: [native] captured backtrace from ucontext with 17 frames
  Debug: [native] captured backtrace with 17 frames
  Debug: [native] merging scope into event
  Debug: [native] adding attachments to envelope
  Debug: [native] sending envelope
  Debug: [native] serializing envelope into buffer
   Info: [native] crash has been captured

@vaind vaind changed the base branch from main to feat/4.0.0 November 22, 2023 20:54
Copy link
Contributor

github-actions bot commented Nov 22, 2023

Fails
🚫 Please consider adding a changelog entry for the next release.
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- native crash reporting ([#2887](https://github.com/getsentry/sentry-dotnet/pull/2887))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against bf0bd8a

src/Sentry/SentrySdk.cs Outdated Show resolved Hide resolved
src/Sentry/SentrySdk.cs Outdated Show resolved Hide resolved
@vaind vaind force-pushed the feat/native-crash-reporting branch 2 times, most recently from 005fc4b to 2e51a9f Compare November 24, 2023 09:28
@vaind vaind force-pushed the feat/native-crash-reporting branch from 2e51a9f to 3342217 Compare November 24, 2023 13:40
@bitsandfoxes bitsandfoxes self-assigned this Nov 27, 2023
Base automatically changed from feat/4.0.0 to main November 27, 2023 19:49
@vaind vaind marked this pull request as ready for review November 28, 2023 15:46
@bruno-garcia
Copy link
Member

what do we want to do about the cache directory path? Defaults to the current working directory. Can we figure out a more reasonable default or do we want to document this? Or even warn unless explicitly specified?

This is a problem on a Mac where the OS asks the user to give permission if the file is on Documents and others.
I wonder if using a consistent path name under Path.GetTempPath is an alternative.

It's a problem on Unity today btw ^ so we could align the solution here

ALso a problem the fact we don't have paths by default. And the .NET SDK doesn't support having more than 1 instance of the app running at the same time (afaik not fixed) as worker transports would race to pick work off of the directory

integration-test/runtime.Tests.ps1 Outdated Show resolved Hide resolved
// You can set it in the SENTRY_DSN environment variable, or you can set it in code here.
// options.Dsn = "... Your DSN ...";

// When debug is enabled, the Sentry client will emit detailed debugging information to the console.
Copy link
Member

Choose a reason for hiding this comment

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

Today setting a path (in the .NET layer) is the way to opt-in to offline caching. Are we changing that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Native backend uses offline caching to store the error at the time of the crash and only sends it later on the second app run.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So is this also really an issue with SentryNative?

what do we want to do about the cache directory path? Defaults to the current working directory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I get your question right, @jamescrosswell, then yes, this is only an issue with native crashes that are captured by sentry-native

Copy link
Member

Choose a reason for hiding this comment

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

Today setting a path (in the .NET layer) is the way to opt-in to offline caching. Are we changing that?

We might document that, today we only say: Only disk access happen if u set a path here

src/Sentry/Internal/ContextWriter.cs Show resolved Hide resolved
src/Sentry/Platforms/Native/CFunctions.cs Show resolved Hide resolved
src/Sentry/User.cs Show resolved Hide resolved
src/Sentry/User.cs Show resolved Hide resolved
src/Sentry/User.cs Show resolved Hide resolved
src/Sentry/User.cs Show resolved Hide resolved
src/Sentry/User.cs Show resolved Hide resolved
vaind and others added 4 commits November 28, 2023 20:18
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
src/Sentry/User.cs Outdated Show resolved Hide resolved
@vaind vaind merged commit 217c11c into main Nov 29, 2023
19 checks passed
@vaind vaind deleted the feat/native-crash-reporting branch November 29, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Native AOT - sentry-native crash reporting
4 participants