feat: Update view hierarchy attachment format to Json #2491

brustolin · 2022-12-05T15:58:15Z

📜 Description

Change view hierarchy format to conform to RFC 33

💡 Motivation and Context

close #2467

💚 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-12-05T15:59:40Z

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

Generated by 🚫 dangerJS against 193a2f7

codecov · 2022-12-05T16:15:06Z

Codecov Report

Merging #2491 (193a2f7) into 8.0.0 (1018de3) will increase coverage by 0.10%.
The diff coverage is 95.63%.

Additional details and impacted files

@@            Coverage Diff             @@
##            8.0.0    #2491      +/-   ##
==========================================
+ Coverage   78.94%   79.04%   +0.10%     
==========================================
  Files         240      243       +3     
  Lines       22124    22259     +135     
  Branches     9768     9839      +71     
==========================================
+ Hits        17466    17595     +129     
- Misses       4206     4213       +7     
+ Partials      452      451       -1

Impacted Files	Coverage Δ
Sources/Sentry/PrivateSentrySDKOnly.m	`91.17% <0.00%> (-4.21%)`	⬇️
Sources/Sentry/SentryEnvelope.m	`88.55% <83.33%> (-0.84%)`	⬇️
Sources/Sentry/SentryAttachment.m	`92.85% <87.50%> (-7.15%)`	⬇️
Sources/Sentry/SentryViewHierarchy.m	`98.97% <98.87%> (+52.82%)`	⬆️
Sources/Sentry/SentryCrashReportSink.m	`88.88% <100.00%> (ø)`
Sources/Sentry/SentryEnvelopeAttachmentHeader.m	`100.00% <100.00%> (ø)`
Sources/Sentry/SentryEnvelopeItemHeader.m	`100.00% <100.00%> (ø)`
Sources/Sentry/SentryScope.m	`96.72% <100.00%> (+0.10%)`	⬆️
Sources/Sentry/SentrySerialization.m	`87.84% <100.00%> (+0.13%)`	⬆️
Sources/Sentry/SentryViewHierarchyIntegration.m	`88.88% <100.00%> (-0.86%)`	⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1018de3...193a2f7. Read the comment docs.

github-actions · 2022-12-05T16:16:34Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1196.53 ms	1209.35 ms	12.82 ms
Size	20.75 KiB	404.20 KiB	383.44 KiB

Baseline results on branch: 8.0.0

Startup times

Revision	Plain	With Sentry	Diff
`5da2d48`	1209.52 ms	1232.16 ms	22.64 ms
`9f8d429`	1239.57 ms	1255.12 ms	15.55 ms
`3fdb749`	1227.42 ms	1248.48 ms	21.06 ms
`b9c9598`	1232.86 ms	1253.84 ms	20.98 ms
`b696ae1`	1229.64 ms	1246.94 ms	17.30 ms
`d8c347f`	1240.53 ms	1257.70 ms	17.17 ms
`459ae5e`	1223.84 ms	1250.14 ms	26.31 ms
`a8ac44f`	1198.63 ms	1231.08 ms	32.45 ms
`a0c8696`	1239.33 ms	1261.92 ms	22.59 ms
`88ac2c2`	1223.04 ms	1243.12 ms	20.08 ms

App size

Revision	Plain	With Sentry	Diff
`5da2d48`	20.75 KiB	383.77 KiB	363.01 KiB
`9f8d429`	20.75 KiB	383.40 KiB	362.65 KiB
`3fdb749`	20.75 KiB	383.81 KiB	363.06 KiB
`b9c9598`	20.75 KiB	383.77 KiB	363.01 KiB
`b696ae1`	20.75 KiB	383.77 KiB	363.01 KiB
`d8c347f`	20.75 KiB	383.77 KiB	363.02 KiB
`459ae5e`	20.75 KiB	383.50 KiB	362.75 KiB
`a8ac44f`	20.75 KiB	405.17 KiB	384.41 KiB
`a0c8696`	20.75 KiB	383.89 KiB	363.14 KiB
`88ac2c2`	20.75 KiB	373.61 KiB	352.86 KiB

Previous results on branch: feat/view-hierarchy

Startup times

