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: Ignore specific MetricKit reports #3176

Closed
wants to merge 3 commits into from

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Jul 20, 2023

This is a proof of concept for ignoring MetricKit reports that stem from our SDK (#2847). For example, SentryNSDataSwizzling hooks into almost every file I/O operation. MetricKit detects this and assumes that it's responsible for a MXDiskWriteException, which doesn't make any sense. Therefore, we could ignore such events.

The idea is to walk through the stacktrace and check if the topmost non-system frame is, for example, from SentryNSDataSwizzling. As we don't have function names available on the device, cause symbolication won't work on the device in release, we have to work with memory addresses of functions. What could work is to get the IMP, which is the instruction address, then use local symbolication with dladdr to get the symbol address and do the same with the MetricKit stacktrace. Then, we can compare the symbol addresses. To know if it's a system frame, we can use the same approach as for SDK crash detection. We simply check if the path of the binary image starts with /System/Library/ or /usr/lib/, but again we need local symbolication to work for this. It's worth noting that we won't get function names with local symbolication, but I hope we can get the symbol addresses and the full paths of the binary images.

One downside of this approach is, that it won't work after app updates or OS updates.

I hacked this PR together to get some initial feedback.

@philipphofmann philipphofmann marked this pull request as draft July 20, 2023 09:05
@github-actions
Copy link

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

- Ignore specific MetricKit reports ([#3176](https://github.com/getsentry/sentry-cocoa/pull/3176))

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 215c461

@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Merging #3176 (215c461) into main (84fb4d9) will decrease coverage by 1.540%.
The diff coverage is 40.336%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3176       +/-   ##
=============================================
- Coverage   89.149%   87.609%   -1.540%     
=============================================
  Files          502       503        +1     
  Lines        53923     53680      -243     
  Branches     19341     18838      -503     
=============================================
- Hits         48072     47029     -1043     
- Misses        4997      5797      +800     
  Partials       854       854               
Impacted Files Coverage Δ
Sources/Sentry/SentryDependencyContainer.m 90.090% <0.000%> (-4.505%) ⬇️
Sources/Sentry/SentryMetricKitIntegration.m 5.369% <0.000%> (-94.236%) ⬇️
...tryCrash/Recording/Tools/SentryCrashSymbolicator.c 67.500% <50.000%> (-16.284%) ⬇️
Sources/Sentry/SentryAddressRetriever.m 67.857% <67.857%> (ø)
...tryTests/SentryCrash/SentryAddressRetrieverTests.m 100.000% <100.000%> (ø)

... and 91 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@github-actions
Copy link

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1238.96 ms 1249.54 ms 10.58 ms
Size 22.84 KiB 403.55 KiB 380.71 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
4386045 1225.67 ms 1244.90 ms 19.23 ms
9e96fd6 1207.20 ms 1229.40 ms 22.20 ms
b6ba04e 1217.45 ms 1248.92 ms 31.47 ms
06548c0 1226.71 ms 1252.37 ms 25.66 ms
407ff99 1250.10 ms 1269.78 ms 19.67 ms
8f397a7 1230.10 ms 1253.88 ms 23.77 ms
8c8654d 1214.43 ms 1223.88 ms 9.45 ms
257c2a9 1231.45 ms 1252.12 ms 20.67 ms
efb2222 1256.44 ms 1278.90 ms 22.46 ms
98a8c16 1243.33 ms 1257.86 ms 14.53 ms

App size

Revision Plain With Sentry Diff
4386045 22.84 KiB 403.07 KiB 380.23 KiB
9e96fd6 20.76 KiB 425.80 KiB 405.04 KiB
b6ba04e 20.76 KiB 414.45 KiB 393.69 KiB
06548c0 20.76 KiB 427.36 KiB 406.59 KiB
407ff99 20.76 KiB 427.87 KiB 407.10 KiB
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
8c8654d 22.84 KiB 403.18 KiB 380.33 KiB
257c2a9 20.76 KiB 401.36 KiB 380.60 KiB
efb2222 20.76 KiB 424.45 KiB 403.69 KiB
98a8c16 20.76 KiB 431.00 KiB 410.24 KiB

@philipphofmann
Copy link
Member Author

After giving this some thought, the backend most likely would be a better place to ignore such events. We only have to find the right place.

@armcknight
Copy link
Member

This could be a good reason to switch from the swizzle-based File I/O detection to profile-based. All backend, and no client changes required since we can do this today with profiling by just flipping a switch (except for removing the swizzle, which, viewed purely in client times, would be a strict improvement). I guess the hardest part would be migrating the current perf issues to the new prof issues.

cc @indragiek because we've talked about this in the past.

@armcknight
Copy link
Member

We're going to close this draft for now as we're not sure it makes sense for the client and @philipphofmann is on leave for a while.

@armcknight armcknight closed this Aug 30, 2023
@philipphofmann philipphofmann deleted the feat/ignore-metrickit branch October 12, 2023 08:42
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.

2 participants