Improve Log Attributes API #4416
Conversation
adinauer
requested review from
romtsn,
stefanosiano,
markushi and
lcian
as code owners
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
- Improve Log Attributes API ([#4416](https://github.com/getsentry/sentry-java/pull/4416))If none of the above apply, you can opt out of this check by adding |
Performance metrics 🚀
|
| return new SentryAttribute(name, null, value); | ||
| } | ||
|
|
||
| public static @NotNull SentryAttribute booleanAttribute( |
There was a problem hiding this comment.
L: how about boolean, integer, etc. instead?
| @Nullable Map<String, Object> attributes, | ||
| @NotNull SentryLogLevel level, | ||
| @Nullable SentryDate timestamp, | ||
| @NotNull LogParams params, |
There was a problem hiding this comment.
L: should it be named SentryLogParams or SentryLogParameters maybe?
| public static @NotNull LogParams create( | ||
| final @Nullable SentryDate timestamp, final @Nullable SentryAttributes attributes) { | ||
| final @NotNull LogParams params = new LogParams(); | ||
|
|
||
| params.setTimestamp(timestamp); | ||
| params.setAttributes(attributes); | ||
|
|
||
| return params; | ||
| } |
There was a problem hiding this comment.
why is this not directly the constructor?
| public static @NotNull SentryAttribute named( | ||
| final @NotNull String name, final @Nullable Object value) { | ||
| return new SentryAttribute(name, null, value); | ||
| } |
There was a problem hiding this comment.
We call this in sentry/src/main/java/io/sentry/SentryAttributes.java when building attributes from a Map.
As we take in a generic Map, the key for an entry could be null.
We should check for nulls either here (making the name param nullable and e.g. not constructing the attribute if it is) or in sentry/src/main/java/io/sentry/SentryAttributes.java when iterating the entry set.
| } | ||
|
|
||
| public static @NotNull SentryAttributes of(SentryAttribute... attributes) { | ||
| return new SentryAttributes(Arrays.asList(attributes)); |
There was a problem hiding this comment.
Where we use this class, we rely on this.attributes not containing nulls, however one or more elements in the list could be null and we would include them in the list, risking a NPE down the line.
| for (Map.Entry<String, Object> attribute : attributes.entrySet()) { | ||
| sentryAttributes.add(SentryAttribute.named(attribute.getKey(), attribute.getValue())); | ||
| } |
There was a problem hiding this comment.
See above, we could filter for nulls here.
| for (SentryAttribute attribute : incomingAttributes.getAttributes()) { | ||
| final @Nullable Object value = attribute.getValue(); | ||
| final @NotNull SentryAttributeType type = | ||
| attribute.getType() == null ? getType(value) : attribute.getType(); | ||
| attributes.put(attribute.getName(), new SentryLogEventAttributeValue(type, value)); |
There was a problem hiding this comment.
Another case where we rely on the individual attribute being not null.
|
|
||
| public final class SentryAttributes { | ||
|
|
||
| private final @NotNull List<SentryAttribute> attributes; |
There was a problem hiding this comment.
should we change this to Map<String, SentryAttribute>?
Now it's possible to add multiple times the same attribute, but only the last one will actually be sent
📜 Description
Improve Log Attributes API:
Sentry.logger().log()overload that takes aLogParamsparam objectSentryAttributesandSentryAttributeSentryAttributeTypeenum💡 Motivation and Context
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps