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

Fix logging-framework static-context headache #8760

Closed
tombujok opened this issue Aug 22, 2016 · 8 comments

Comments

Projects
None yet
4 participants
@tombujok
Copy link
Contributor

commented Aug 22, 2016

As a user I want to be able to use the logger from a static context, whenever there is no access to NodeEngine without any side-effects to the logging configuration.

STATE OF THE ART:
Currently Hazelcast optimises the case when there is more than one HazelcastInstance run on a JVM and they have different logging configurations. Much as I consider this situation feasible and much as I would like to support it I doubt that it's the most common use-case.

Additionally, it is impossible to always access nodeEngine for logging since there are Hazelcast classes that may be used without having an instance (like Config classes, etc.)

There is a possible problem in the most common use-case:

  • one node in one JVM, or a couple of nodes in one JVM
  • the same logging setup configured in the Config.

If somebody uses Logger.getLogger(HazelcastCachingProvider.class); the cached LoggerFactory gets initialised with the default logging system and will never change. Hazelcast will potentially log to two places

  • java.util.logging -> all the loggers accessed from a static context.
  • user-specified logging-system from the Config -> all the loggers accessed from the nodeEngine

FIX:
As a user:

In the one-node per JVM case:

  • I would prefer the statiticly cached LoggerFactory to be overwritten with the one initialised by the newLoggerFactory(String loggerType) so that the logging gets eventually unified.
  • I would like the statiticly cached LoggerFactory to be initialised in the Hazelcast bootstrap sequence, as early as possible in order to reduce the time-spam when the logging is not unified
  • I would like to change the HZ manual to put emphasis on the fact that the logging should be primarily configured by the System.property hazelcast.logging.type in order not to have anything logged to the default logger before the LoggerFactory gets initialised

In the multiple-nodes per JVM case (shared-case):

  • I would like to introduce a flag hazelcast.logging.shared so that the user may indicate that the statiticly cached LoggerFactory does not get overwritten with the by the newLoggerFactory(String loggerType) call
  • I would like to change the HZ manual to put emphasis on the fact that if the shared-logging is enabled all the statically accessed logging messages will land in the default-logging system
  • I would like to change the HZ manual to say that the logging System.properties (logging.type or logging.class) should not be used at all.

@tombujok tombujok added the Team: Core label Aug 22, 2016

@tombujok tombujok added this to the 3.8 milestone Aug 22, 2016

@tombujok tombujok self-assigned this Aug 22, 2016

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2016

the more I'm thinking about it the more hack-ish it appears. I think we should not introduce a new flag to solve our internal issues - it's moving our problems to users = not good.

This is what I see as a better solution:

  1. make sure LoggingService is a) initialized very eargly b) easy to access
  2. use the LoggingService consistently

Now the big question is how to make it easy accessible. Currently it's quite awkward.
LoggingService is a contextual information: "in this context (=with this Hazelcast) I want to use this logging settings.

@pveentjer

This comment has been minimized.

Copy link
Member

commented Aug 22, 2016

There is another feature provided by our logging approach and that is the member that owns the logging, is available. See LoggingServiceImpl.

I'm also not terribly excited about the current logging and would be happier if we could use logging in the traditional way. But we do need to think careful about loosing the 'this member' access.

One of the questions that needs to get addressed is: do we need this information?

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2016

@pveentjer: I find the contextual informations (this member, etc) very useful during log analysis.

@tombujok

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2016

@jerrinot LoggingService doesn't solve the problem of logging in static-context

I thought about it and I think we would be good with:

  • make sure LoggingService is a) initialised very early
  • change the manual to emphasise the fact that logging should be configured using the system.property
  • log a warning if logging is configured in the Config saying that some static-context logs may end-up in the default logging framework

This way:

  • we optimise for the case where there's one node in a single JVM
  • we may log in static context if absolutely necessary
@tombujok

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2016

OK, I managed to remove a lot of places where static-context logging is used

Places where we do not have a Node and logging is used:

  • Config & ClientConfig
  • OSGI bootstrapping classes
  • ServiceLoader
  • CachingProvider (JCache entry-point)
  • AbstractSerializationService (constructed in a builder)

(UPDATED)

@noctarius

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2016

Another very hacky solution would be to put a "LoggingContext" into the thread (storing for example the current HazelcastInstance) and test if this is available, if so you can extract necessary information, otherwise go without it. That way you can use static loggers, have static and instnce-based context logging. As I said, hacky and ... well more of a workaround though but would work since we control most threads anyways.

@tombujok

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2016

Thx @noctarius, I considered it too, but that's a bit too much when it comes to Config since there are classes that most certainly will be used from a user's thread before even starting Hazelcast.

In this case I think that all the logging should be removed from Config classes, and that the config should be validated while passed to HazelcastInstance.

I don't care much about logging in rest of the cases, these can be either removed or a logger could be passed from a ThreadLocal context.

  • OSGI bootstrapping classes
  • ServiceLoader
  • CachingProvider (JCache entry-point)
  • AbstractSerializationService (constructed in a builder)

This way we would ban static-context logging and solve the problem completely.

@tombujok

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2016

It turned out that:

  • there is a lot of places using static-context logging
  • manual already mentions this problem

I limited work on this issue to:

  • switching static-context logging to node-based logging wherever possible
  • fixing logger static-context init section to invoke it as early as possible
  • fixing logger static-context init section to take custom factory under consideration (a bug before)

Recommendations:

  • User node-based logger wherever possible
  • Logger.getLogger() can be used in special cases - it's still better than no logging at all. It does not screw logging but there might be a case that some static loggers log to JDK logging of system properties not used (manual mentions that).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.