-
-
Notifications
You must be signed in to change notification settings - Fork 451
Ensure that log attributes of Object type are stringified #4409
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: feat/logs-attributes-in-api
Are you sure you want to change the base?
Ensure that log attributes of Object type are stringified #4409
Conversation
if (value != null && this.type.equals("string")) { | ||
this.value = value.toString(); | ||
} else { | ||
this.value = value; | ||
} |
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.
Need to do this otherwise the object could get serialized as JSON and not as a String.
Example:
public class Point {
public int x;
public int y;
public Point(int x, int y) {
this.x = x;
this.y = y;
}
@Override
public String toString() {
return "Point(" + x + ", " + y + ")";
}
}
Send it as an attribute/format string param and it serializes as
"sentry.message.parameter.0":{"type":"string","value":{"x":10,"y":20}}}}]}
So with the JSON representation, which will not get ingested. We need to call toString
on it.
It will appear in the formatted message though as String.format
will call toString
, so that's another reason why I think it's correct to call toString
on it.
Performance metrics 🚀
|
a9bc5ac
to
1914d83
Compare
@@ -15,7 +15,11 @@ public final class SentryLogEventAttributeValue implements JsonUnknown, JsonSeri | |||
|
|||
public SentryLogEventAttributeValue(final @NotNull String type, final @Nullable Object value) { | |||
this.type = type; | |||
this.value = value; | |||
if (value != null && type.equals("string")) { |
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.
so we don't have a dedicated object
type? I think this looks a bit ugly from an API standpoint to specify a string type and then pass an object in 😬
I think our UI supports building trees based on the dot-convention, so how I'd image it looking in pseudocode:
// we get this as an input
"point" to SentryLogEventAttributeValue("object", Point(20,30))
// we now know it's an object type so we should dismember it via reflection
val parentKey = "point"
for (field in getDeclaredFields) {
// do this recursively for nested objects
serialize(parentKey + "." + field.name to SentryLogEventAttributeValue(field.type, field.value))
}
Do you think that would be feasible? The object
type is only on the SDK level but does not get passed to our backend. Kind of like our JsonObjectSerializer does via reflection.
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.
I was wondering how objects were actually serialized as JSON :)
So, for what it's worth this SentryLogEventAttributeValue
is not really meant to be used directly by users, I think it should be marked ApiStatus.Internal
.
We expect the user to call log(attributes, ...)
where attributes
is a Map<String, Object>
.
So we could have an object
type that we use internally, also I think we should really use an enum for the type
and not a string.
Anyway, I think yours is a cool idea, I just don't know if that's what a user would expect to see there, or if they would rather see value.toString()
as the attribute value. In the Java world I would say people would expect value.toString()
, what do you think?
Let me also check internally what we expect from an end product prespective because this could apply to many other platforms.
📜 Description
Ensures we stringify objects properly and adds attributes of other types to the de/serialization test.
#skip-changelog