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: monitor for new metric values via sampling and notifications #2493

Merged
merged 75 commits into from
Jan 18, 2023

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Dec 6, 2022

💡 Motivation and Context

For https://github.com/getsentry/team-profiling/issues/121

💚 How did you test it?

Mocked out wrappers so we can mock and test the various parts of the implementation:

  • SentryNSProcessInfoWrapper
  • SentryNSTimerWrapper
  • the various system calls we need to use (SentrySystemWrapper)
  • wrappers for dispatch_source_t, which needed SentryDispatchSourceFactory and SentryDispatchSourceWrapper since it's a different kind of object that must be initialized a bit differently

Presupply them with values and make sure that after running the profiler in the same way as the other tests in its suite, that the payload contains the expected entries.

📝 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

  • Explore alternatives to NSTimer, and/or move as much work as possible off the main queue/thread.
  • Explore alternatives to the current syscalls we use to get cpu usage and memory footprint, as those will take up the most overhead.

@github-actions
Copy link

github-actions bot commented Dec 6, 2022

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

Generated by 🚫 dangerJS against bccfd3c

@github-actions
Copy link

github-actions bot commented Dec 6, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1239.65 ms 1259.26 ms 19.61 ms
Size 20.76 KiB 419.62 KiB 398.86 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
b6ba04e 1217.45 ms 1248.92 ms 31.47 ms
feba2be 1246.67 ms 1254.64 ms 7.97 ms
443723a 1205.24 ms 1220.52 ms 15.28 ms
b6ba04e 1230.48 ms 1253.20 ms 22.72 ms
c6504da 1232.06 ms 1243.28 ms 11.22 ms
621ba9b 1190.66 ms 1230.84 ms 40.18 ms
56ec5d0 1236.65 ms 1261.90 ms 25.25 ms
3a31fc9 1237.35 ms 1249.02 ms 11.67 ms
7c5d161 1224.38 ms 1249.66 ms 25.28 ms
3f1be0f 1208.12 ms 1225.72 ms 17.60 ms

App size

Revision Plain With Sentry Diff
b6ba04e 20.76 KiB 414.45 KiB 393.69 KiB
feba2be 20.76 KiB 414.45 KiB 393.69 KiB
443723a 20.76 KiB 414.44 KiB 393.68 KiB
b6ba04e 20.76 KiB 414.44 KiB 393.68 KiB
c6504da 20.76 KiB 414.44 KiB 393.69 KiB
621ba9b 20.76 KiB 414.45 KiB 393.69 KiB
56ec5d0 20.76 KiB 414.44 KiB 393.69 KiB
3a31fc9 20.76 KiB 414.45 KiB 393.69 KiB
7c5d161 20.76 KiB 414.44 KiB 393.68 KiB
3f1be0f 20.76 KiB 414.44 KiB 393.69 KiB

Previous results on branch: armcknight/feat/profiling-metrics

Startup times

Revision Plain With Sentry Diff
0ab5059 1196.38 ms 1222.76 ms 26.38 ms
782048b 1213.43 ms 1230.02 ms 16.59 ms
e7a8334 1206.92 ms 1231.94 ms 25.02 ms
1eac574 1216.45 ms 1236.66 ms 20.21 ms
812512e 1229.71 ms 1243.08 ms 13.37 ms
61442a2 1231.66 ms 1263.54 ms 31.88 ms
3db4bd8 1218.60 ms 1229.80 ms 11.20 ms
befe418 1226.51 ms 1243.35 ms 16.84 ms
6b23ad3 1223.04 ms 1251.24 ms 28.20 ms
60ee0ca 1235.16 ms 1235.36 ms 0.20 ms

App size

Revision Plain With Sentry Diff
0ab5059 20.75 KiB 409.56 KiB 388.80 KiB
782048b 20.75 KiB 410.54 KiB 389.78 KiB
e7a8334 20.75 KiB 412.41 KiB 391.66 KiB
1eac574 20.76 KiB 419.62 KiB 398.86 KiB
812512e 20.76 KiB 419.15 KiB 398.39 KiB
61442a2 20.75 KiB 410.82 KiB 390.07 KiB
3db4bd8 20.76 KiB 419.22 KiB 398.46 KiB
befe418 20.75 KiB 385.33 KiB 364.58 KiB
6b23ad3 20.75 KiB 412.57 KiB 391.82 KiB
60ee0ca 20.76 KiB 412.85 KiB 392.08 KiB

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

@armcknight, you asked for ideas on how to test the code in this PR. For unit test, you could add some new wrappers around the system APIs we are calling, similar to for example SentryNSNotificationCenterWrapper or SentryDisplayLinkWrapper. For integration tests, this is going to be a bit more tricky. I added two buttons to the iOS-Swift sample app with #2476:

  1. HighCpuLoad: it does some heavy calculation in a loop increasing the CPU load to 100%.
  2. DiskWriteException: It continuously writes data to disk

You could add another button for generating high memory pressure. Something similar to

@IBAction func oomCrash(_ sender: Any) {
DispatchQueue.main.async {
let megaByte = 1_024 * 1_024
let memoryPageSize = NSPageSize()
let memoryPages = megaByte / memoryPageSize
while true {
// Allocate one MB and set one element of each memory page to something.
let ptr = UnsafeMutablePointer<Int8>.allocate(capacity: megaByte)
for i in 0..<memoryPages {
ptr[i * memoryPageSize] = 40
}
}
}
}

