feat: Event hooks & configuration via GDScript #60

limbonaut · 2025-01-14T19:13:31Z

This PR introduces configuration via GDScript and adds the user-facing event hooks before_send and on_crash, as outlined in https://develop.sentry.dev/sdk/expected-features/#before-send-hook and https://docs.sentry.io/platforms/native/configuration/filtering/.

It introduces a new public API class called SentryConfiguration which users can extend in a script, and assign the script file in options to configure the SDK during initialization. However, due to the way scripting is initialized in the Godot Engine, this comes with a trade-off: a slightly later initialization. If a configuration script is assigned, initialization must be delayed until ScriptServer is ready to load and run the user script.

Resolves #47
Resolves #48

github-actions · 2025-01-14T19:13:55Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 8b41d9a

limbonaut · 2025-01-23T20:52:19Z

src/sentry_sdk.cpp

-#ifdef NATIVE_SDK
-		internal_sdk = std::make_shared<NativeSDK>();
-#else
-		// Unsupported platform
-		sentry::util::print_debug("This is an unsupported platform. Disabling Sentry SDK...");
-		enabled = false;
-#endif
-	}
-
-	if (!enabled) {
-		sentry::util::print_debug("Sentry SDK is DISABLED! Operations with Sentry SDK will result in no-ops.");
-		internal_sdk = std::make_shared<DisabledSDK>();
-		return;


FYI this code moved to _intialize()

bitsandfoxes

This is amazing stuff. The changelog reads as if the SDK initializes at different points depending on whether the configure callback has been implemented. I don't quite follow the chain of notifications to get from init to configure back to actual init with the actual options. Would unifying the initialization to just the one right after the script server runs simplify this?

bitsandfoxes · 2025-01-28T12:17:15Z

project/example_configuration.gd

+func _configure(options: SentryOptions) -> void:
+	print("[example_configuration.gd] Configuring SDK options via GDScript")
+	options.debug = true
+


We could extend the sample here and provide some more options. I.e. setting the release and/or the environment. To give an idea what's there.

I like the idea, although, we can't change environment until #66 is merged. Will keep it in mind.
I've updated the example configuration with setting release attribute in 46c122e

bitsandfoxes · 2025-01-28T12:18:33Z

project/example_configuration.gd

+
+## before_send callback example
+func _before_send(ev: SentryEvent) -> SentryEvent:
+	print("[example_configuration.gd] Processing event: ", ev.id)


One of the more important uses of this callback is to allow users filtering of events. I.e. return null for dropping events entirely. Based on the native implementation it looks like this should be supported. Can we update the sample with some filtering?

We can, although, this would apply to everything in the demo project, including our unit tests, so it should be something harmless.

Updated the example with filtering in 9a49f11

bitsandfoxes · 2025-01-28T12:21:27Z

src/sentry/native/native_event.cpp

+	if (sentry_value_refcount(p_native_event) > 0) {
+		sentry_value_incref(p_native_event); // acquire ownership
+		native_event = p_native_event;
+	} else {
+		// Shouldn't happen in healthy code.
+		native_event = sentry_value_new_event();
+		ERR_PRINT("Sentry: Internal error: Event refcount is zero.");
+	}


Where is this change coming from?

I adjusted how ownership is handled in the NativeEvent class, just so we don't have to preclude constructor call with incref every time.

bitsandfoxes · 2025-01-28T12:30:35Z

src/sentry_sdk.cpp

+	}
+
+	if (!enabled) {
+		sentry::util::print_debug("Sentry SDK is DISABLED! Operations with Sentry SDK will result in no-ops.");


I'm not quite following the whole initialization. Is logging set up prior to initialization? Does this show up 100% of the time with the SDK disabled? The way to get rid of this would be to increase the logging verbosity I guess?

If you are commenting on this specific if-branch, then this message would appear in stdout if options.debug is ON and SDK is disabled either manually via options or because it is unsupported platform.

I'm not sure what you mean by logging here. Godot doesn't employ the logger concept, with verbosity levels and such. There is print, push_error, and push_warning in GDScript. But we're not adding this message to such channels, and similar to sentry_native, it is simply printed to stdout (for consistency’s sake). Our error logger will eventually process errors and warnings, but will not deal with prints.

limbonaut · 2025-01-28T18:25:33Z

Thanks for reviewing @bitsandfoxes. It's a best-effort solution among very few options. Let me explain.

The changelog reads as if the SDK initializes at different points depending on whether the configure callback has been implemented.

Correct, more on that below.

Would unifying the initialization to just the one right after the script server runs simplify this?

We don't have an initialization opportunity at that point. Here are our options:

Before the ScriptServer is initialized -- no user configuration is possible. Several opportunities to hook up here. We use this method if the user configuration script is not supplied.
Via autoload singleton, for which we need a user script. If we don't have a script - we can't use this option. This is our current approach if the user provides a script. Otherwise, we default to early initialization, and potentially very early initialization when Initialize SDK as early as possible #39 gets implemented. This step happens shortly after ScriptServer and the scene tree are initialized, but before the main scene is loaded and added to the tree.
Initialize late, during the main loop's first iteration - that's kinda late for us. Hence, I went with 1 and 2 combined.

These are the options I could find to tap into initialization, and I don't see any other way aside from introducing upstream changes.

I don't quite follow the chain of notifications to get from init to configure back to actual init with the actual options.

We're registering an autoload singleton (aka user script that extends our SentryConfiguration class) in the project settings. Only a script can be registered as an autoload singleton. This step takes place in the SentrySDK constructor.
Autoload singleton script is loaded and added to the scene tree by the engine.
The SentryConfiguration instance (user script must extend this class) receives NOTIFICATION_READY when added to the scene tree. In C++, our notification processing code in SentryConfiguration calls _configure() on the user script and then calls notify_options_configured() in SentrySDK.
_notify_options_configured() finally initializes the internal SDK.
We also have a safety catch (implemented via option 3), just in case the user script fails for whatever reason, to notify the user about the script problem and initialize anyway at that point.

UPDATE:
Added explanation in 8b41d9a

Add SentryOptions.environment property. Environment is auto-detected for user convenience, and it will be possible to assign it after #60 is merged. Also, change auto-detected environment names to prioritize development environments. Resolves GH-64

limbonaut added 2 commits January 13, 2025 23:29

Try-implement event hooks

549a602

Add integration test

af51cbc

limbonaut added the enhancement New feature or request label Jan 14, 2025

limbonaut added 13 commits January 14, 2025 20:26

Fix compilation error on macOS

61efe59

Add on_crash hook

a6272a5

FIx configuration script

84d21bc

Clarify comments

e9f6355

Merge branch 'main' into feat/event-hooks

df663f0

Add tip

9812184

Pass options as parameter in _initialize()

a6e587d

Store callbacks in options

3685467

Hide SentrySDK.set_{before_send,on_crash}

54f6c44

Restructure initialization

2aaabd8

Note about user configuration

64637c2

Rework SentryConfiguration as autoload node

f091945

Rename _initialize => _configure

ff81efb

limbonaut changed the title ~~feat: Event hooks~~ feat: Event hooks & configuration via GDScript Jan 22, 2025

limbonaut added 5 commits January 22, 2025 17:33

Initialize only if enabled & don't initialize in editor with user script

287ff3b

Check if configuration succeeds, dont start logger unless it does

d9ee288

Improve native event ownership

50cca83

Clean up

cef3266

Update CHANGELOG

a5267cc

limbonaut commented Jan 23, 2025

View reviewed changes

limbonaut marked this pull request as ready for review January 23, 2025 20:58

limbonaut requested review from bitsandfoxes and bruno-garcia January 23, 2025 20:58

limbonaut mentioned this pull request Jan 23, 2025

feat: Add environment property to SentryOptions #66

Merged

bitsandfoxes reviewed Jan 28, 2025

View reviewed changes

Expand configuration example

46c122e

Update example: discard event with message "junk"

9a49f11

Explain delayed initialization in comments

8b41d9a

limbonaut force-pushed the feat/event-hooks branch from 9b8e805 to 8b41d9a Compare January 28, 2025 18:57

bitsandfoxes approved these changes Jan 29, 2025

View reviewed changes

bitsandfoxes merged commit 623b874 into main Jan 29, 2025
16 checks passed

bitsandfoxes deleted the feat/event-hooks branch January 29, 2025 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Event hooks & configuration via GDScript #60

feat: Event hooks & configuration via GDScript #60

limbonaut commented Jan 14, 2025 •

edited

Loading

github-actions bot commented Jan 14, 2025 •

edited

Loading

limbonaut Jan 23, 2025

bitsandfoxes left a comment

bitsandfoxes Jan 28, 2025

limbonaut Jan 28, 2025

bitsandfoxes Jan 28, 2025

limbonaut Jan 28, 2025

limbonaut Jan 28, 2025

bitsandfoxes Jan 28, 2025

limbonaut Jan 28, 2025

bitsandfoxes Jan 28, 2025

limbonaut Jan 28, 2025 •

edited

Loading

limbonaut commented Jan 28, 2025 •

edited

Loading

feat: Event hooks & configuration via GDScript #60

feat: Event hooks & configuration via GDScript #60

Conversation

limbonaut commented Jan 14, 2025 • edited Loading

github-actions bot commented Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

bitsandfoxes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

limbonaut Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

limbonaut commented Jan 28, 2025 • edited Loading

limbonaut commented Jan 14, 2025 •

edited

Loading

github-actions bot commented Jan 14, 2025 •

edited

Loading

limbonaut Jan 28, 2025 •

edited

Loading

limbonaut commented Jan 28, 2025 •

edited

Loading