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

Does Elasticsearch need to include wrappers for java.util.logging and slf4j? #16585

Closed
nik9000 opened this issue Feb 10, 2016 · 7 comments
Closed
Assignees
Labels
:Core/Infra/Logging Log management and logging utilities v5.0.0-alpha1

Comments

@nik9000
Copy link
Member

nik9000 commented Feb 10, 2016

Right now Elasticsearch has three logger implementations - one for log4j, one for java.util.logging, and one for slf4j. What does that buy us? The elasticsearch zip, tar, deb, and rpm distributions bundle log4j and logging.yaml doesn't work without it. When Elaticsearch is used as a client library master no longer tries to configure the logger because that is crazy behavior for a library. Normal Java libraries just pick a logging dependencies and use it. They rely on the user to include whatever bridges are required. Can we get away with being normal?

Proposal: Remove support for java.util.logging and slf4j logging support. We fold the log4j support into its parent class/package. We put a breaking change note for users of the TransportClient that they'll need to include the appropriate shim to point log4j12 at whatever logging library they use. They probably already have it on their classpath. We get to nuke three whole packages, lots of weird try/catch statements, and 2 optional dependencies.

If anyone asks why we still use log4j12 when log4j2 and logback are all around we tell them that it works and we have bigger fish to fry....

@rjernst
Copy link
Member

rjernst commented Feb 10, 2016

Thanks for starting this discussion Nik, I agree our logging setup is crazy. :)

I have an alternative suggestion though: let's use java.util.logging for everything. This removes a dependency, and I don't think we really use anything complicated in log4j? Until we have a separate client, any users can configure bridges to j.u.l, just like they would log4j.

@nik9000
Copy link
Member Author

nik9000 commented Feb 10, 2016

I don't think we really use anything complicated in log4j

The LogConfigurator is the complex bit. Not because it is complex but because the logging features it exposes come from log4j. I get wanting to dump the dependency though. I haven't investigated how difficult it would be to get the same features we have out of j.u.l.

Can we get it in steps? Collapse onto log4j in step 1 and switch to j.u.l is step 2 if we still think it is worth it?

The question of "do we even need our own logging wrapper" is a fine one and will probably be easier to answer when we only have a single logging wrapper rather than three.

@clintongormley clintongormley added the :Core/Infra/Logging Log management and logging utilities label Feb 10, 2016
@rjernst
Copy link
Member

rjernst commented Feb 10, 2016

Can we get it in steps? Collapse onto log4j in step 1 and switch to j.u.l is step 2 if we still think it is worth it?

Absolutely. I think that is a fine approach, and getting rid of slf4j as a start solves a ton of problems we have in gradle and our distributions (how we include this right now is a huge hack and very annoying).

@rjernst
Copy link
Member

rjernst commented Feb 10, 2016

Also, another thing we should look at is logging.yml. This should really just be a prefix in elasticsearch.yml, there is no reason to have a separate config file.

@clintongormley
Copy link
Contributor

Also, another thing we should look at is logging.yml. This should really just be a prefix in elasticsearch.yml, there is no reason to have a separate config file.

++

@clintongormley
Copy link
Contributor

The one thing we'll probably be asked for is different log management schemes, eg #14956

Not sure how much we can support things like this?

@rjernst
Copy link
Member

rjernst commented Feb 14, 2016

We should get the configration/code we have cleaned up, and then consider additional features for how we expose logging, like that. But it should be through our own configuration settings, not through exposing the api of our underlying impl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Logging Log management and logging utilities v5.0.0-alpha1
Projects
None yet
Development

No branches or pull requests

3 participants