-
-
Notifications
You must be signed in to change notification settings - Fork 455
Send Logback logs to Sentry as logs #4502
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
Conversation
There's currently a problem, where we don't flush logs before the application shuts down. Will open a separate PR to fix. |
|
Performance metrics 🚀
|
final Hint hint = new Hint(); | ||
hint.set(SENTRY_SYNTHETIC_EXCEPTION, eventObject); |
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.
final Hint hint = new Hint(); | |
hint.set(SENTRY_SYNTHETIC_EXCEPTION, eventObject); |
I think the hint is not needed here, right?
// for the Android compatibility we must use old Java Date class | ||
@SuppressWarnings("JdkObsolete") | ||
protected void captureLog(@NotNull ILoggingEvent loggingEvent) { | ||
final @NotNull SentryLogLevel sentryLevel = toSentryLogLevel(loggingEvent.getLevel()); |
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.
let's also check if logs is enabled here, so we don't format the string if not necessary?
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 it's ok
In mobile we want to wait for priority queue for logs integrations to avoid log envelopes throwing away errors, especially when the app is offline, but also in case an app logs really a lot.
Not sure how much of it is applicable to backend, though
Just added code to treat unformatted message + params as PII if encoder is present. |
if (attributes.get("sentry.message.template") == null) { | ||
attributes.put( | ||
"sentry.message.template", | ||
new SentryLogEventAttributeValue(SentryAttributeType.STRING, message)); | ||
} |
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.
l
: I don't think we should set this. If the message is already formatted, then this will just duplicate it
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.
In case of logging integrations, we can just set the sentry.message.template
attribute in the integration and the newly added if
prevents replacing that attribute.
The duplicate content should compress nicely.
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
📜 Description
Allow setting a
minimumLevel
to our Logback integration which causes logs >= that level to be sent to Sentry as logs.You also have to enable the logs feature:
Either in
logback.xml
:Or in
sentry.properties
:Or when calling
Sentry.init
:Note: This is different from breadcrumbs (
minimumBreadcrumbLevel
) and error events (minimumEventLevel
) we already had since Sentry Logs is a separate product feature. Breadcrumbs and error events still work.💡 Motivation and Context
Closes #4404
💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps