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

Fix threading issues capturing log messages as breadcrumbs #559

Merged
merged 3 commits into from
May 23, 2024
Merged

Fix threading issues capturing log messages as breadcrumbs #559

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
375608b
Change logic of adding breadcrumbs with given params to avoid extra U…
tustanivsky May 15, 2024
c0805b0
Add some threading considerations for adding breadcrumbs via output d…
tustanivsky May 15, 2024
10dc737
Update changelog
tustanivsky May 15, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Features

- Sentry-CLI now shares the diagnostic level set in the UE Editor settings ([#555](https://github.com/getsentry/sentry-unreal/pull/555))
- Fix threading issues capturing log messages as breadcrumbs ([#559](https://github.com/getsentry/sentry-unreal/pull/559))

### Dependencies

Expand Down
13 changes: 13 additions & 0 deletions plugin-dev/Source/Sentry/Private/Android/SentrySubsystemAndroid.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,19 @@ void SentrySubsystemAndroid::AddBreadcrumb(USentryBreadcrumb* breadcrumb)
breadcrumbAndroid->GetJObject());
}

void SentrySubsystemAndroid::AddBreadcrumbWithParams(const FString& Message, const FString& Category, const FString& Type, const TMap<FString, FString>& Data, ESentryLevel Level)
{
TSharedPtr<SentryBreadcrumbAndroid> breadcrumbAndroid = MakeShareable(new SentryBreadcrumbAndroid());
breadcrumbAndroid->SetMessage(Message);
breadcrumbAndroid->SetCategory(Category);
breadcrumbAndroid->SetType(Type);
breadcrumbAndroid->SetData(Data);
breadcrumbAndroid->SetLevel(Level);

FSentryJavaObjectWrapper::CallStaticMethod<void>(SentryJavaClasses::Sentry, "addBreadcrumb", "(Lio/sentry/Breadcrumb;)V",
breadcrumbAndroid->GetJObject());
}

void SentrySubsystemAndroid::ClearBreadcrumbs()
{
FSentryJavaObjectWrapper::CallStaticMethod<void>(SentryJavaClasses::Sentry, "clearBreadcrumbs", "()V");
Expand Down
1 change: 1 addition & 0 deletions plugin-dev/Source/Sentry/Private/Android/SentrySubsystemAndroid.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class SentrySubsystemAndroid : public ISentrySubsystem
virtual bool IsEnabled() override;
virtual ESentryCrashedLastRun IsCrashedLastRun() override;
virtual void AddBreadcrumb(USentryBreadcrumb* breadcrumb) override;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're no longer using the "plain" AddBreadcrumb, do we need it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's still a part of the public API so I think we should leave it even though we're not using it internally anymore.

virtual void AddBreadcrumbWithParams(const FString& Message, const FString& Category, const FString& Type, const TMap<FString, FString>& Data, ESentryLevel Level) override;
virtual void ClearBreadcrumbs() override;
virtual USentryId* CaptureMessage(const FString& message, ESentryLevel level) override;
virtual USentryId* CaptureMessageWithScope(const FString& message, const FConfigureScopeNativeDelegate& onConfigureScope, ESentryLevel level) override;
Expand Down
12 changes: 12 additions & 0 deletions plugin-dev/Source/Sentry/Private/Apple/SentrySubsystemApple.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,18 @@ void SentrySubsystemApple::AddBreadcrumb(USentryBreadcrumb* breadcrumb)
[SENTRY_APPLE_CLASS(SentrySDK) addBreadcrumb:breadcrumbIOS->GetNativeObject()];
}

void SentrySubsystemApple::AddBreadcrumbWithParams(const FString& Message, const FString& Category, const FString& Type, const TMap<FString, FString>& Data, ESentryLevel Level)
{
TSharedPtr<SentryBreadcrumbApple> breadcrumbIOS = MakeShareable(new SentryBreadcrumbApple());
breadcrumbIOS->SetMessage(Message);
breadcrumbIOS->SetCategory(Category);
breadcrumbIOS->SetType(Type);
breadcrumbIOS->SetData(Data);
breadcrumbIOS->SetLevel(Level);

[SENTRY_APPLE_CLASS(SentrySDK) addBreadcrumb:breadcrumbIOS->GetNativeObject()];
}

void SentrySubsystemApple::ClearBreadcrumbs()
{
[SENTRY_APPLE_CLASS(SentrySDK) configureScope:^(SentryScope* scope) {
Expand Down
1 change: 1 addition & 0 deletions plugin-dev/Source/Sentry/Private/Apple/SentrySubsystemApple.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class SentrySubsystemApple : public ISentrySubsystem
virtual bool IsEnabled() override;
virtual ESentryCrashedLastRun IsCrashedLastRun() override;
virtual void AddBreadcrumb(USentryBreadcrumb* breadcrumb) override;
virtual void AddBreadcrumbWithParams(const FString& Message, const FString& Category, const FString& Type, const TMap<FString, FString>& Data, ESentryLevel Level) override;
virtual void ClearBreadcrumbs() override;
virtual USentryId* CaptureMessage(const FString& message, ESentryLevel level) override;
virtual USentryId* CaptureMessageWithScope(const FString& message, const FConfigureScopeNativeDelegate& onConfigureScope, ESentryLevel level) override;
Expand Down
23 changes: 22 additions & 1 deletion plugin-dev/Source/Sentry/Private/Desktop/SentryScopeDesktop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,26 @@ SentryScopeDesktop::SentryScopeDesktop()
{
}

SentryScopeDesktop::SentryScopeDesktop(const SentryScopeDesktop& Scope)
{
Dist = Scope.Dist;
Environment = Scope.Environment;
FingerprintDesktop = Scope.FingerprintDesktop;
TagsDesktop = Scope.TagsDesktop;
ExtraDesktop = Scope.ExtraDesktop;
ContextsDesktop = Scope.ContextsDesktop;
BreadcrumbsDesktop = Scope.BreadcrumbsDesktop;
LevelDesktop = Scope.LevelDesktop;
}

Comment on lines +20 to +31
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this coming from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's essential to implement a custom copy constructor for the scope class after we've introduced a critical section member.

Since FCriticalSection disables its default copy constructor using the default mechanism for scope isn't possible and results into compilation errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL. Thanks!

SentryScopeDesktop::~SentryScopeDesktop()
{
}

void SentryScopeDesktop::AddBreadcrumb(USentryBreadcrumb* breadcrumb)
{
FScopeLock Lock(&CriticalSection);
Copy link
Contributor

Choose a reason for hiding this comment

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

That's neat, that it works like that.


BreadcrumbsDesktop.Add(StaticCastSharedPtr<SentryBreadcrumbDesktop>(breadcrumb->GetNativeImpl()));
}

Expand Down Expand Up @@ -264,7 +278,7 @@ void SentryScopeDesktop::Apply(USentryEvent* event)
sentry_value_incref(nativeBreadcrumb);
sentry_value_append(eventBreadcrumbs, nativeBreadcrumb);
}

sentry_value_set_by_key(nativeEvent, "breadcrumbs", eventBreadcrumbs);
}
else
Expand All @@ -279,4 +293,11 @@ void SentryScopeDesktop::Apply(USentryEvent* event)
}
}

void SentryScopeDesktop::AddBreadcrumb(TSharedPtr<SentryBreadcrumbDesktop> breadcrumb)
{
FScopeLock Lock(&CriticalSection);

BreadcrumbsDesktop.Add(breadcrumb);
}

#endif
6 changes: 6 additions & 0 deletions plugin-dev/Source/Sentry/Private/Desktop/SentryScopeDesktop.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

#pragma once

#include "HAL/CriticalSection.h"

#include "Interface/SentryScopeInterface.h"

#if USE_SENTRY_NATIVE
Expand All @@ -16,6 +18,7 @@ class SentryScopeDesktop : public ISentryScope
{
public:
SentryScopeDesktop();
SentryScopeDesktop(const SentryScopeDesktop& Scope);
virtual ~SentryScopeDesktop() override;

virtual void AddBreadcrumb(USentryBreadcrumb* breadcrumb) override;
Expand Down Expand Up @@ -45,6 +48,7 @@ class SentryScopeDesktop : public ISentryScope
virtual void Clear() override;

void Apply(USentryEvent* event);
void AddBreadcrumb(TSharedPtr<SentryBreadcrumbDesktop> breadcrumb);

private:
FString Dist;
Expand All @@ -60,6 +64,8 @@ class SentryScopeDesktop : public ISentryScope
TArray<TSharedPtr<SentryBreadcrumbDesktop>> BreadcrumbsDesktop;

ESentryLevel LevelDesktop;

FCriticalSection CriticalSection;
};

#endif
12 changes: 12 additions & 0 deletions plugin-dev/Source/Sentry/Private/Desktop/SentrySubsystemDesktop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,18 @@ void SentrySubsystemDesktop::AddBreadcrumb(USentryBreadcrumb* breadcrumb)
GetCurrentScope()->AddBreadcrumb(breadcrumb);
}

