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 additivity=false to prevent double logging #843

Merged
merged 4 commits into from
Jul 9, 2024

Conversation

labkey-willm
Copy link
Contributor

@labkey-willm labkey-willm commented Jul 2, 2024

Rationale

the block at the start of the Loggers could cause the subsequent calls to create double logs. Setting additivity="false" will prevent that.

https://logging.apache.org/log4j/2.x/manual/configuration.html#Additivity

Related Pull Requests

Changes

@labkey-adam
Copy link
Contributor

I'm not an additivity expert by any means, but are you sure these parameters are needed here? My reading of the docs says this is useful only when a Logger element specifies an appender and we want its logging to go to just that appender (not the root as well). Most of these Logger elements don't specify an appender. They are present to provide the option (typically during development) of lowering the logging level for specific packages OR to raise the level for noisy third-party libraries. I don't see why we'd need additivity for these.

@labkey-adam
Copy link
Contributor

There are only three Logger elements here that specify an appender; I'll comment directly on those lines.

Copy link
Contributor

@labkey-adam labkey-adam left a comment

Choose a reason for hiding this comment

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

See comments

server/embedded/src/main/resources/log4j2.xml Outdated Show resolved Hide resolved
@labkey-adam
Copy link
Contributor

I see now that a couple of these additivity parameters were already present (not added in this PR). Comments still hold, though... additivity plus CONSOLE appender seems unnecessary.

@labkey-adam
Copy link
Contributor

labkey-adam commented Jul 3, 2024

I'm not an additivity expert by any means, but are you sure these parameters are needed here? My reading of the docs says this is useful only when a Logger element specifies an appender and we want its logging to go to just that appender (not the root as well). Most of these Logger elements don't specify an appender. They are present to provide the option (typically during development) of lowering the logging level for specific packages OR to raise the level for noisy third-party libraries. I don't see why we'd need additivity for these.

This is the very first scenario cited on the doc page as an example where additivity is not needed: https://logging.apache.org/log4j/2.x/manual/configuration.html#Additivity

@labkey-willm
Copy link
Contributor Author

I'm not an additivity expert by any means, but are you sure these parameters are needed here? My reading of the docs says this is useful only when a Logger element specifies an appender and we want its logging to go to just that appender (not the root as well). Most of these Logger elements don't specify an appender. They are present to provide the option (typically during development) of lowering the logging level for specific packages OR to raise the level for noisy third-party libraries. I don't see why we'd need additivity for these.

This is the very first scenario cited on the doc page as an example where additivity is not needed: https://logging.apache.org/log4j/2.x/manual/configuration.html#Additivity

yes, you're quite right, and this confirms what we just found with the chef version, is that having additivity=false on loggers w/o specific appenders results in no logs at all for that logger. I'll fix things up.

Copy link
Contributor

@labkey-adam labkey-adam left a comment

Choose a reason for hiding this comment

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

A couple remaining tweaks

server/embedded/src/main/resources/log4j2.xml Outdated Show resolved Hide resolved
server/embedded/src/main/resources/log4j2.xml Outdated Show resolved Hide resolved
server/embedded/src/main/resources/log4j2.xml Outdated Show resolved Hide resolved
@labkey-willm labkey-willm merged commit d0999f5 into release24.7-SNAPSHOT Jul 9, 2024
4 of 5 checks passed
@labkey-willm labkey-willm deleted the 24.7_fb_fix-log4j-double-logging branch July 9, 2024 18:08
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.

2 participants