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

fix: report instances of dropped profiles #3108

Merged
merged 4 commits into from
Jun 27, 2023

Conversation

armcknight
Copy link
Member

We'd like to know when we discard profiles due to insufficient amounts of samples so we can assist customers in the frontend.

#skip-changelog

{
NSMutableArray<SentrySample *> *const samples = profileData[@"profile"][@"samples"];
// We need at least two samples to be able to draw a stack frame for any given function: one
// sample for the start of the frame and another for the end. Otherwise we would only have a
// stack frame with 0 duration, which wouldn't make sense.
if ([samples count] < 2) {
SENTRY_LOG_DEBUG(@"Not enough samples in profile");
[hub.getClient recordLostEvent:kSentryDataCategoryProfile
reason:kSentryDiscardReasonEventProcessor];
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on introducing a separate reason for this use case? Perhaps something like "insufficient_samples" or if that's too specific since the other reasons are more generic we could do something like "failed_validation"

Copy link
Member Author

Choose a reason for hiding this comment

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

I did consider it, but was put off by the scope of work, since it'd require changes to snuba. I think this is ok for now because this is the only combination of profile category event processor reason, if we hit trouble with it in the future getting overloaded let's change it then.

Copy link
Member

Choose a reason for hiding this comment

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

This seems like a small change in snuba to allow them through https://github.com/getsentry/snuba/blob/master/snuba/datasets/processors/outcomes_processor.py#L25 so ideally we do this right and not worry about having to migrate it later.

Copy link
Member Author

Choose a reason for hiding this comment

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

It'll also at least require changes to these docs: https://develop.sentry.dev/sdk/client-reports/#envelope-item-payload

Copy link
Member Author

Choose a reason for hiding this comment

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

What should we call the new reason? insufficient_data is my suggestion

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

done in c6bcf82

@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

Merging #3108 (20e23fd) into main (1d11695) will increase coverage by 0.036%.
The diff coverage is 60.000%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3108       +/-   ##
=============================================
+ Coverage   89.007%   89.043%   +0.036%     
=============================================
  Files          500       500               
  Lines        53926     53951       +25     
  Branches     19296     19317       +21     
=============================================
+ Hits         47998     48040       +42     
- Misses        4962      5060       +98     
+ Partials       966       851      -115     
Impacted Files Coverage Δ
Sources/Sentry/SentryProfiler.mm 83.595% <33.333%> (-0.723%) ⬇️
Sources/Sentry/SentryDiscardReasonMapper.m 60.000% <100.000%> (-1.112%) ⬇️
Tests/SentryProfilerTests/SentryProfilerTests.mm 98.728% <100.000%> (ø)
...ts/Networking/SentryDiscardReasonMapperTests.swift 100.000% <100.000%> (ø)

... and 35 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 1d11695...20e23fd. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Jun 22, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1247.02 ms 1250.68 ms 3.66 ms
Size 20.76 KiB 401.66 KiB 380.89 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
371db89 1226.40 ms 1251.54 ms 25.14 ms
adcc7d8 1225.90 ms 1245.08 ms 19.18 ms
407ff99 1250.10 ms 1269.78 ms 19.67 ms
bfa3487 1194.19 ms 1224.20 ms 30.01 ms
d257eb9 1206.98 ms 1227.50 ms 20.52 ms
06548c0 1226.71 ms 1252.37 ms 25.66 ms
17afc4b 1228.94 ms 1251.10 ms 22.16 ms
31208ed 1217.45 ms 1246.69 ms 29.24 ms
7bc3c0d 1212.35 ms 1228.94 ms 16.59 ms
ecd9ecd 1214.16 ms 1232.59 ms 18.43 ms

App size

Revision Plain With Sentry Diff
371db89 20.76 KiB 427.31 KiB 406.55 KiB
adcc7d8 20.76 KiB 426.15 KiB 405.39 KiB
407ff99 20.76 KiB 427.87 KiB 407.10 KiB
bfa3487 20.76 KiB 393.36 KiB 372.60 KiB
d257eb9 20.76 KiB 433.22 KiB 412.46 KiB
06548c0 20.76 KiB 427.36 KiB 406.59 KiB
17afc4b 20.76 KiB 436.25 KiB 415.49 KiB
31208ed 20.76 KiB 435.26 KiB 414.50 KiB
7bc3c0d 20.76 KiB 427.35 KiB 406.59 KiB
ecd9ecd 20.76 KiB 420.23 KiB 399.47 KiB

Previous results on branch: armcknight/fix/report-profile-drop-reason

Startup times

Revision Plain With Sentry Diff
7110218 1239.82 ms 1267.40 ms 27.58 ms

App size

Revision Plain With Sentry Diff
7110218 20.76 KiB 401.51 KiB 380.75 KiB

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

@armcknight armcknight merged commit c2ec420 into main Jun 27, 2023
@armcknight armcknight deleted the armcknight/fix/report-profile-drop-reason branch June 27, 2023 17:28
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.

3 participants