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 property to specify the loggers where the SentryAppender is attached #2173

Merged
merged 10 commits into from
Mar 13, 2023
Merged

Conversation

marbon87
Copy link
Contributor

@marbon87 marbon87 commented Jul 14, 2022

📜 Description

This pull-request adds the property sentry.loggers, which can be configured with a list of loggers, where the sentry appender is registered and defaults to the ROOT logger.

💡 Motivation and Context

The current implementation of the spring-boot-starter-sentry registers the sentry appender only to the root logger.
If the logback configuration contains loggers, that are not additive, the log messages from children of the non addtive logger are not sent to sentry.

💚 How did you test it?

Added a new test to SentryLogbackAppenderAutoConfigurationTest

📝 Checklist

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

🔮 Next steps

@adinauer
Copy link
Member

@marbon87 thank you for opening this PR. Can you provide some more details on how you configure your logback loggers and appenders.

As an alternative to this PR developers can also simply add the SentryAppender to their logback.xml and then add appender-refs to all the loggers.

@marbon87
Copy link
Contributor Author

@adinauer the loggers are configured like this:

	<root level="ERROR">
		<appender-ref ref="APP_SYSTEM_OUT" />
		<appender-ref ref="SPRINGLOG" />
	</root>
	
	<logger name="de" level="OFF" additivity="false" />

	<logger name="de.foo.bar" level="WARN">
		<appender-ref ref="APP_INFO"/>
		<appender-ref ref="APP_WARNING"/>
		<appender-ref ref="APP_ERROR"/>
		<appender-ref ref="GELF"/>
		<appender-ref ref="SPRINGLOG" />
	</logger>

If i manually define the SentryAppender in logback.xml and use appender-ref for all loggers, i cannot use the comfortable configuration through spring boot by setting properties like sentry.context-tags.

@philipphofmann
Copy link
Member

@adinauer, maybe you missed this PR somehow. Do we need more information from @marbon87, or can we review this PR?

@adinauer
Copy link
Member

Nothing's missing as far as I know, I just didn't get around to doing the review yet - sorry.

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 @marbon87 and sorry for taking so long - we've been pretty busy lately. I've changed the PR a bit, can you please take another look and tell me what you think about them. If you're OK with them, I'll merge the PR.

return loggers;
}

public void setLoggers(final @NotNull List<String> loggers) {
Copy link
Member

Choose a reason for hiding this comment

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

I had to add the setter here to make the new test pass.

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2022

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (3a20d40) 81.14% compared to head (1fd0fcb) 81.17%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2173      +/-   ##
============================================
+ Coverage     81.14%   81.17%   +0.02%     
- Complexity     4096     4102       +6     
============================================
  Files           333      333              
  Lines         15193    15213      +20     
  Branches       1980     1980              
============================================
+ Hits          12329    12349      +20     
  Misses         2084     2084              
  Partials        780      780              
Impacted Files Coverage Δ
.../spring/boot/jakarta/SentryLogbackInitializer.java 96.87% <100.00%> (+0.72%) ⬆️
...o/sentry/spring/boot/jakarta/SentryProperties.java 80.00% <100.00%> (+2.22%) ⬆️
...o/sentry/spring/boot/SentryLogbackInitializer.java 96.87% <100.00%> (+0.72%) ⬆️
...n/java/io/sentry/spring/boot/SentryProperties.java 77.41% <100.00%> (+3.34%) ⬆️

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@marbon87
Copy link
Contributor Author

Sorry @adinauer, i overlooked your feedback. Your changes look good to me.

@marbon87
Copy link
Contributor Author

Any updates on this?

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.

I've added a changelog entry and duplicated the changes into the jakarta module (for Spring Boot 3). I've also moved the config properties under sentry.logging. Thanks @marbon87 🚀

@adinauer adinauer merged commit 08b072d into getsentry:main Mar 13, 2023
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

4 participants