Revision	Plain	With Sentry	Diff
`fbb44fb`	1217.22 ms	1239.00 ms	21.78 ms
`4ffd327`	1217.31 ms	1248.26 ms	30.95 ms
`e9ee171`	1203.84 ms	1237.76 ms	33.92 ms
`2e2a243`	1238.42 ms	1268.50 ms	30.08 ms
`797af24`	1243.04 ms	1279.43 ms	36.39 ms
`806bdb8`	1248.11 ms	1252.32 ms	4.21 ms
`5551192`	1188.55 ms	1234.82 ms	46.27 ms
`b989d78`	1196.17 ms	1241.08 ms	44.91 ms
`ddb8e6c`	1213.39 ms	1239.22 ms	25.83 ms

App size

Revision	Plain	With Sentry	Diff
`fbb44fb`	20.75 KiB	405.61 KiB	384.85 KiB
`4ffd327`	20.75 KiB	406.30 KiB	385.54 KiB
`e9ee171`	20.75 KiB	384.62 KiB	363.86 KiB
`2e2a243`	20.75 KiB	406.25 KiB	385.50 KiB
`797af24`	20.75 KiB	406.78 KiB	386.03 KiB
`806bdb8`	20.75 KiB	406.69 KiB	385.93 KiB
`5551192`	20.75 KiB	406.30 KiB	385.54 KiB
`b989d78`	20.75 KiB	406.97 KiB	386.21 KiB
`ddb8e6c`	20.75 KiB	406.81 KiB	386.06 KiB

…oa into feat/view-hierarchy

Sources/Sentry/Public/SentryAttachment.h

Sources/Sentry/SentryViewHierarchy.m

kevinrenskers · 2022-12-06T12:13:31Z

Sources/Sentry/SentryViewHierarchy.m

+    tryJson(sentrycrashjson_addFloatingPointElement(context, "x", view.frame.origin.x));
+    tryJson(sentrycrashjson_addFloatingPointElement(context, "y", view.frame.origin.y));
+    tryJson(sentrycrashjson_addFloatingPointElement(context, "alpha", view.alpha));
+    tryJson(sentrycrashjson_addBooleanElement(context, "visible", !view.hidden));


Why are we not just using [NSJSONSerialization dataWithJSONObject:]? I don't really understand why we're creating JSON in such a strange way with sentrycrashjson_addFloatingPointElement.

This way we can write directly to file, or to memory buffer, no need to first create a dictionary, and then serialize the dictionary.
This will be used during signal handling, the faster and less memory allocation being done the better.

Yes, but we already call into objective C code to make this work. We should only call async safe code in a signal handler, but as we are already violating that contract, I'm unsure if it's worth the effort. We call this only after SentryCrash wrote the crash report. In some cases, we will crash when calling into ObjC, but I think it doesn't matter how often we do it. Once is enough.

Is not about safety, because this code itself is called from an objc class. Is about performance, this is the fastest way to do it, and since we may deal of view hierarchy with hundreds of views, the faster the better.

How much faster? Do you have benchmarks?

You convinced me with during signal handling.

…oa into feat/view-hierarchy

kevinrenskers

Personally I don't think the JSON code is a good thing to maintain, but if you say it makes a significant difference with CPU / memory usage, I guess it makes sense?

I also feel the goto code is a footgun, but I'll leave final judgement for @philipphofmann.

brustolin · 2022-12-07T11:51:33Z

@kevinrenskers goto is just another language feature, like any other feature, you can use it right, you can use it wrong, thats what tests are for.

But in this case I have another solution, I will use it to make you happy! 😁

kevinrenskers · 2022-12-07T11:55:05Z

Well I didn't deny the PR, like I said I'll just leave it up to Philipp :)

philipphofmann

First pass. Still need to review a couple of files.

CHANGELOG.md

Sources/Sentry/Public/SentryAttachment.h

Sources/Sentry/SentryEnvelope.m

Sources/Sentry/include/HybridPublic/SentryEnvelope.h

Sources/Sentry/SentrySerialization.m

Sources/Sentry/include/HybridPublic/SentryEnvelope.h

Sources/Sentry/SentryEnvelope.m

Tests/SentryTests/Protocol/SentryAttachment+Equality.m

philipphofmann · 2022-12-09T10:26:49Z

I also feel the goto code is a footgun, but I'll leave final judgement for @philipphofmann.

I also don't like goto in general, but let's see 😁

philipphofmann

