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.

Sign up for GitHub

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

Jump to bottom

fix: report instances of dropped profiles #3108

Merged
merged 4 commits into from
Jun 27, 2023
Merged

fix: report instances of dropped profiles #3108

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5dce7e
fix: report instances of dropped profiles
armcknight Jun 16, 2023
8ee7f08
fix build
armcknight Jun 22, 2023
c6bcf82
add new discard reason for insufficient data
armcknight Jun 27, 2023
20e23fd
Merge remote-tracking branch 'origin/main' into armcknight/fix/report…
armcknight Jun 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions Sources/Sentry/SentryProfiler.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,21 +186,26 @@
NSDictionary<NSString *, id> *
serializedProfileData(NSDictionary<NSString *, id> *profileData, SentryTransaction *transaction,
SentryId *profileID, NSString *truncationReason, NSString *environment, NSString *release,
NSDictionary<NSString *, id> *serializedMetrics, NSArray<SentryDebugMeta *> *debugMeta)
NSDictionary<NSString *, id> *serializedMetrics, NSArray<SentryDebugMeta *> *debugMeta,
SentryHub *hub)
{
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.

getsentry/snuba#4407

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

return nil;
}

// slice the profile data to only include the samples/metrics within the transaction
const auto slicedSamples = slicedProfileSamples(samples, transaction);
if (slicedSamples.count < 2) {
SENTRY_LOG_DEBUG(@"Not enough samples in profile during the transaction");
[hub.getClient recordLostEvent:kSentryDataCategoryProfile
reason:kSentryDiscardReasonEventProcessor];
return nil;
}
const auto payload = [NSMutableDictionary<NSString *, id> dictionary];
Expand Down Expand Up @@ -592,7 +597,8 @@ + (void)useFramesTracker:(SentryFramesTracker *)framesTracker
?: _gCurrentProfiler->_hub.getClient.options.environment,
_gCurrentProfiler->_hub.getClient.options.releaseName,
[_gCurrentProfiler->_metricProfiler serializeForTransaction:transaction],
[_gCurrentProfiler->_debugImageProvider getDebugImagesCrashed:NO]);
[_gCurrentProfiler->_debugImageProvider getDebugImagesCrashed:NO],
_gCurrentProfiler -> _hub);
}

+ (void)timeoutAbort
Expand Down