Allow multi-enum usage of opslogger

[ravn]

Currently OpsLogger only allows for a single enum for configuring the logging system. For large modular systems this may not be a usable approach.

Would reworking OpsLoggerFactory to allow multiple calls to build() with different enums but combining the use of the same underlying BasicOpsLogger be an option?

[seanjreilly]

We could do something like that, yes.

As a workaround in the meantime, since Java generics are implemented using type erasure, you can actually do what you want with a cast:

OpsLogger<Foo> foo = new OpsLoggerFactory.build();
OpsLogger<Bar> bar = (OpsLogger<Bar>) foo;

Going forward, we could potentially encapsulate this behaviour into a method on OpsLoggerFactory, and have it perform the cast:

OpsLoggerFactory factory = new OpsLoggerFactory();
OpsLogger<Foo> foo = factory.build();
OpsLogger<Bar> bar = factory.convert(Bar.class, foo); //a cast to OpsLogger<Bar> is performed inside the method

Or we could encapsulate the behaviour in a different class, perhaps something like this:

MultiOpsLogger multiLogger = new OpsLoggerFactory().buildMulti();
multiLogger.get(Foo.class).log(Foo.MessageA, "some argument");
multiLogger.get(Bar.class).log(Bar.MessageB, "some other argument");

Or we could implement something in the Factory itself. I'm not as sure how that might work, and I'm a little worried about calling one of the mutator methods (like setAsync) after build is called, and how that might work.

What do you think?

[ravn]

I really think that the current design with OpsLoggerFactory being able to
provide several vastly different backends, has already crossed the
complexity threshold, and should be refactored into something with less
ifs. I think the slf4j API got it right. Provide an interface and let the
implementor provide a backend (one for testing and one for production).

That said, I think that explicity exploiding type erasure is too much
bending ovre backwards to avoid adjusting the API. I have some spare time
this week, so I would like to experiment a little to see how it would look.

On Fri, Apr 10, 2015 at 5:38 PM, Sean Reilly notifications@github.com
wrote:

We could do something like that, yes.

As a workaround in the meantime, since Java generics are implemented using
type erasure, you can actually do what you want with a cast:

OpsLogger foo = new OpsLoggerFactory.build();OpsLogger bar = (OpsLogger) foo;

We could potentially encapsulate this behaviour into a method on
OpsLoggerFactory, and have it perform the cast:

OpsLoggerFactory factory = new OpsLoggerFactory();OpsLogger foo = factory.build();OpsLogger bar = factory.convert(Bar.class, foo); //a cast to OpsLogger is performed inside the method

Or we could encapsulate the behaviour in a different class, perhaps
something like this:

MultiOpsLogger multiLogger = new OpsLoggerFactory().buildMulti();
multiLogger.get(Foo.class).log(Foo.MessageA, "some argument");
multiLogger.get(Bar.class).log(Bar.MessageB, "some other argument");

Or we could implement something in the Factory itself. I'm not as sure how
that might work, and I'm a little worried about calling one of the mutator
methods (like setAsync) after build is called, and how that might work.

Thoughts?

—
Reply to this email directly or view it on GitHub
#6 (comment)
.

Thorbjørn Ravn Andersen - "...plus...Tubular Bells!"

[ravn]

Initial impression is that the underlying "log an Enum which is a LogMessage" part can relatively easily be changed into "log an Enum" all the way up to AsyncOpsLogger and BasicOpsLogger, and then the build() method can wrap a singleton OpsLoggerBase (just using a LogMessage interface) into the full blown OpsLogger interface (using T) with a small wrapper.

[seanjreilly]

Just out of curiosity, with a singleton performing the actual logging, how would you handle multiple log destinations/formats? For example, logging some messages to the console, some to a file, etc?

We have a definite use case with one of our clients where log messages are sent to different files, and each file contains log messages from a specific enum only.

[ravn]

An instance of OpsLoggerFactory (javax.inject.Provider?) has a
single configuration which must be frozen (which in turn speaks for
constructor dependencies) after the first call to build().

If another configuration is needed, create another instance of
OpsLoggerFactory. is there any reason that the use case cannot be done
with the current code?

On Tue, Apr 14, 2015 at 4:12 PM, Sean Reilly notifications@github.com
wrote:

Just out of curiosity, with a singleton performing the actual logging, how
would you handle multiple log destinations/formats? For example, logging
some messages to the console, some to a file, etc?

We have a definite use case with one of our clients where log messages are
sent to different files, and each file contains log messages from a
specific enum only.

—
Reply to this email directly or view it on GitHub
#6 (comment)
.

Thorbjørn Ravn Andersen - "...plus...Tubular Bells!"

[ravn]

My primary concern was to have multiple enums use the same logger, not to have multiple enums have multiple loggers.

[seanjreilly]

Your use case is valid, but unfortunately, so is the other one. We have at least one project using multiple enums with multiple loggers in production right now.

As far as freezing the output of OpsLoggerFactory, I can't say that I've ever used the factory pattern and had the factory keep a reference to the result once build is called. I wonder if that would be intuitive to users. What would happen if a user accidentally calls one of the mutation methods after calling build?

[ravn]

Perhaps you would care to elaborate on the use case you mention, so I can
get a better idea of how you currently solve the problem.

These stories might be beneficial to add to the project so it is easier for
non-EqualExperts to understand the various design issues.

On Tue, Apr 14, 2015 at 5:14 PM, Sean Reilly notifications@github.com
wrote:

Your use case is valid, but unfortunately, so is the other one. We have at
least one project using multiple enums with multiple loggers in production
right now.

As far as freezing the output of OpsLoggerFactory, I can't say that I've
ever used the factory pattern and had the factory keep a reference to the
result once build is called. I wonder if that would be intuitive to users.
What would happen if a user accidentally calls one of the mutation methods
after calling build?

—
Reply to this email directly or view it on GitHub
#6 (comment)
.

Thorbjørn Ravn Andersen - "...plus...Tubular Bells!"

[seanjreilly]

I'm afraid that the specific details are confidential, but the gist is:

• Warnings and non-critical events are output to one file.
• Critical events (somebody is paged, even at night) are output to a different file.
• The files are associated with different enums, but should have exactly the same formats otherwise.

With this use case, a single factory can be used quite easily:

OpsLoggerFactory factory = new OpsLoggerFactory()
    .setStackTraceStoragePath(Paths.get("/tmp/stacktraces"))
    .setAsync(true)
    .setCorrelationIdSupplier(context) //defined somewhere else, but used for both loggers
    .setErrorHandler(TheApplication::loggingError);

OpsLogger<Foo> fooLogger = factory.setPath(Paths.get("/logs/foo.log")).build();

OpsLogger<Bar> barLogger = factory.setPath(Paths.get("/logs/bar.log")).build();

We commonly use the factory pattern in this way: calling build, mutating one or two settings, and calling build again is a very nice way to describe building two instances with specific differences.

[ravn]

There is no tests in OpsLoggerFactoryTest which test this usecase behavior with multiple calls to build(). To my understanding this mean that my implementation is as "good" as yours, and that the usage you describe is exploiting implementation specific details.

[seanjreilly]

Hi @ravn,

Sorry for the delay in replying. I've been on holiday.

You're absolutely right that there were no tests in OpsLoggerFactory test that specified this use case behaviour. Thanks for pointing that out! I've just added some to clarify this expectation. But either way, the tests aren't an inviolable contract. Rather, I view them as expressing our current understanding of what we expect the code to do, which is entirely subject to change or rethink. Tests can always be changed if that's what we decide to do.

(As an aside, that's the reason behind the OpsLoggerFactory in the first place. I want to keep as much code as possible out of the backwards-compatibility zone, to protect our ability to make as many changes as possible.)

So please don't ever be hesitant to make a suggestion like this, even if there is a current test that would fail or need to be changed!

Now onto the main topic: why I feel the current behaviour is a good thing to stick to.

I feel like the current approach works better with the Principle of Least Astonishment. Having a setter that works when it is first called, but that throws an exception (or worse, is silently ignored) doesn't feel like what the majority of users would expect to happen.

Also, introducing a new lifecycle phase (build has already been called at least once) for OpsLoggerFactory will add complexity, and as you've rightly pointed out, the implementation for that class is already complicated enough.

If we're going to add a facility to adapt an existing OpsLogger to a new LogMessage enum, I think we'd be much better served by making that behaviour explicit (either by an adapt method or some sort of MultiOpsLogger class) in a way where it can't possibly be a surprise to a user.

[ravn]

I do not have a preference for any particular behaviour. In my experience it is just a good thing if configurations are immutable after being made ready to be used by other code. Perhaps introducing a generic interface implemented by OpsLoggerFactory so that can be injected?

I'm looking forward to seeing what you come up with.

[seanjreilly]

Perhaps introducing a generic interface implemented by OpsLoggerFactory so that can be injected?

I'm confused. This sounds intriguing, but I don't think I properly understand what you're suggesting here....

Is there any chance you could illustrate a very short example (even in pseudocode would be super helpful) to help me understand what you mean by this?

[seanjreilly]

@ravn: I've been thinking about this, and I think I understand now. Are you suggesting that you would like to inject an OpsLogger factory as a dependency of some class or service?

If so, that's a fascinating use case, and I'd like to hear more about it. This is the first time I've heard of anyone mention this use case — the dependency injection use case we've considered before uses created OpsLogger instances as singletons, injecting them into application classes. In this use case, OpsLoggerFactory instances are used to create dependencies, but aren't injected into anything themselves — they are only interacted with when creating the object graph.

I'd love to hear more about what you're thinking about accomplishing by injecting a factory... it seems quite interesting.

In the meantime, I'm leaning towards the MultiOpsLogger solution from above:

MultiOpsLogger multiLogger = new OpsLoggerFactory().buildMulti();
OpsLogger<Foo> foo = multiLogger.build();
foo.log(Foo.MessageA, "some argument");
OpsLogger<Bar> bar = multiLogger.build();
bar.log(Bar.MessageB, "some other argument");

In this case, OpsLoggerFactory is the mutable class used to express configurations. Calling buildMulti "freezes" the configuration, and indeed the OpsLogger will contain a single frozen OpsLogger instance that is a singleton from that point on. (For now, it might as well just use type erasure, but it can just as easily contain a BaseOpsLogger instance, and wrap it when build is called, if we ever need that in the future).

In fact, since the method that returns the actual OpsLogger has the same signature on MultiOpsLogger as it does on OpsLoggerFactory, there is an opportunity to extract an interface that both classes implement.

In fact, the regular build method in OpsLoggerFactory could probably just call buildMulti().build() and then throw the MultiBuilder away — although I'd still like to keep the original method around as a convenience.

What do you think?