I'm unsure about writing the hierarchy with C APIs. I explained my concerns in my comment. Once we resolved that I'm going to give this another review.

philipphofmann · 2022-12-09T10:46:28Z

Sources/Sentry/SentryViewHierarchy.m

+    const char *path = [filePath UTF8String];
+    int fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0644);
+    if (fd < 0) {
+        SENTRY_LOG_DEBUG(@"Could not open file %s for writing: %s", path, strerror(errno));
+        return false;
+    }
+


m: Why don't we use ObjC here? SENTRY_LOG_DEBUG calls into ObjC code.

Why don't we use ObjC here?

What do you mean? Almost everything here is objc.
If you're talking about the serialization approach in the other function, the explanation is here

Tests/SentryTests/Integrations/ViewHierarchy/TestSentryViewHierarchy.h

philipphofmann · 2022-12-09T10:55:06Z

Sources/Sentry/SentryViewHierarchy.m

+    tryJson(sentrycrashjson_addFloatingPointElement(context, "x", view.frame.origin.x));
+    tryJson(sentrycrashjson_addFloatingPointElement(context, "y", view.frame.origin.y));
+    tryJson(sentrycrashjson_addFloatingPointElement(context, "alpha", view.alpha));
+    tryJson(sentrycrashjson_addBooleanElement(context, "visible", !view.hidden));


Yes, but we already call into objective C code to make this work. We should only call async safe code in a signal handler, but as we are already violating that contract, I'm unsure if it's worth the effort. We call this only after SentryCrash wrote the crash report. In some cases, we will crash when calling into ObjC, but I think it doesn't matter how often we do it. Once is enough.

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

…oa into feat/view-hierarchy

Sources/Sentry/include/SentryViewHierarchy.h

Sources/Sentry/include/SentryAttachment+Private.h

philipphofmann · 2022-12-15T16:25:18Z

Sources/Sentry/SentryScope.m

@@ -396,6 +396,19 @@ - (void)addAttachment:(SentryAttachment *)attachment
    }
 }

