[Logs 1] Add Sentry Log feature to Java SDK#4372
Conversation
|
lcian
left a comment
There was a problem hiding this comment.
Left some comments, otherwise LGTM as a first implementation
buenaflor
left a comment
There was a problem hiding this comment.
doing a drive by review since we also implement logs for Dart :)
| public final class SentryLogEvent implements JsonUnknown, JsonSerializable { | ||
|
|
||
| private @NotNull SentryId traceId; | ||
| private @NotNull Double timestamp; | ||
|
|
||
| private @NotNull String body; | ||
|
|
||
| private @Nullable SentryLevel level; | ||
| private @Nullable Map<String, SentryLogEventAttributeValue> attributes; | ||
| private @Nullable Map<String, Object> unknown; |
There was a problem hiding this comment.
I think the optional severity_number is missing which later on is inferred by the SDK if not set, e.g
There was a problem hiding this comment.
Do you mean for other SDKs using the Java SDK? That should land in unknown if Java SDK doesn't have it directly.
I can add it in a follow up PR.
There was a problem hiding this comment.
There is also the severity number field that is optional. If it is not set (by the user) then the SDK will use the default values like in the link above
See here
There was a problem hiding this comment.
Added in #4391 but it cannot be passed in via API yet.
lbloder
left a comment
There was a problem hiding this comment.
LGTM 👍
Left some comments to be addressed in later prs
Performance metrics 🚀
|
| - `options.getLogs().setEnabled(true)` in `Sentry.init` / `SentryAndroid.init` | ||
| - `logs.enabled=true` in `sentry.properties` | ||
| - `sentry.logs.enabled=true` in `application.properties` | ||
| - `sentry.logs.enabled: true` in `application.yml` |
There was a problem hiding this comment.
Ah yeah we should add it to manifest as well.
| public final class SentryLogEvent implements JsonUnknown, JsonSerializable { | ||
|
|
||
| private @NotNull SentryId traceId; | ||
| private @NotNull Double timestamp; |
| private @NotNull SentryId traceId; | ||
| private @NotNull Double timestamp; | ||
|
|
||
| private @NotNull String body; |
There was a problem hiding this comment.
haven't seen it in the docs, but maybe you know - do we need to do any truncation here? E.g. if there's a requirement that relay will drop logs with body longer than XXX or something
There was a problem hiding this comment.
The same limit that applies to error envelopes also applies here. Not truncation needed / wanted. We probably want to implement a weight based approach in the future to only send a lower batch size in some cases.
There was a problem hiding this comment.
And the limit applies to the whole batch, since it's a single envelope.
📜 Description
Add API for capturing logs.
💡 Motivation and Context
Partial implementation for #4350
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps