-
-
Notifications
You must be signed in to change notification settings - Fork 199
feat(logs): custom attributes API #1435
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
base: master
Are you sure you want to change the base?
Changes from 13 commits
573a683
95494e7
fe2416a
44089a3
ad53186
1415f3e
15bb9bb
46cd9f1
878a488
33a4471
c753b66
1ea83a8
5408b75
ad7b2fd
e54fbe3
e6f2655
660cc77
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -546,15 +546,14 @@ static | |
| * This function assumes that `value` is owned, so we have to make sure that the | ||
| * `value` was created or cloned by the caller or even better inc_refed. | ||
| * | ||
| * Replaces attributes[name] if it already exists. | ||
| * No-op if 'name' already exists in the attributes. | ||
| */ | ||
| static void | ||
| add_attribute(sentry_value_t attributes, sentry_value_t value, const char *type, | ||
| const char *name) | ||
| { | ||
| if (!sentry_value_is_null(sentry_value_get_by_key(attributes, name))) { | ||
| // already exists, so we remove and create a new one | ||
| sentry_value_remove_by_key(attributes, name); | ||
| return; | ||
| } | ||
| sentry_value_t param_obj = sentry_value_new_object(); | ||
| sentry_value_set_by_key(param_obj, "value", value); | ||
|
|
@@ -648,10 +647,6 @@ add_scope_and_options_data(sentry_value_t log, sentry_value_t attributes) | |
| } | ||
| } | ||
|
|
||
| // fallback in case options doesn't set it | ||
| add_attribute(attributes, sentry_value_new_string(SENTRY_SDK_NAME), | ||
| "string", "sentry.sdk.name"); | ||
|
|
||
| SENTRY_WITH_OPTIONS (options) { | ||
| if (options->environment) { | ||
| add_attribute(attributes, | ||
|
|
@@ -677,25 +672,66 @@ construct_log(sentry_level_t level, const char *message, va_list args) | |
| sentry_value_t log = sentry_value_new_object(); | ||
| sentry_value_t attributes = sentry_value_new_object(); | ||
|
|
||
| va_list args_copy_1, args_copy_2, args_copy_3; | ||
| va_copy(args_copy_1, args); | ||
| va_copy(args_copy_2, args); | ||
| va_copy(args_copy_3, args); | ||
| int len = vsnprintf(NULL, 0, message, args_copy_1) + 1; | ||
| va_end(args_copy_1); | ||
| size_t size = (size_t)len; | ||
| char *fmt_message = sentry_malloc(size); | ||
| if (!fmt_message) { | ||
| SENTRY_WITH_OPTIONS (options) { | ||
| // Extract custom attributes if the option is enabled | ||
| if (sentry_options_get_logs_with_attributes(options)) { | ||
| va_list args_copy; | ||
| va_copy(args_copy, args); | ||
|
Comment on lines
673
to
+680
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The removal of the fallback 🤖 Prompt for AI Agent Did we get this right? 👍 / 👎 to inform future reviews. |
||
| sentry_value_t custom_attributes | ||
| = va_arg(args_copy, sentry_value_t); | ||
| va_end(args_copy); | ||
| if (sentry_value_get_type(custom_attributes) | ||
| == SENTRY_VALUE_TYPE_OBJECT) { | ||
| // TODO do we want to inspect that the object is attribute-like? | ||
| sentry_value_decref(attributes); | ||
| attributes = sentry__value_clone(custom_attributes); | ||
| } else { | ||
|
Comment on lines
684
to
688
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure how far we need to go to protect from user error. An invalid attribute (no "type" and "value" keys) seems to just get ignored in processing (the log still makes it to the UI) |
||
| SENTRY_DEBUG("Discarded custom attributes on log: non-object " | ||
| "sentry_value_t passed in"); | ||
| } | ||
| sentry_value_decref(custom_attributes); | ||
| } | ||
|
|
||
| // Format the message with remaining args (or all args if not using | ||
| // custom attributes) | ||
| va_list args_copy_1, args_copy_2, args_copy_3; | ||
| va_copy(args_copy_1, args); | ||
| va_copy(args_copy_2, args); | ||
| va_copy(args_copy_3, args); | ||
|
|
||
| // Skip the first argument (attributes) if using custom attributes | ||
| if (sentry_options_get_logs_with_attributes(options)) { | ||
| va_arg(args_copy_1, sentry_value_t); | ||
| va_arg(args_copy_2, sentry_value_t); | ||
| va_arg(args_copy_3, sentry_value_t); | ||
| } | ||
|
|
||
| int len = vsnprintf(NULL, 0, message, args_copy_1) + 1; | ||
| va_end(args_copy_1); | ||
| size_t size = (size_t)len; | ||
| char *fmt_message = sentry_malloc(size); | ||
| if (!fmt_message) { | ||
| va_end(args_copy_2); | ||
| va_end(args_copy_3); | ||
| return sentry_value_new_null(); | ||
| } | ||
|
|
||
| vsnprintf(fmt_message, size, message, args_copy_2); | ||
| va_end(args_copy_2); | ||
|
|
||
| sentry_value_set_by_key( | ||
| log, "body", sentry_value_new_string(fmt_message)); | ||
| sentry_free(fmt_message); | ||
|
|
||
| // Parse variadic arguments and add them to attributes | ||
| if (populate_message_parameters(attributes, message, args_copy_3)) { | ||
| // only add message template if we have parameters | ||
| add_attribute(attributes, sentry_value_new_string(message), | ||
| "string", "sentry.message.template"); | ||
| } | ||
| va_end(args_copy_3); | ||
| return sentry_value_new_null(); | ||
| } | ||
|
|
||
| vsnprintf(fmt_message, size, message, args_copy_2); | ||
| va_end(args_copy_2); | ||
|
|
||
| sentry_value_set_by_key(log, "body", sentry_value_new_string(fmt_message)); | ||
| sentry_free(fmt_message); | ||
| sentry_value_set_by_key( | ||
| log, "level", sentry_value_new_string(level_as_string(level))); | ||
|
|
||
|
|
@@ -708,14 +744,6 @@ construct_log(sentry_level_t level, const char *message, va_list args) | |
| // to the log | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The entire message formatting and parameter parsing logic is now wrapped inside 🤖 Prompt for AI Agent Did we get this right? 👍 / 👎 to inform future reviews.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no exclusive lock over the options, since we only increment the refcount (and the options themselves are read-only). So there is no real performance impact here. |
||
| add_scope_and_options_data(log, attributes); | ||
|
|
||
| // Parse variadic arguments and add them to attributes | ||
| if (populate_message_parameters(attributes, message, args_copy_3)) { | ||
| // only add message template if we have parameters | ||
| add_attribute(attributes, sentry_value_new_string(message), "string", | ||
| "sentry.message.template"); | ||
| } | ||
| va_end(args_copy_3); | ||
|
|
||
| sentry_value_set_by_key(log, "attributes", attributes); | ||
|
|
||
| return log; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we can overwrite all default attributes except for string format values that are passed in; otherwise there would be a mismatch between the string template +
sentry.message.parameter.Xand the logbodyvalueThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using both the attributes to pass
message.parameter.0and the sentry-native interpolation API is already a misuse. I don't think we need to be protective about it. Let it be an undefined behavior.