+- (void)addCrashReportAttachmentInPath:(NSString *)filePath
+{
+    if ([filePath.lastPathComponent isEqualToString:@"view-hierarchy.json"]) {


Can you please address this comment ⬆️

Sources/Sentry/Public/SentryEnvelopeItemHeader.h

philipphofmann

LGTM

marandaneto · 2022-12-20T14:22:27Z

@philipphofmann @brustolin is there docs for that already?

philipphofmann · 2022-12-20T16:34:23Z

I'm pretty sure there aren't. @brustolin, please add some after your PTO. I created an issue getsentry/sentry-docs#5984.

View hierarchy update

99c8dd5

Format code

a214728

brustolin changed the title ~~feat: Update view hierarchy format~~ feat: Update view hierarchy attachment format Dec 5, 2022

brustolin added 2 commits December 5, 2022 17:33

more test and changelog

03b0ace

Merge branch 'feat/view-hierarchy' of github.com:getsentry/sentry-coc…

bfcd3b4

…oa into feat/view-hierarchy

brustolin marked this pull request as ready for review December 5, 2022 16:34

brustolin requested review from philipphofmann and armcknight as code owners December 5, 2022 16:34

getsentry-bot and others added 4 commits December 5, 2022 16:34

Format code

292d4ab

more tests

f756f27

Merge branch 'feat/view-hierarchy' of github.com:getsentry/sentry-coc…

ba76c7a

…oa into feat/view-hierarchy

Format code

bcfb522

kevinrenskers reviewed Dec 6, 2022

View reviewed changes

Sources/Sentry/Public/SentryAttachment.h Outdated Show resolved Hide resolved

kevinrenskers reviewed Dec 6, 2022

View reviewed changes

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

kevinrenskers reviewed Dec 6, 2022

View reviewed changes

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

kevinrenskers reviewed Dec 6, 2022

View reviewed changes

brustolin and others added 10 commits December 6, 2022 17:37

attachment Type and tests

5afeb90

Merge branch 'feat/view-hierarchy' of github.com:getsentry/sentry-coc…

af738ab

…oa into feat/view-hierarchy

Update TestSentryViewHierarchy.swift

45f25a2

Merge branch '8.0.0' into feat/view-hierarchy

4e4bac1

Format code

f63407c

Update SentryViewHierarchy.m

7250ee8

Merge branch 'feat/view-hierarchy' of github.com:getsentry/sentry-coc…

cfd74de

…oa into feat/view-hierarchy

Update SentrySerialization.m

31b47dd

Update SentrySerialization.m

e7fdb6a

Format code

cf9a5a7

kevinrenskers reviewed Dec 7, 2022

View reviewed changes

brustolin and others added 2 commits December 7, 2022 12:45

add view controller

cf3fe22

Format code

3492aee

Merge branch '8.0.0' into feat/view-hierarchy

2ee93d4

philipphofmann reviewed Dec 9, 2022

View reviewed changes

brustolin and others added 5 commits December 9, 2022 12:19

Apply suggestions from code review

c2d5aa7

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

adjustments

dfb8151

Merge branch 'feat/view-hierarchy' of github.com:getsentry/sentry-coc…

c609944

…oa into feat/view-hierarchy

serialization

d564015

Update PrivateSentrySDKOnly.h

3fdcf64

brustolin requested a review from philipphofmann December 12, 2022 15:03

krystofwoldrich reviewed Dec 13, 2022

View reviewed changes

Sources/Sentry/include/SentryViewHierarchy.h Show resolved Hide resolved

capture viewhierarchy for hybrid SDKs

5c5e72d

philipphofmann reviewed Dec 15, 2022

View reviewed changes

philipphofmann approved these changes Dec 16, 2022

View reviewed changes

brustolin changed the title ~~feat: Update view hierarchy attachment format~~ feat: Update view hierarchy attachment format to Json Dec 16, 2022

Merge branch '8.0.0' into feat/view-hierarchy

193a2f7

philipphofmann merged commit 1a18683 into 8.0.0 Dec 20, 2022

philipphofmann deleted the feat/view-hierarchy branch December 20, 2022 09:20

philipphofmann mentioned this pull request Dec 20, 2022

Apple View Hierarchy Docs getsentry/sentry-docs#5984

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Update view hierarchy attachment format to Json #2491

feat: Update view hierarchy attachment format to Json #2491

brustolin commented Dec 5, 2022 •

edited

Loading

github-actions bot commented Dec 5, 2022 •

edited

Loading

codecov bot commented Dec 5, 2022 •

edited

Loading

github-actions bot commented Dec 5, 2022 •

edited

Loading

Baseline results on branch: 8.0.0

Startup times

App size

Previous results on branch: feat/view-hierarchy

Startup times

App size

kevinrenskers Dec 6, 2022

brustolin Dec 6, 2022

philipphofmann Dec 9, 2022

brustolin Dec 9, 2022

philipphofmann Dec 15, 2022

philipphofmann Dec 16, 2022

kevinrenskers left a comment

brustolin commented Dec 7, 2022

kevinrenskers commented Dec 7, 2022

philipphofmann left a comment

philipphofmann commented Dec 9, 2022

philipphofmann left a comment

philipphofmann Dec 9, 2022

brustolin Dec 9, 2022 •

edited

Loading

philipphofmann Dec 9, 2022

philipphofmann Dec 15, 2022

philipphofmann left a comment

marandaneto commented Dec 20, 2022

philipphofmann commented Dec 20, 2022

feat: Update view hierarchy attachment format to Json #2491

feat: Update view hierarchy attachment format to Json #2491

Conversation

brustolin commented Dec 5, 2022 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Dec 5, 2022 • edited Loading

codecov bot commented Dec 5, 2022 • edited Loading

Codecov Report

github-actions bot commented Dec 5, 2022 • edited Loading

Performance metrics 🚀

Baseline results on branch: 8.0.0

Startup times

App size

Previous results on branch: feat/view-hierarchy

Startup times

App size

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrenskers left a comment

Choose a reason for hiding this comment

brustolin commented Dec 7, 2022

kevinrenskers commented Dec 7, 2022

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann commented Dec 9, 2022

philipphofmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brustolin Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

marandaneto commented Dec 20, 2022

philipphofmann commented Dec 20, 2022

brustolin commented Dec 5, 2022 •

edited

Loading

github-actions bot commented Dec 5, 2022 •

edited

Loading

codecov bot commented Dec 5, 2022 •

edited

Loading

github-actions bot commented Dec 5, 2022 •

edited

Loading

brustolin Dec 9, 2022 •

edited

Loading