-
-
Notifications
You must be signed in to change notification settings - Fork 451
Add Sentry Log feature to Java SDK #4372
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: main
Are you sure you want to change the base?
Conversation
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
- Add Sentry Log feature to Java SDK ([#4372](https://github.com/getsentry/sentry-java/pull/4372)) If none of the above apply, you can opt out of this check by adding |
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.
Left some comments, otherwise LGTM as a first implementation
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Left some comments to be addressed in later prs
final @NotNull SpanId spanId = | ||
span == null ? propagationContext.getSpanId() : span.getSpanContext().getSpanId(); | ||
final SentryLogEvent logEvent = new SentryLogEvent(traceId, timestampToUse, messageToUse); | ||
logEvent.setLevel(level); |
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.
Also update baggage here
"sentry.message.template", new SentryLogEventAttributeValue("string", message)); | ||
int i = 0; | ||
for (Object arg : args) { | ||
final @NotNull String type = getType(arg); |
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.
arg could be null
public void serialize(final @NotNull ObjectWriter writer, final @NotNull ILogger logger) | ||
throws IOException { | ||
writer.beginObject(); | ||
writer.name("items").value(logger, items); |
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.
use JsonKeys.ITEMS
here
📜 Description
Add API for capturing logs.
💡 Motivation and Context
Partial implementation for #4350
💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps