feat: Add screenshot at crash #1920

brustolin · 2022-06-27T09:27:55Z

📜 Description

Try to add a screenshot during a hard crash

💡 Motivation and Context

Close #1873

💚 How did you test it?

Unit tests

📝 Checklist

I reviewed the submitted code
I added tests to verify the changes
I updated the docs if needed
Review from the native team if needed
No breaking changes

🔮 Next steps

github-actions · 2022-06-27T09:28:34Z

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

Generated by 🚫 dangerJS against bf6008e

…sentry-cocoa into feat/crash-screenshot

philipphofmann

The approach makes sense to me. Good job 💯. We still miss tests.

Sources/SentryCrash/Recording/SentryCrashReportStore.h

Sources/SentryCrash/Recording/SentryCrashC.h

Sources/SentryCrash/Recording/SentryCrashC.c

Sources/Sentry/SentryCrashReportSink.m

Sources/Sentry/SentryScreenshotIntegration.m

Sources/Sentry/SentryScreenshot.m

Sources/Sentry/SentryScreenshotIntegration.m

Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>

CHANGELOG.md

Sources/SentryCrash/Recording/SentryCrashReportStore.h

Sources/Sentry/SentryScreenshotIntegration.m

Sources/Sentry/SentryScreenshot.m

philipphofmann

Nice, another pass.

…sentry-cocoa into feat/crash-screenshot

philipphofmann

Almost LGTM, thanks great job 👏 .

Sources/Sentry/SentryScreenshotIntegration.m

Sources/Sentry/SentryScreenshot.m

Sources/SentryCrash/Recording/SentryCrash.m

Sources/SentryCrash/Recording/SentryCrashC.c

philipphofmann · 2022-07-05T15:01:20Z

Sources/SentryCrash/Recording/SentryCrashReportStore.c

+    uint32_t lowerBaseID = (uint32_t)(baseID & 0xffffffff);
+    g_nextUniqueIDLow = lowerBaseID;


m: Why this change?

This I don't get it. But without the change it does not work and g_nextUniqueIDLow is always the same.

I just debugged it and it wasn't for me. That would be weird.

Tests/SentryTests/TestClient.swift

philipphofmann · 2022-07-05T15:05:55Z

Sources/Sentry/SentryCrashReportSink.m

+
+    if (report[SENTRYCRASH_REPORT_SCREENSHOT_ITEM]) {
+        for (NSString *ssPath in report[SENTRYCRASH_REPORT_SCREENSHOT_ITEM]) {
+            [scope addAttachment:[[SentryAttachment alloc] initWithPath:ssPath]];


h: When do we delete the screenshots? The attachments will be read when the SDK creates the envelope. How do we know that the screenshots are still stored on disk? Maybe it's better to read the attachments already here, and use SentryAttachment .initWithData. Then we could delete them already here.

Screenshots are deleted alongside the report. We had a code for that already.
Lets say we fail to transmit the report, on the next attempt the files won't be there, I rather have the current mechanism that deletes it.

Tests/SentryTests/SentryCrash/SentryCrashReportStore_Tests.m

Tests/SentryTests/SentryCrash/SentryCrashTests.m

philipphofmann · 2022-07-05T15:17:45Z

@brustolin, I think you should ping someone from the native team or maybe Andrew to get a review on the C code, please.

Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>

…sentry-cocoa into feat/crash-screenshot

brustolin · 2022-07-06T08:39:53Z

Hey @Swatinem and @armcknight. I had to wrote a little bit of C here, can you review that? Thanks!!

philipphofmann

Just one h comment to address and a review from native to merge this PR.

philipphofmann · 2022-07-06T08:47:59Z

Sources/SentryCrash/Recording/SentryCrashReportStore.c

+    uint32_t lowerBaseID = (uint32_t)(baseID & 0xffffffff);
+    g_nextUniqueIDLow = lowerBaseID;


I just debugged it and it wasn't for me. That would be weird.

Sources/SentryCrash/Recording/SentryCrash.m

Sources/SentryCrash/Recording/SentryCrashReportStore.c

…sentry-cocoa into feat/crash-screenshot

bruno-garcia · 2022-07-06T19:08:39Z

Nice! Congtats!

* master: ref: Remove unused SentryCrashDeadlock (#1941) feat: Add screenshot at crash (#1920) Add code docs for SentryScope (#1942)

* master: build: Remove trailing-whitespace (#1953) ci: Compile iOS13-Swift sample (#1951) release: 7.20.0 meta: Fix changelog (#1950) feat: Add sample rate in the baggage header, remove Userid and Transaction (#1936) build: Upate Brewfile.lock and Gemfile.lock (#1947) meta: Add Pre Commit Hook (#1946) ref: Remove unused SentryCrashDeadlock (#1941) feat: Add screenshot at crash (#1920) Add code docs for SentryScope (#1942)

Was forgotten to add in GH-1920.

brustolin added 3 commits June 22, 2022 16:01

SentryCrash screenshot callback

4ba0322

add screenshot attachment

9494db5

Merge branch 'master' into feat/crash-screenshot

cc44cca

getsentry-bot and others added 3 commits June 27, 2022 09:28

Format code

aca1443

Update CHANGELOG.md

a4bad19

Merge branch 'feat/crash-screenshot' of https://github.com/getsentry/…

8c5465c

…sentry-cocoa into feat/crash-screenshot

philipphofmann reviewed Jun 28, 2022

View reviewed changes

brustolin and others added 5 commits June 28, 2022 15:29

Apply suggestions from code review

66820bc

Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>

Improvements with file interactions

23d3536

Merge branch 'master' into feat/crash-screenshot

ae6b9e0

string format

d60054c

Format code

4eadd21

philipphofmann reviewed Jun 29, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Sources/SentryCrash/Recording/SentryCrashReportStore.h Show resolved Hide resolved

Sources/Sentry/SentryScreenshotIntegration.m Outdated Show resolved Hide resolved

Sources/Sentry/SentryScreenshot.m Show resolved Hide resolved

philipphofmann reviewed Jun 29, 2022

View reviewed changes

brustolin and others added 12 commits June 29, 2022 10:02

Merge branch 'master' into feat/crash-screenshot

dcfb4f9

Merge branch 'feat/crash-screenshot' of https://github.com/getsentry/…

7f66d8d

…sentry-cocoa into feat/crash-screenshot

Report sink test

a723c7a

integration test

636bcbe

Update SentryScreenshot.m

511864a

Merge branch 'master' into feat/crash-screenshot

db0865e

tests

e2fa1f2

Format code

ceb5fbe

Update SentryCrash.m

453dddf

Merge branch 'feat/crash-screenshot' of https://github.com/getsentry/…

cbe448f

…sentry-cocoa into feat/crash-screenshot

Update SentryCrashReportStore.c

60a7c2e

Format code

5229655

brustolin marked this pull request as ready for review July 1, 2022 09:24

bruno-garcia requested a review from kevinrenskers July 5, 2022 12:03

philipphofmann reviewed Jul 5, 2022

View reviewed changes

brustolin and others added 5 commits July 6, 2022 10:29

Apply suggestions from code review

da48b93

Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>

testfs

dbf5d0b

Merge branch 'feat/crash-screenshot' of https://github.com/getsentry/…

bd84076

…sentry-cocoa into feat/crash-screenshot

Update SentryCrash.m

64845d0

Format code

6a66e78

brustolin requested a review from armcknight July 6, 2022 08:38

philipphofmann reviewed Jul 6, 2022

View reviewed changes

Swatinem approved these changes Jul 6, 2022

View reviewed changes

Sources/SentryCrash/Recording/SentryCrashReportStore.c Show resolved Hide resolved

brustolin and others added 5 commits July 6, 2022 11:36

Update SentryCrashReportStore_Tests.m

0b5531a

Format code

9b880f6

more fixes

8c66ac8

Merge branch 'feat/crash-screenshot' of https://github.com/getsentry/…

068179d

…sentry-cocoa into feat/crash-screenshot

Merge branch 'master' into feat/crash-screenshot

bf6008e

bruno-garcia approved these changes Jul 6, 2022

View reviewed changes

bruno-garcia merged commit b711c79 into master Jul 6, 2022

bruno-garcia deleted the feat/crash-screenshot branch July 6, 2022 19:08

kevinrenskers added a commit that referenced this pull request Jul 7, 2022

Merge branch 'master' into feat/extra-controller-breadcrumb-data

6ed0aac

* master: ref: Remove unused SentryCrashDeadlock (#1941) feat: Add screenshot at crash (#1920) Add code docs for SentryScope (#1942)

philipphofmann mentioned this pull request Oct 20, 2022

Finish and capture transaction/span bound to the Scope in case of a fatal crash #2306

Open

denrase mentioned this pull request Oct 25, 2022

Feat: Screenshot Attachment getsentry/sentry-dart#1088

Merged

5 tasks

philipphofmann added a commit that referenced this pull request Jan 17, 2023

docs: Mention attachScreenshot works for crashes

8906b7e

Was forgotten to add in GH-1920.

philipphofmann mentioned this pull request Jan 17, 2023

docs: Mention attachScreenshot works for crashes #2620

Merged

philipphofmann added a commit that referenced this pull request Jan 18, 2023

docs: Mention attachScreenshot works for crashes (#2620)

088f732

Was forgotten to add in GH-1920.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add screenshot at crash #1920

feat: Add screenshot at crash #1920

brustolin commented Jun 27, 2022 •

edited

github-actions bot commented Jun 27, 2022 •

edited

philipphofmann left a comment

philipphofmann left a comment

philipphofmann left a comment

philipphofmann Jul 5, 2022

brustolin Jul 6, 2022

philipphofmann Jul 6, 2022

philipphofmann Jul 5, 2022

brustolin Jul 6, 2022

philipphofmann commented Jul 5, 2022

brustolin commented Jul 6, 2022

philipphofmann left a comment •

edited

philipphofmann Jul 6, 2022

bruno-garcia commented Jul 6, 2022

		uint32_t lowerBaseID = (uint32_t)(baseID & 0xffffffff);
		g_nextUniqueIDLow = lowerBaseID;

feat: Add screenshot at crash #1920

feat: Add screenshot at crash #1920

Conversation

brustolin commented Jun 27, 2022 • edited

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Jun 27, 2022 • edited

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann Jul 5, 2022

Choose a reason for hiding this comment

brustolin Jul 6, 2022

Choose a reason for hiding this comment

philipphofmann Jul 6, 2022

Choose a reason for hiding this comment

philipphofmann Jul 5, 2022

Choose a reason for hiding this comment

brustolin Jul 6, 2022

Choose a reason for hiding this comment

philipphofmann commented Jul 5, 2022

brustolin commented Jul 6, 2022

philipphofmann left a comment • edited

Choose a reason for hiding this comment

philipphofmann Jul 6, 2022

Choose a reason for hiding this comment

bruno-garcia commented Jul 6, 2022

brustolin commented Jun 27, 2022 •

edited

github-actions bot commented Jun 27, 2022 •

edited

philipphofmann left a comment •

edited