Your UI tests could check the metrics before clicking these buttons and then compare them to the values after clicking those buttons.

Sources/Sentry/SentryMetricProfiler.mm Outdated Show resolved Hide resolved
Sources/Sentry/SentryMetricProfiler.mm Outdated Show resolved Hide resolved
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

That's great stuff in there 😀. We can use some parts of that for transactions in the future as well. I'm looking forward to this PR getting merged.

Sources/Sentry/SentryMetricProfiler.h Outdated Show resolved Hide resolved
Sources/Sentry/SentryMetricProfiler.mm Outdated Show resolved Hide resolved
Sources/Sentry/SentrySystemWrapper.h Outdated Show resolved Hide resolved
Sources/Sentry/SentrySystemWrapper.mm Outdated Show resolved Hide resolved
Sources/Sentry/SentrySystemWrapper.mm Show resolved Hide resolved
Sources/Sentry/SentryNSProcessInfoWrapper.h Outdated Show resolved Hide resolved
Sources/Sentry/SentryMetricProfiler.mm Outdated Show resolved Hide resolved
Sources/Sentry/SentryMetricProfiler.mm Outdated Show resolved Hide resolved
Sources/Sentry/SentryMetricProfiler.mm Outdated Show resolved Hide resolved
Sources/Sentry/SentryMetricProfiler.mm Show resolved Hide resolved
@armcknight armcknight force-pushed the armcknight/feat/profiling-metrics branch from 8600fdd to 2b727d3 Compare January 10, 2023 22:39
@armcknight
Copy link
Member Author

@philipphofmann FYI after discussing with @phacops and @viglia we decided to scale back the metrics we will collect for now to just CPU usage and RAM footprint. The others have been removed and i'm saving them in a patch if we want to readd them later.

@armcknight
Copy link
Member Author

armcknight commented Jan 12, 2023

I pushed a couple commits to fix some test issues codecov uncovered:

  • new sentry error factory function
  • early returns from gathering cpu/memory metrics on api errors
  • making sure frame render and rates are included in payloads

I had one other one planned but I'm going to defer it to the profile slicing PR since that's the target domain for those changes anyways:

  • trimming out frame render timestamps outside of transaction/profile bounds

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Great stuff @armcknight. A few minor things. LGTM

Comment on lines +189 to +197
# if defined(TEST) || defined(TESTCI)
// we don't currently validate the timestamps in tests, and the mock doesn't provide
// realistic ones, so they'd fail the checks below. just write them directly into the data
// structure so we can count *how many* were recorded
[relativeFrameInfo addObject:@{
@"elapsed_since_start_ns" : @(frameRenderStart),
@"value" : @(frameRenderStart),
}];
return;
Copy link
Member

Choose a reason for hiding this comment

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

m: Why can't you use useFramesTracker to test this with mocked values?

What you could do instead of this is to just call processFrameRenders directly in your tests with your desired values. You could achieve this by using a test category making processFrameRenders public in tests or use Dynamic(profiler).processFrameRenders().

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I thought about it but wasn't quite sure how to mock the values so that the original logic won't wind up crashing with negative durations, which is what the early returns in the logic are for. This workaround essentially allows us to ensure that the frame tracker did in fact record frame render info and pass it to the profiler. As I mentioned in #2493 (comment), I will write more tests to ensure the slicing logic in here and added in that PR are tested for correctness. Are you OK kicking that can down the road a bit more?

Copy link
Member

Choose a reason for hiding this comment

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

Are you OK kicking that can down the road a bit more?

Obviously, otherwise, I wouldn't have approved your PR.

#import "SentryError.h"
#import <mach/mach.h>

@implementation SentrySystemWrapper
Copy link
Member

Choose a reason for hiding this comment

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

m: Unit testing this for the exact functionality isn't going to work, but what we could do at least would be to add tests checking if the methods here return some meaningful data. For example, for cpuUsagePerCore we could check that each core has usage between 0% and 100% or something similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that can work, and that it returns data for more than 0 cores.

Copy link
Member Author

Choose a reason for hiding this comment

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

added tests for system, NSProcessInfo and NSTimer wrappers in 48f2b5a and 08c0979

@@ -6,6 +6,15 @@

#import "SentryMachLogging.hpp"

uint64_t
timeIntervalToNanoseconds(double seconds)
Copy link
Member

Choose a reason for hiding this comment

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

l: Maybe a bit cleaner.

Suggested change
timeIntervalToNanoseconds(double seconds)
timeIntervalToNanoseconds(NSTimeInterval seconds)

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately can't do it, because Foundation needs to be imported, and then for some reason that breaks a bunch of weird stuff, the compiler won't find things like NSString/NSObject. I wasn't able to crack it 😕 It's weird because it's already importing foundation in the implementation file, but adding that to the header takes it all down ¯\(ツ)

@armcknight
Copy link
Member Author

@philipphofmann makes no sense why testCustomURLProtocol_BlocksAllRequests1 is consistently failing based on the diff of the last few commits. I think it should be ignored.

@philipphofmann philipphofmann changed the base branch from 8.0.0 to main January 16, 2023 15:05
@armcknight
Copy link
Member Author

Oh, it appears that it's something to do with how NSTimerWrapper is being used for the network tracker integration tests 🤔

@armcknight armcknight merged commit b2f82fa into main Jan 18, 2023
@armcknight armcknight deleted the armcknight/feat/profiling-metrics branch January 18, 2023 17: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.

5 participants