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

Intermediary Error logger implementation #9

Merged
merged 23 commits into from
Nov 4, 2024
Merged

Intermediary Error logger implementation #9

merged 23 commits into from
Nov 4, 2024

Conversation

limbonaut
Copy link
Collaborator

@limbonaut limbonaut commented Oct 30, 2024

While we are waiting for the Godot PRs (see #4), this is a logger implementation based on log file parsing. For practical usage, my performance measurements showed it as quite usable, and it's completely optional for game developers. It should be trivial to adapt it when the user-facing logger class is exposed in the Godot public API.

The current limitation is that we don't have access to a script stack trace, but we still have the line number, the function and the file in which the error has occurred (almost equivalent to the top frame of the stack trace).

Error logger section in Project Settings

image

Error captured as event

image
Note: the source code is automatically included with the event if the corresponding option is enabled. It is not available if project scripts are exported as binary tokens.

@limbonaut limbonaut added the enhancement New feature or request label Oct 30, 2024
@limbonaut limbonaut linked an issue Oct 30, 2024 that may be closed by this pull request
@limbonaut limbonaut marked this pull request as ready for review October 30, 2024 15:18
@limbonaut
Copy link
Collaborator Author

I've spun off a branch from this PR's - going to wait for the review/merge, and later rebase onto main.
I know it's risky, but it's either wait or have even bigger rebase later on.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

you might want a review from @bitsandfoxes and/or @PlasmaDev5 before merging but I skimmed and lgtm.

Just still missing tests but I imagine this is coming then as thigns shape up a bit more

project/project.godot Show resolved Hide resolved
src/sentry_util.h Outdated Show resolved Hide resolved
src/sentry_util.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@PlasmaDev5 PlasmaDev5 left a comment

Choose a reason for hiding this comment

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

Everything looks good to me.

@limbonaut limbonaut merged commit 416550e into main Nov 4, 2024
3 checks passed
@limbonaut limbonaut deleted the logger branch November 4, 2024 19:02
Copy link

@bitsandfoxes bitsandfoxes left a comment

Choose a reason for hiding this comment

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

This looks amazing! One thing just does not fully make sense to me:

The way I see this feature is that we're creating breadcrumbs out of logs and that errors that got logged to the logfile are automatically captured as events. After they have been captured they should get added as breadcrumbs as well.

I mean we can iterate on it for sure so this is not a blocker but I think the functionality is a tiny bit off.

@@ -26,3 +30,4 @@ window/stretch/aspect="keep_height"

config/dsn="https://3f1e095cf2e14598a0bd5b4ff324f712@o447951.ingest.us.sentry.io/6680910"
config/debug=true
config/error_logger/capture_type=1

Choose a reason for hiding this comment

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

I take it this is the option that controls this feature? Why is it a type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, it shouldn't be mutually exclusive.

@@ -8,6 +8,12 @@
#include <godot_cpp/variant/variant.hpp>

class SentryOptions {
public:
enum CaptureType {

Choose a reason for hiding this comment

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

I'm not sure I understand the split between capture as breadcrumb and capture as event. Those are not mutually exclusive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense! Thanks for reviewing. I added an issue to track it: #10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants