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

feat: add more db info on span data #3231

Merged
merged 8 commits into from
Aug 29, 2023
Merged

feat: add more db info on span data #3231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
edfef05
feat: add more db info on span data
armcknight Aug 17, 2023
43fb9d0
changelog
armcknight Aug 17, 2023
87fed6c
tests
armcknight Aug 18, 2023
1f31a08
fix test
armcknight Aug 18, 2023
9fd258d
wip adding back accidentally removed assertions
armcknight Aug 29, 2023
5bf3225
Merge remote-tracking branch 'origin/main' into armcknight/feat/3212-…
armcknight Aug 29, 2023
8251982
put assert back in right spot
armcknight Aug 29, 2023
bc5e393
fix changelog
armcknight Aug 29, 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

### Features

- Report database backing store information for Core Data (#3231)

## 8.10.0

### Features
Expand Down
69 changes: 38 additions & 31 deletions Sources/Sentry/SentryCoreDataTracker.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,23 +44,18 @@ - (NSArray *)managedObjectContext:(NSManagedObjectContext *)context
}];

if (fetchSpan) {
[SentryLog
logWithMessage:[NSString stringWithFormat:
@"SentryCoreDataTracker automatically "
@"started a new span with description: %@, operation: %@",
fetchSpan.description, SENTRY_COREDATA_FETCH_OPERATION]
andLevel:kSentryLevelDebug];
SENTRY_LOG_DEBUG(@"SentryCoreDataTracker automatically started a new span with "
@"description: %@, operation: %@",
fetchSpan.description, fetchSpan.operation);
Comment on lines -47 to +49
Copy link
Member Author

Choose a reason for hiding this comment

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

Converted all the logs to the new macros. Also here's the second place I changed interpolating the operation constant for SentrySpan.operation.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for doing this 👍

} else {
[SentryLog
logWithMessage:
@"managedObjectContext:executeFetchRequest:error:originalImp: fetchSpan is nil."
andLevel:kSentryLevelError];
SENTRY_LOG_ERROR(
@"managedObjectContext:executeFetchRequest:error:originalImp: fetchSpan is nil.");
}

NSArray *result = original(request, error);

if (fetchSpan) {
[self mainThreadExtraInfo:fetchSpan];
[self addExtraInfoToSpan:fetchSpan withContext:context];

[fetchSpan setDataValue:[NSNumber numberWithInteger:result.count] forKey:@"read_count"];
[fetchSpan
Expand All @@ -78,35 +73,34 @@ - (BOOL)managedObjectContext:(NSManagedObjectContext *)context
originalImp:(BOOL(NS_NOESCAPE ^)(NSError **))original
{

__block id<SentrySpan> fetchSpan = nil;
__block id<SentrySpan> saveSpan = nil;
Comment on lines -81 to +76
Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed this variable

if (context.hasChanges) {
__block NSDictionary<NSString *, NSDictionary *> *operations =
[self groupEntitiesOperations:context];

[SentrySDK.currentHub.scope useSpan:^(id<SentrySpan> _Nullable span) {
fetchSpan = [span startChildWithOperation:SENTRY_COREDATA_SAVE_OPERATION
description:[self descriptionForOperations:operations
inContext:context]];
fetchSpan.origin = SentryTraceOriginAutoDBCoreData;
if (fetchSpan) {
[SentryLog
logWithMessage:[NSString
stringWithFormat:@"SentryCoreDataTracker automatically "
@"started a new span with description: %@, "
@"operation: %@",
fetchSpan.description, SENTRY_COREDATA_FETCH_OPERATION]
Copy link
Member Author

Choose a reason for hiding this comment

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

This should've been SENTRY_COREDATA_SAVE_OPERATION, but I did one better and just access SentrySpan.operation for both such logs (here and in fetches)

andLevel:kSentryLevelDebug];

[fetchSpan setDataValue:operations forKey:@"operations"];
}
saveSpan = [span startChildWithOperation:SENTRY_COREDATA_SAVE_OPERATION
description:[self descriptionForOperations:operations
inContext:context]];
saveSpan.origin = SentryTraceOriginAutoDBCoreData;
}];

if (saveSpan) {
SENTRY_LOG_DEBUG(@"SentryCoreDataTracker automatically started a new span with "
@"description: %@, operation: %@",
saveSpan.description, saveSpan.operation);

[saveSpan setDataValue:operations forKey:@"operations"];
} else {
SENTRY_LOG_ERROR(@"managedObjectContext:save:originalImp: saveSpan is nil");
}
Comment on lines -91 to +96
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this out of the useSpan: scope, like the code for fetch operations.

}

BOOL result = original(error);

if (fetchSpan) {
[self mainThreadExtraInfo:fetchSpan];
[fetchSpan finishWithStatus:result ? kSentrySpanStatusOk : kSentrySpanStatusInternalError];
if (saveSpan) {
[self addExtraInfoToSpan:saveSpan withContext:context];
[saveSpan finishWithStatus:result ? kSentrySpanStatusOk : kSentrySpanStatusInternalError];

SENTRY_LOG_DEBUG(@"SentryCoreDataTracker automatically finished span with status: %@",
result ? @"ok" : @"error");
Expand All @@ -115,11 +109,24 @@ - (BOOL)managedObjectContext:(NSManagedObjectContext *)context
return result;
}

- (void)mainThreadExtraInfo:(SentrySpan *)span
- (void)addExtraInfoToSpan:(SentrySpan *)span withContext:(NSManagedObjectContext *)context
Comment on lines -118 to +112
Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed this method as it's doing more than just main thread stuff now.

{
BOOL isMainThread = [NSThread isMainThread];

[span setDataValue:@(isMainThread) forKey:BLOCKED_MAIN_THREAD];
NSMutableArray<NSString *> *systems = [NSMutableArray<NSString *> array];
NSMutableArray<NSString *> *names = [NSMutableArray<NSString *> array];
[context.persistentStoreCoordinator.persistentStores enumerateObjectsUsingBlock:^(
__kindof NSPersistentStore *_Nonnull obj, NSUInteger idx, BOOL *_Nonnull stop) {
[systems addObject:obj.type];
if (obj.URL != nil) {
[names addObject:obj.URL.path];
} else {
[names addObject:@"(null)"];
Copy link
Contributor

Choose a reason for hiding this comment

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

h: I believe we need a better option to fallback. This is not user friendly and I'm foreseeing complains.

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'm open to any alternatives. Nobody has proposed any so far.

Copy link
Member Author

@armcknight armcknight Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I would also be interested to know the cases where the URL is nil. I expected it to be nil for in memory stores, but even that has a value like memory://563428497. It is probably nil if there is no persistent store/coordinator at all, but then I don't know what the complaint would be. In that case I question if we should even be reporting anything at all, but that kind of refactor is out of scope for this issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm open to any alternatives.

<App Name>:coredata anything really other than (null)

Copy link
Member Author

@armcknight armcknight Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We already gather the equivalent of that in the type.

In this case, we should send some kind of sentinel value so the frontend knows to render a special state equivalent to "there was no database". This seems as good as any.

If I were a customer I would rather know that this value was not available, vs having it covered up by a default value like what you're proposing. It could help find an upstream error.

Copy link
Member

@kahest kahest Aug 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with @armcknight here - in case this occurs a lot and provides not enough context we can always re-iterate.

}
}];
[span setDataValue:[systems componentsJoinedByString:@";"] forKey:@"db.system"];
[span setDataValue:[names componentsJoinedByString:@";"] forKey:@"db.name"];
Comment on lines +117 to +129
Copy link
Member Author

Choose a reason for hiding this comment

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

The new db.name and db.system data.


if (!isMainThread) {
return;
Expand Down
Loading
Loading