rfoltyns / log4j2-elasticsearch

Log4j2 Elasticsearch Appender plugins
Apache License 2.0
174 stars 45 forks source link

Package naming conventions #56

Open globalbus opened 4 years ago

globalbus commented 4 years ago

I'm using this brilliant library in modular project (OSGi). Of course library is not OSGi complaint, but bunch of wrapping instructions and dirty solution for refreshing log4j2 PluginRegistry and log4j2 context is enough to make it working in apache karaf environment.

Newer versions are much more problematic with running in modular environment. It's because weird package naming inside jars. In example, inside log4j2-elasticsearch-core we have

For now, biggest fault is com.fasterxml.jackson.core. That package is already present in jackson-core and that thing breaks package resolving.

There is a special reason for that kind of packaging? Most of projects in java world tries to keep things under single package root, equal to project groupId + moduleName. That really helps with modularity problems.

rfoltyns commented 4 years ago

Thank you for the good word! :+1:

I agree, unfortunately basic good practices are not followed when it comes to package names. In most cases it's due to the need to access package-private or protected classes, either for extensions (see *MixIn classes) or to introduce some performance improvements - com.fasterxml.jackson.core is a good example as it needs to do some wizardry with JsonWriteContext in single threaded mode.

Also, most classes are just dumped into the main package - this will change in 2.0 (end of next year at least) when core will be split into multiple modules.

I'll have a look at those packages and see what I can do. So far:

Is this issue blocking you completely atm? Is 1.4.4 unusable for you?

globalbus commented 4 years ago

I'm not blocked, that's a kind of general question. I'm doing modules since years, but my descendants in company could be not very happy after taking over such legacy.

If I simply say "I will not use singleThread option", package 'com.fasterxml.jackson.core' is optional. I must omit this export and that's all (classes from this package will be never loaded, if this option is disabled). Splitting that to separate jars will be a good approach. Then I could load 'com.fasterxml.jackson.core' as fragment bundle (kind of trick to attach it to jackson-core bundle). But keeping all of thirdparties in single jar will be also problematic (I can attach fragment to only one host bundle).

rfoltyns commented 4 years ago

"I will not use singleThread option", package 'com.fasterxml.jackson.core' is optional. I must omit this export and that's all (classes from this package will be never loaded, if this option is disabled)

I was really hoping that this single design decision and classloading tricks will do the job for all cases..

Separate jar for jackson package-private classes would also contain actual SingleThreadJsonFactory from org.appenders.log4j2.elasticsearch. I'm not sure how well fragment bundles would handle that.

Shading classes into org.appenders.log4j2.elasticsearch.thirdparty might be a way to go for Log4j2 classes here. Similar approach was taken in Netty project - JCTools collections are shaded to prevent any classloading issues. I was reluctant to do it this way, but I might have to re-consider.

globalbus commented 4 years ago

will do the job for all cases..

For openjdk, I would say - yes. I'm only using this for container environment in kubernetes, so I could feel safe with that (no other implementation could be used accidentally).

Separate jar for jackson package-private classes would also contain actual SingleThreadJsonFactory from org.appenders.log4j2.elasticsearch. I'm not sure how well fragment bundles would handle that.

It would handle, but if SingleThreadJsonFactory have their separate unique package name (that could be exported and consumed elsewhere).

Yes, I'm also not a huge fan of shading classes. But decision about that should be consulted with question "Am I using internal APIs of third party library, that could be incompatibly changed between versions without notice?". Jackson library in the past had breaking changes between minor versions. Anyway, sometimes it's easier to handle such things with reflection (checking unaccessible field existence before reading). And reflection it's pretty fast in last JRE implementations. Or even multi approach, checking conditions via reflection before loading faster hacks.

rfoltyns commented 4 years ago

Incompatibility is a risk with both reflection and package-private access approaches. I'll be very reluctant to add any reflection-based logic to the code base. If there's a need for a package-private access, I'd rather try to contribute to the source project to make this access easier.

reflection it's pretty fast in last JRE implementations

Regardless of latency, it just feels wrong. If property is private, there's a reason for it. If it's package-private or protected - I can take the risk, especially if it's on the hot path.

rfoltyns commented 4 years ago

Thankfully, repackaging will be possible after jackson-core:2.12.0 release. Thanks, Tatu! :+1:

I'll release it in 1.5 if possible.

globalbus commented 4 years ago

Good to hear. Please, send me a notification before release, I will try to check it for compatibility in modular system.

rfoltyns commented 3 years ago

com.fasterxml.jackson.core classes were moved to appenders-jackson-st. compile scope, but not required by default.

I'll extract org.appenders.core.logging in 1.5.0 as well.

Can you handle org.apache.logging.log4j.core.jackson? I'd would just deprecate it and repackage in 2.0.

globalbus commented 3 years ago

looks good for me. I really appreciate it.

with org.apache.logging.log4j.core.jackson I have no problems at the moment, because it's not exported explicitly by pax-logging (no version conflict).

rfoltyns commented 3 years ago

org.appenders.core.logging classes were moved to appenders-logging.

I pushed latest log4j2-elasticsearch snapshots to Sonatype repos.

rfoltyns commented 3 years ago

Released in 1.5.0.

org.apache.logging.log4j.core.jackson will be moved in 2.0.

Enjoy!