feat: Event objects #51

limbonaut · 2024-12-23T22:18:16Z

This PR implements:

SentryEvent objects, which can be used to create or modify an event, and a native implementation.
- NativeEvent class -- implementation used with NativeSDK
Add SentrySDK.capture_event(event), SentrySDK.create_event(), SentrySDK.create_message_event() methods.
Unit tests for the SentryEvent class and SDK methods.

In future PRs, we can implement accessing tags, breadcrumbs, contexts, fingerprint, etc.

Dependency for:

So we don't return a null object when SDK is disabled

limbonaut · 2025-01-03T19:01:17Z

src/sentry_sdk.h

 	String get_last_event_id() const;

+	Ref<SentryEvent> create_event() const;
+	Ref<SentryEvent> create_message_event(const String &p_message, sentry::Level p_level = sentry::LEVEL_INFO, const String &p_logger = "");


Do we need to expose logger? It's unclear what it's used for.

I don't see the use for logger either. Logging itself is setup during initialization, right?

Our error logger is set up at initialization and has nothing to do with this. I'm not even sure what this field is used for. This logger bit is a part of event payload:

https://develop.sentry.dev/sdk/data-model/event-payloads/

Maybe we just remove this part from function call, but leave it as property of SenteryEvent?

It's a part of sentry-native call, so I left it as is:
https://github.com/getsentry/sentry-native/blob/3981ea7e7fd012524e4c04501c9dd647563aa391/include/sentry.h#L377-L380
We also have it in capture_message signature.

github-actions · 2025-01-08T19:12:31Z

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

Generated by 🚫 dangerJS against 8e3ac2c

bitsandfoxes

This looks good to me!
I'm just questioning the usecase of create_message_event.

bitsandfoxes · 2025-01-09T12:16:22Z

src/sentry/disabled_event.h

+
+#include "sentry_event.h"
+
+// Event class that is used when the SDK is disabled.


Where does the need for a DisabledEvent come from? Can't we use the SentryEvent and still have the SDK disabled?

SentryEvent is abstract and cannot be instantiated, and it's supposed to be implemented by NativeEvent class and such (for each internal SDK used). The reason behind DisabledEvent is to return a non-null value, so that it doesn't break scripts which may not be checking if SentrySDK is enabled. I could put this logic into SentryEvent, but it would add some bloated state to each event. I.e. SentryEvent is basically an interface.

bitsandfoxes · 2025-01-09T12:24:02Z

src/sentry/native/native_event.cpp

+
+namespace {
+
+inline void _sentry_value_set_or_remove_string_by_key(sentry_value_t value, const char *k, const String &v) {


So we end up with shortcuts and instead of event.remove_release() we want users to use event.set_release("")?

Not sure if I understand the question. This is a helper method. It sets a string value on a sentry_value object, unless the string is empty, in which case it removes such key. Hence, the name of the method.
So we don't store such values:
os.name = ""
Instead, we remove the os.name key-value from the object.

Oh, I think I see what you mean. We have a pair of setter/getter for each property of SentryEvent.
event.release is how we access it in GDScript. event.release = "some_release" is how we set it. So in order to remove the value, we would be doing event.release = ""

@bitsandfoxes Do you think we need explicit methods for removing event properties?

bitsandfoxes · 2025-01-09T12:27:08Z

src/sentry/native/native_sdk.cpp

+Ref<SentryEvent> NativeSDK::create_message_event(const String &p_message, sentry::Level p_level, const String &p_logger) {
+	sentry_value_t event_value = sentry_value_new_message_event(
+			native::level_to_native(p_level),
+			p_logger.utf8().get_data(),
+			p_message.utf8().get_data());
+	Ref<SentryEvent> event = memnew(NativeEvent(event_value));
+	return event;
+}


I'm wondering, what's the usecase for this over capture_message?

Technically, this is not necessary, as you can add an event, set its message and level properties, and whichever other properties you need. So this can be removed - it's a shortcut for creating customized message events. I think I took it from some other SDKs, I'm not sure.

Removed in 8e3ac2c

bitsandfoxes · 2025-01-09T13:14:48Z

src/sentry_sdk.h

 	String get_last_event_id() const;

+	Ref<SentryEvent> create_event() const;
+	Ref<SentryEvent> create_message_event(const String &p_message, sentry::Level p_level = sentry::LEVEL_INFO, const String &p_logger = "");


I don't see the use for logger either. Logging itself is setup during initialization, right?

limbonaut added 7 commits December 23, 2024 21:37

Add SentryEvent base class & native implemention

85bc598

Implement SentrySDK.capture_event(event)

94539dd

Add test for capture_event()

a74b9fc

Add SentryEvent.message property

966850d

Refactor code

b1cd501

Add unit tests for SentryEvent

9893d33

Fix issues discovered in testing

cab54aa

limbonaut added the enhancement New feature or request label Dec 23, 2024

limbonaut mentioned this pull request Dec 23, 2024

User-facing event hooks #47

Open

limbonaut added 14 commits December 23, 2024 23:34

Add doc comments for tests

4619a1b

Harden the code

8b6c784

Add DisabledEvent class

14f392a

So we don't return a null object when SDK is disabled

Fix compilation error

12beaef

Revise native event ownership

dae8091

Add SentryEvent.platform property

3adcdd9

Add SentryEvent.logger property

3dd697b

Add SentryEvent.release property

5a48235

Add SentryEvent.dist property

6e6a16d

Add SentryEvent.environment property

17bd246

Fix accessing message

718663f

Fix native event id

4f72880

Add SentrySDK.create_message_event()

0f83a6a

Missing level

8055c6d

limbonaut commented Jan 3, 2025

View reviewed changes

Add comment

a7a296a

limbonaut marked this pull request as ready for review January 3, 2025 19:22

limbonaut requested review from bruno-garcia and bitsandfoxes January 3, 2025 19:22

Merge remote-tracking branch 'upstream/main' into feat/event-objects

ebba28a

Update changelog

ec07866

bitsandfoxes reviewed Jan 9, 2025

View reviewed changes

limbonaut force-pushed the feat/event-objects branch from 988c53a to 8e3ac2c Compare January 10, 2025 14:58

Remove create_message_event method

8e3ac2c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Event objects #51

feat: Event objects #51

limbonaut commented Dec 23, 2024 •

edited

Loading

limbonaut Jan 3, 2025

bitsandfoxes Jan 9, 2025

limbonaut Jan 10, 2025 •

edited

Loading

limbonaut Jan 10, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited

Loading

bitsandfoxes left a comment

bitsandfoxes Jan 9, 2025

limbonaut Jan 10, 2025

bitsandfoxes Jan 9, 2025

limbonaut Jan 10, 2025

limbonaut Jan 10, 2025

limbonaut Jan 10, 2025 •

edited

Loading

bitsandfoxes Jan 9, 2025

limbonaut Jan 10, 2025

limbonaut Jan 10, 2025

bitsandfoxes Jan 9, 2025


		#include "sentry_event.h"

		// Event class that is used when the SDK is disabled.


		namespace {

		inline void _sentry_value_set_or_remove_string_by_key(sentry_value_t value, const char *k, const String &v) {

feat: Event objects #51

Are you sure you want to change the base?

feat: Event objects #51

Conversation

limbonaut commented Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

limbonaut Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

limbonaut Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

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

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

limbonaut Jan 10, 2025 • edited Loading

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 commented Dec 23, 2024 •

edited

Loading

limbonaut Jan 10, 2025 •

edited

Loading

limbonaut Jan 10, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited

Loading

limbonaut Jan 10, 2025 •

edited

Loading