void SentrySubsystemDesktop::AddBreadcrumbWithParams(const FString& Message, const FString& Category, const FString& Type, const TMap<FString, FString>& Data, ESentryLevel Level)
{
TSharedPtr<SentryBreadcrumbDesktop> breadcrumbDesktop = MakeShareable(new SentryBreadcrumbDesktop());
breadcrumbDesktop->SetMessage(Message);
breadcrumbDesktop->SetCategory(Category);
breadcrumbDesktop->SetType(Type);
breadcrumbDesktop->SetData(Data);
breadcrumbDesktop->SetLevel(Level);

GetCurrentScope()->AddBreadcrumb(breadcrumbDesktop);
}

void SentrySubsystemDesktop::ClearBreadcrumbs()
{
GetCurrentScope()->ClearBreadcrumbs();
Expand Down
1 change: 1 addition & 0 deletions plugin-dev/Source/Sentry/Private/Desktop/SentrySubsystemDesktop.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class SentrySubsystemDesktop : public ISentrySubsystem
virtual bool IsEnabled() override;
virtual ESentryCrashedLastRun IsCrashedLastRun() override;
virtual void AddBreadcrumb(USentryBreadcrumb* breadcrumb) override;
virtual void AddBreadcrumbWithParams(const FString& Message, const FString& Category, const FString& Type, const TMap<FString, FString>& Data, ESentryLevel Level) override;
virtual void ClearBreadcrumbs() override;
virtual USentryId* CaptureMessage(const FString& message, ESentryLevel level) override;
virtual USentryId* CaptureMessageWithScope(const FString& message, const FConfigureScopeNativeDelegate& onScopeConfigure, ESentryLevel level) override;
Expand Down
1 change: 1 addition & 0 deletions plugin-dev/Source/Sentry/Private/Interface/SentrySubsystemInterface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class ISentrySubsystem
virtual bool IsEnabled() = 0;
virtual ESentryCrashedLastRun IsCrashedLastRun() = 0;
virtual void AddBreadcrumb(USentryBreadcrumb* breadcrumb) = 0;
virtual void AddBreadcrumbWithParams(const FString& Message, const FString& Category, const FString& Type, const TMap<FString, FString>& Data, ESentryLevel Level) = 0;
virtual void ClearBreadcrumbs() = 0;
virtual USentryId* CaptureMessage(const FString& message, ESentryLevel level) = 0;
virtual USentryId* CaptureMessageWithScope(const FString& message, const FConfigureScopeNativeDelegate& onConfigureScope, ESentryLevel level) = 0;
Expand Down
12 changes: 4 additions & 8 deletions plugin-dev/Source/Sentry/Private/SentrySubsystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,14 +174,10 @@ void USentrySubsystem::AddBreadcrumb(USentryBreadcrumb* Breadcrumb)

void USentrySubsystem::AddBreadcrumbWithParams(const FString& Message, const FString& Category, const FString& Type, const TMap<FString, FString>& Data, ESentryLevel Level)
{
USentryBreadcrumb* Breadcrumb = NewObject<USentryBreadcrumb>();
Breadcrumb->SetMessage(Message);
Breadcrumb->SetCategory(Category);
Breadcrumb->SetType(Type);
Breadcrumb->SetData(Data);
Breadcrumb->SetLevel(Level);

AddBreadcrumb(Breadcrumb);
if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled())
return;

SubsystemNativeImpl->AddBreadcrumbWithParams(Message, Category, Type, Data, Level);
}

void USentrySubsystem::ClearBreadcrumbs()
Expand Down
Loading