Skip to content
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

Add support for using Encoder with logback.SentryAppender #2246

Merged
merged 7 commits into from Oct 17, 2022

Conversation

norwood
Copy link
Contributor

@norwood norwood commented Sep 16, 2022

📜 Description

this adds support for logback encoders in the sentry appender

💡 Motivation and Context

this was previously added in https://github.com/getsentry/sentry-java/pull/794/files and was mentioned here #2217

💚 How did you test it?

added a test to the test suite
tested against my own sentry instance that formatted messages go through

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution @norwood 🙏 and sorry for taking so long - we've been pretty busy. Left one question regarding try/catch otherwise this LGTM.

@@ -137,6 +140,13 @@ protected void append(@NotNull ILoggingEvent eventObject) {
return event;
}

private String formatted(@NotNull ILoggingEvent loggingEvent) {
if (encoder != null) {
return new String(encoder.encode(loggingEvent), StandardCharsets.UTF_8);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to wrap this with a try/catch just in case a faulty encoder has been configured so we still get the formatted message as fallback?

@norwood norwood force-pushed the add-encoder-to-sentry-logback branch from 3d3b04c to 44d2d92 Compare October 11, 2022 15:45
@norwood norwood requested review from adinauer and removed request for romtsn and stefanosiano October 11, 2022 15:46
@adinauer adinauer changed the title feat(sentry-logback): add encoder for message formatting Add support for using Encoder with logback.SentryAppender Oct 12, 2022
@codecov-commenter
Copy link

Codecov Report

Base: 80.10% // Head: 80.11% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (fff5493) compared to base (4796772).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2246      +/-   ##
============================================
+ Coverage     80.10%   80.11%   +0.01%     
- Complexity     3430     3433       +3     
============================================
  Files           242      242              
  Lines         12758    12765       +7     
  Branches       1703     1703              
============================================
+ Hits          10220    10227       +7     
  Misses         1889     1889              
  Partials        649      649              
Impacted Files Coverage Δ
...rc/main/java/io/sentry/logback/SentryAppender.java 89.10% <100.00%> (+0.81%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jeacott1
Copy link

Is there still a means to add global MDC tags?

@adinauer
Copy link
Member

adinauer commented Nov 11, 2022

@jeacott1 you can set a list of MDC tags that Sentry will pick up and convert to tags as shown in this sample:

<contextTag>userId</contextTag>
<contextTag>requestId</contextTag>

I looks like we never documented this feature, created #2351 to do so.

Is the feature not working for you?

@jeacott1
Copy link

@adinauer the option used to exist due to my lost commit https://github.com/getsentry/sentry-java/pull/794/files
I didn't know there was now an alternative.
thanks!

@transducer
Copy link

transducer commented Jun 21, 2023

@norwood how can this encoder be configured? If I add an encoder to my logback.xml, in Sentry I see an <unlabeled event>.

@norwood
Copy link
Contributor Author

norwood commented Jun 21, 2023

i have only configured manually, not with logback.xml, and i'm not familiar with the xml configuration path.

@transducer
Copy link

@norwood thanks for reply. It looks like this:

<appender name="SENTRY" class="io.sentry.logback.SentryAppender">
  <encoder class="ch.qos.logback.core.encoder.LayoutWrappingEncoder">
    <layout class="com.baeldung.logback.MaskingPatternLayout">
      <maskPattern>\"SSN\"\s*:\s*\"(.*?)\"</maskPattern>
    </layout>
  </encoder>
  <options>
    <dsn>***</dsn>
  </options>
</appender>

I thought this would be similar to setEncoder. But it results in empty events like this:

Screenshot 2023-06-21 at 20 03 04

I guess then I have to figure out how to manually configure Sentry (with Java code?).

@adinauer
Copy link
Member

@transducer can you please check for processing errors in the Sentry UI? If there's none, you could turn on debug and check the JSON that gets sent to Sentry.

@transducer
Copy link

transducer commented Jun 26, 2023

@adinauer I think something else went wrong in my configuration; locally I missed passing a pattern in the XML, thus the whole of Logback was not properly instantiated and onload also an ERROR was visible from Logback ("PatternLayout("null") - Empty or null pattern"). With proper configuration, it does work!

Screenshot 2023-06-26 at 11 36 36

Thanks a lot for the help both.

@transducer
Copy link

transducer commented Oct 5, 2023

@adinauer it seems breadcrumbs and messages are encoded, but exception messages are not. Any ideas how exception messages could also be using the encoder? In our use case of scrubbing sensitive information, sometimes sensitive information (like an email address or a database spec) ends up in the message, we want to scrub that.

@adinauer
Copy link
Member

adinauer commented Oct 5, 2023

@transducer can you please specify where exactly you see the message you're referring to? Is this on Sentry UI or on the JSON you can see in debug log?

I'm assuming the problem is this line:

message.setMessage(loggingEvent.getMessage());

Just trying to confirm before digging deeper. As a workaround for now you should be able to filter data from errors in beforeSend.

@transducer
Copy link

transducer commented Oct 5, 2023

@adinauer thanks for your reply. It is in the Sentry UI when logging an uncaught exception. It shows the exception message which is not encoded above the stacktrace. Other log messages and breadcrumbs are encoded.

@adinauer
Copy link
Member

adinauer commented Oct 6, 2023

@transducer thanks. Can you confirm the workaround does fix your problem?

@adinauer
Copy link
Member

adinauer commented Oct 9, 2023

@transducer we've created #2973 to track. We want to discuss this a bit more internally before implementing. Will update there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants