sentry-native SDK integration by vaind · Pull Request #2704 · getsentry/sentry-dotnet

vaind · 2023-10-06T11:11:12Z

closes Native AOT - attach debug images #2700
based on macOS native SDK integration #2625 but only for Windows/Linux

Summary of the changes

This PR adds a Sentri.Native.Bindings nuget packages which collects compiled static libraries of the sentry-native submodule.
These libs are linked statically via NativeLibrary and DirectPInvoke if a dependant app is AOT compiled. The static libs mean end-users don't get an additional library alongside their single executable 🥳
the integration is currently limited to collecting native debug images, no crash handling yet

Next steps

integration test (depends on Trimming for .NET 7+ #2699 and Native AOT - upload debug files and sources #2692) - Native AOT stack trace integration test #2772
macOS (depends on macOS native SDK integration #2625 which needs .NET SDK 8 RC2)
collect both platforms for the nuget package when running in CI - feat: packaging native runtime libs #2781
test the nuget package (how?)

#skip-changelog

…sh-reporting

github-actions · 2023-10-06T11:12:25Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against eb30270

bruno-garcia · 2023-10-10T22:12:11Z

src/Sentry.Bindings.Native/Sentry.Bindings.Native.csproj

+    </None>
+  </ItemGroup>
+
+  <ItemGroup Condition="'$(CI_PUBLISHING_BUILD)' == 'true' or $([MSBuild]::IsOsPlatform('Linux'))">


I imagine this CI_PUBLISHING_BUILD stuff is on the other binding projects and got copied here. But why do we need this? I'm concerned about stuff behaving differently in CI vs locally

It's intentional here though not yet necessary, I've extracted that as a "next step" in the PR description:

collect both platforms for the nuget package when running in CI

As to why: because we can't build all platforms on a single machine and I don't really want to do the download from a CI job the way I had to do in sentry-unity because that is very painful whenever there's a change, e.g. you want to run different sentry-native version on your branch than what has been compiled as an artifact on the main branch.

The way this project is currently implemented is that it compiles for your platform locally and only if necessary (i.e. sentry-native submodule checkout has changed or if there's a change in this Sentry.Bindings.Native.csproj file). In CI, it will collect and package for all platforms. At least that's the idea.

src/Sentry/DebugImage.cs

bruno-garcia · 2023-10-11T12:51:12Z

src/Sentry/Internal/DebugStackTrace.cs

            {
-                relocations.Add(GetRelativeAddressMode(i), GetRelativeAddressMode(@event.DebugImages.Count));
-                @event.DebugImages.Add(DebugImages[i]);
+                _options.LogWarning("Unexpected debug image: neither ModuleVersionId nor ImageAddress is defined: {0}", DebugImages[i].ToString());


No ToString seems to be implemented here:

sentry-dotnet/src/Sentry/DebugImage.cs

Line 10 in 61fb2ee

public sealed class DebugImage : IJsonSerializable

Did you mean to add one? Or access a property here instead?

Shouldn't really happen, I've removed .ToString() and added a Debug.Assert()

src/Sentry/Internal/ScopeObserver.cs

src/Sentry/Platforms/Native/BaseContextWriter.cs

src/Sentry/Platforms/Native/SentryNativeBridge.cs

src/Sentry/Sentry.csproj

vaind · 2023-10-14T10:22:58Z

@vaind I'm wondering how to approach reviewing this... it's a pretty massive set of changes to reverse engineer.

Is there any chance you could give a high level problem summary and solution description in the PR? The Native integrations are a bit mysterious to me in general... so I don't have a lot of context coming into this.

I've update a description a bit (also make sure to read the issue first as that's where the problem statement is). However, it may still be insufficient if you're not very familiar with the native integrations (and native stack trace handling). In that case, either let me know and I'll try to walk you through some relevant bits or maybe someone else would have a chance to review this in the meantime.

bitsandfoxes

You mentioned you'll move the ImageAddress type change to another PR?

src/Sentry/Platforms/Native/SentryNativeBridge.cs

jamescrosswell · 2023-10-17T00:28:55Z

src/Sentry/Internal/DebugStackTrace.cs

Similar to @bitsandfoxes, it'd be really good to get a breakdown of the changes in this file. I'll try to put some specific questions in where I've got enough context to ask something sensible 😉

jamescrosswell · 2023-10-17T00:29:55Z

src/Sentry/Internal/DebugStackTrace.cs

+
+                if (!found)
+                {
+                    relocations.Add(GetRelativeAddressMode(i), GetRelativeAddressMode(@event.DebugImages.Count));


Why @event.DebugImages.Count here?

because that's the index we're adding the image at on the next line.

jamescrosswell · 2023-10-17T00:40:17Z

src/Sentry/Internal/DebugStackTrace.cs

@@ -88,25 +91,55 @@ internal void MergeDebugImagesInto(SentryEvent @event)
        Dictionary<string, string> relocations = new();


We only get this far if there are already debug images on the event... from the comments above, it seems that only ever happens when there are InnerExceptions (and so multiple stack traces) - correct?

I guess a high level question at that point: What is the information that could overlap from one stack trace to another, in this scenario and how/what are we trying to merge here?

jamescrosswell · 2023-10-17T00:47:27Z

src/Sentry/Internal/DebugStackTrace.cs

+                for (var j = 0; j < originalCount; j++)
                {
-                    if (i != j)
+                    if (DebugImages[i].ModuleVersionId == @event.DebugImages[j].ModuleVersionId)


Is this what we mean by ModuleVersionId here? That seems to identify a particular assembly... I take it the offsets in PDBs are module relative. I'm wondering what relocations is doing then... my best guess is that maybe the modules are stored in a different order in each of the different stack traces, but we only send the modules once in the SentryEvent so we need to map debug images from each stack trace to the correct module index to get the correct offsets??? It's just as likely that I have no idea whatsoever what's going on here though 😜

Firstly, this code is actually old, it's just moved under a if (DebugImages[i].ModuleVersionId is not null) condition so not really in need for reviewing (#2050). Also, this would be a good read if you wanted to start looking into this topic: https://github.com/getsentry/rfcs/blob/main/text/0013-portable-pdb.md

But I'll try to answer your questions to the best of my knowledge (and memory :) ):

Is this what we mean by ModuleVersionId here?

yes

I'm wondering what relocations is doing then...

To Sentry (backend), we're sending a list of debug images and each stack frame references an image in this list by "rel:IMAGE_INDEX" (see addr_mode).

my best guess is that maybe the modules are stored in a different order in each of the different stack traces

correct

, but we only send the modules once in the SentryEvent so we need to map debug images from each stack trace to the correct module index to get the correct offsets???

kinda - we can send the same image multiple times but it's wasteful. One could argue that running this code is also wasteful, but to me, it feels better to run the code once on the client and reduce the payload (less stuff to serialize, transfer, deserialize and process, store)

jamescrosswell

At least as far as I understand, it looks good. Most of the meat is in DebugStackTrace.cs and we try to get a managed stack trace but now, failing that, fall back on native stack traces.

One important question is, other than the fact that it compiles, how do we know it works? Do we have any automated tests for this or was it just manual smoke tests?

vaind

Since the outstanding questions were unrelated to the changes made in this PR, I'm merging

vaind · 2023-10-17T09:17:26Z

src/Sentry/Internal/DebugStackTrace.cs

+                for (var j = 0; j < originalCount; j++)
                {
-                    if (i != j)
+                    if (DebugImages[i].ModuleVersionId == @event.DebugImages[j].ModuleVersionId)


Firstly, this code is actually old, it's just moved under a if (DebugImages[i].ModuleVersionId is not null) condition so not really in need for reviewing (#2050). Also, this would be a good read if you wanted to start looking into this topic: https://github.com/getsentry/rfcs/blob/main/text/0013-portable-pdb.md

But I'll try to answer your questions to the best of my knowledge (and memory :) ):

Is this what we mean by ModuleVersionId here?

yes

I'm wondering what relocations is doing then...

To Sentry (backend), we're sending a list of debug images and each stack frame references an image in this list by "rel:IMAGE_INDEX" (see addr_mode).

my best guess is that maybe the modules are stored in a different order in each of the different stack traces

correct

, but we only send the modules once in the SentryEvent so we need to map debug images from each stack trace to the correct module index to get the correct offsets???

kinda - we can send the same image multiple times but it's wasteful. One could argue that running this code is also wasteful, but to me, it feels better to run the code once on the client and reduce the payload (less stuff to serialize, transfer, deserialize and process, store)

vaind · 2023-10-17T09:18:00Z

src/Sentry/Internal/DebugStackTrace.cs

+
+                if (!found)
+                {
+                    relocations.Add(GetRelativeAddressMode(i), GetRelativeAddressMode(@event.DebugImages.Count));


because that's the index we're adding the image at on the next line.

vaind · 2023-10-17T09:21:31Z

One important question is, other than the fact that it compiles, how do we know it works? Do we have any automated tests for this or was it just manual smoke tests?

https://github.com/getsentry/sentry-dotnet/blob/5e0d377e38ebfa8c6e6056225b1f2390d5ddc615/test/Sentry.Tests/Internals/DebugStackTraceTests.verify.cs

And there will be an integration test for Native AOT once we can build one, see "Next steps" in the PR description

jamescrosswell · 2023-10-17T10:35:59Z

https://github.com/getsentry/sentry-dotnet/blob/5e0d377e38ebfa8c6e6056225b1f2390d5ddc615/test/Sentry.Tests/Internals/DebugStackTraceTests.verify.cs

And there will be an integration test for Native AOT once we can build one, see "Next steps" in the PR description

OK, but none of those tests changed in the PR right? And we'd need to be generating native debug images for any of the new code to get tested so I think those tests are just ensuring the PR doesn't break any existing functionality with managed code???

We will test the new code once we have the Native AOT integration tests though, so that answers my question.

jamescrosswell · 2023-10-17T10:36:50Z

Also, this would be a good read if you wanted to start looking into this topic: https://github.com/getsentry/rfcs/blob/main/text/0013-portable-pdb.md

🤯 OMG I had no idea those existed - thank you!

bruno-garcia and others added 12 commits September 15, 2023 23:02

native bridge from unity

4fe3227

bridge native

8b6fb29

mac bridge

545544f

pack

b964139

cocoa binding

ba8f995

console runs

fbac943

crash cli natively

47cd5b1

crash captured by sentry cocoa

dcee9d4

Merge remote-tracking branch 'origin/feat/4.0.0' into feat/native-cra…

c28271a

…sh-reporting

trying to get this to compile

d71c58a

fix compilation

abc3543

chore: remove macos stuff

e1e5927

vaind added 12 commits October 6, 2023 13:14

remove more macos stuff

56d181e

windows net48 test issue

5c6a94d

feat: add sentry-native submodule & bindings project

5896483

restore unintended formatting changs

144f1c3

chore: update solution filters

041deaa

update native bindings msbuild

e999663

native packaging

47f5758

trim trailing spaces

6037759

wip: native lib static linking

4c515b3

refactor: change DebugImage.ImageAddress to long

476e538

finish native debug image collection

4ff2d58

native aot linux support

f509b42

vaind marked this pull request as ready for review October 10, 2023 18:44

vaind requested review from bitsandfoxes and jamescrosswell as code owners October 10, 2023 18:44

jamescrosswell added this to the 4.0.0 milestone Oct 11, 2023

bruno-garcia reviewed Oct 11, 2023

View reviewed changes

bitsandfoxes approved these changes Oct 16, 2023

View reviewed changes

src/Sentry/Platforms/Native/SentryNativeBridge.cs Outdated Show resolved Hide resolved

src/Sentry/Platforms/Native/SentryNativeBridge.cs Outdated Show resolved Hide resolved

vaind added 3 commits October 16, 2023 13:11

Merge branch 'feat/4.0.0' into feat/sentry-native-integration

42b8096

remove unused native binding code

9607da4

fixup changelog

29d02a4

vaind force-pushed the feat/sentry-native-integration branch from 0ba14ff to 29d02a4 Compare October 16, 2023 11:59

vaind added 5 commits October 16, 2023 14:07

chore: remove unused code

146f2a0

chore: add sentry-native updater entry

5700783

chore: update changelog

5922415

fixup: log an unexpected image

17133c3

chore: remove unnecessary todo

eb30270

jamescrosswell reviewed Oct 17, 2023

View reviewed changes

vaind commented Oct 17, 2023

View reviewed changes

vaind merged commit d32f05b into feat/4.0.0 Oct 17, 2023

vaind deleted the feat/sentry-native-integration branch October 17, 2023 09:21

This was referenced Oct 18, 2023

Native AOT - attach debug images #2700

Closed

feat: cocoa SDK integration for macOS #2750

Closed

This was referenced Nov 16, 2023

feat: Add NET8 to major packages #2852

Merged

chore: Update DeviceTests from upstream #2853

Closed

chore: remove obsolete APIs #2856

Merged

		@@ -88,25 +91,55 @@ internal void MergeDebugImagesInto(SentryEvent @event)
		Dictionary<string, string> relocations = new();

Uh oh!

Conversation

vaind commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the changes

Next steps

Uh oh!

github-actions bot commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vaind commented Oct 14, 2023

Uh oh!

bitsandfoxes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamescrosswell Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaind Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamescrosswell left a comment

Choose a reason for hiding this comment

Uh oh!

vaind left a comment

Choose a reason for hiding this comment

Uh oh!

vaind Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaind commented Oct 17, 2023

Uh oh!

jamescrosswell commented Oct 17, 2023

Uh oh!

jamescrosswell commented Oct 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vaind commented Oct 6, 2023 •

edited

Loading

github-actions bot commented Oct 6, 2023 •

edited

Loading

jamescrosswell Oct 17, 2023 •

edited

Loading

vaind Oct 17, 2023 •

edited

Loading

vaind Oct 17, 2023 •

edited

Loading