Standardize logging libraries for Java components

[woop]

Currently there is a mix of logging libraries being used in the Java components (ingestion, serving, core). We currently used both SLF4J and Log4j2.

The plan is to standardize on only SLF4J, and this issue tracks the work item.

[ches]

Bringing some rationale into context here, @davidheryanto pointed out that SLF4J is recommended for Beam.

Practically speaking I believe this means library modules in Feast should use the SLF4J API (only slf4j-api dependency), and the executable applications use Logback as the de facto implementation.

[ches]

Reopening as I think #430 hasn't completely addressed this:

1. That PR added log4j-slf4j-impl which "allows applications coded to the SLF4J API to use Log4j 2 as the implementation". I think (?) what we decided is that we want the opposite, SLF4J as the implementation (and API), if we are already committed to it as the "lowest common denominator" by Beam. Good summary info about all this on SO.
2. We still have direct usage of both APIs in Feast. The last holdout of Log4j2 is core/src/main/java/feast/core/log/AuditLogger.java. SLF4J is used a lot "by hand" and with the Lombok annotation.

I guess my acceptance criterion for closing this issue would be:

A Feast operator does not need to configure both a logback.xml and log4j2.xml to change logging behavior for their deployment. (A Feast developer shouldn't either).

If there are Feast dependencies which use the Log4j (2) APIs, we should take care of the bridging so that they don't need to think about all this.

[Yanson]

Agree that #430 doesn't fully fix this issue. All that did was remove any impl from ingestion and standardise on log4j impl for core and serving to give them both JSON logging and env conf.

The link you posted suggests using log4j2 API, not "SLF4J as the implementation (and API)".

Given Dataflow will work with log4j or slf4j, I don't think it matters which we standardise on. However, since log4j2 offers more functionality (which we are using in AuditLogger), I think that is the more sensible choice.

Alternatively we could stick to slf4j just for ingestion and use log4j for core + serving.

Also note that the feast java sdk has an import of slf4j, though the instance is not used. I think I would prefer slf4j as a more standard api dependency for a library but it could be removed altogether.

Happy to make the changes if we decide on a forward path.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[ches]

The link you posted suggests using log4j2 API, not "SLF4J as the implementation (and API)".

(For the record, my intention for the SO link I shared was for its objective summary info, not its (biased) recommendation).

I don't have a particularly strong opinion on which to use, and frankly I wish I could have some of my life back that has gone to the age-old pissing contest of Java logging frameworks and finding libraries that include concrete impls that require exclusions… I would just like a clear guideline stating what to use, per module if mixing is really justified.

AuditLogger seems to have gotten the axe now, so I believe there is no usage of Log4j2 API and thus any of its exclusive features (except optional JSON format in config). But, Core and Serving have long shipped log4j2.xml, so changing implementation would be annoying for operators at upgrade to change format of custom/overriding configurations.

I guess we should stay with things as they are, and document it. I'd agree that slf4j-api still seems much more established for a library like the SDK at this point in time.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[danielsiwiec]

Does this issue still require any work? I'd be happy to help wrap this one up

[danielsiwiec]

@woop I'm happy to lean in here, if this still needs any work

[woop]

Hey @danielsiwiec. Happy for contributions here if you want to take a stab at it. This issue has gone under the radar for quite some time.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.