esanchezros / quickfixj-spring-boot-starter

Spring Boot Starter for QuickFIX/J
Apache License 2.0
125 stars 57 forks source link

log-factory `compositelog` is missing in QuickFixJClientConfiguration #110

Closed wtiann closed 1 year ago

wtiann commented 1 year ago

In the README, log-factory has value of compositelog But it's not configured in QuickFixJClientConfiguration

esanchezros commented 1 year ago

Thanks for reporting, I'll take a look and add the missing factory

esanchezros commented 1 year ago

The problem with compositeLog is that it requires an array of LogFactorys. At the moment it's possible to configure a log factory like this:

quickfixj.server.log-factory=screen

Maybe for compositeLog we could have something like this:

quickfixj.server.log-factory=composite
quickfixj.server.log-factory.composite.log-factories[0]=screen
quickfixj.server.log-factory.composite.log-factories[1]=file

and make the List<LogFactory> bean created as part of this compositeLog bean accessible too. I'll give this a go and see if it's possible, let me know if you have any other thoughts on this

wtiann commented 1 year ago

This looks good. Or set the properties like quickfixj.server.log-factory.composite.log-factories=screen, file ? Whichever is approachable.

esanchezros commented 1 year ago

Hey @wtiann

I'd like to discuss this feature as I've started implementing it and a few questions were raised.

CompositeLogFactory takes an array of LogFactorys as the only argument in the constructor. The idea as described in the documentation is Allows multiple log factories to be used with QuickFIX/J. For example, you could log events to the console and also log all events and messages to a file.

The other available LogFactorys take a SessionSettings as an argument, but in theory, these settings could come from the same config file or from other config files. For example:

quickfixj.server.log-factory=composite
quickfixj.server.log-factory.composite.log-factories[0].type=screen
quickfixj.server.log-factory.composite.log-factories[0].config=classpath:quickfixj-server.cfg
quickfixj.server.log-factory.composite.log-factories[1].type=file
quickfixj.server.log-factory.composite.log-factories[1].config=classpath:quickfixj-server-extra.cfg
quickfixj.server.log-factory.composite.log-factories[2].type=screen
quickfixj.server.log-factory.composite.log-factories[2].config=classpath:quickfixj-server-screen.cfg

The alternative is to delegate the creation of the LogFactorys to the developer and just define the composite log factory as this:

@Configuration(proxyBeanMethods = false)
@ConditionalOnMissingBean(name = "clientLogFactory")
@ConditionalOnProperty(prefix = "quickfixj.client", name = "log-factory", havingValue = "composite")
static class CompositeLogFactoryConfiguration {

    @Bean
    public LogFactory clientLogFactory(List<LogFactory> compositeLogFactories) {
        return new CompositeLogFactory(compositeLogFactories.toArray(new LogFactory[0]));
    }
}

The third option is to assume the same SessionSettings will apply to all LogFactorys but the developer will be able to define further factories if needed as they will be injected in the list of factories.

wtiann commented 1 year ago

Hi @esanchezros , Maybe we can have a mix of options 2 and 3? By default, it has SessionSetting in all LogFatroy. And developer will define a new one if they need.

esanchezros commented 1 year ago

Fixed on 2.16.1

I went with allowing the developer to define any LogFactory in their Spring context. For example:

quickfixj.client.log-factory=compositelog

and

    @Configuration
    @EnableQuickFixJClient
    @PropertySource("classpath:client-log-factory/client-composite-log-factory.properties")
    static class ClientCompositeLogFactoryConfiguration {

        @Bean
        public LogFactory screenLogFactory(SessionSettings clientSessionSettings) {
            return new ScreenLogFactory(clientSessionSettings);
        }

        @Bean
        public LogFactory slf4jLogFactory(SessionSettings clientSessionSettings) {
            return new SLF4JLogFactory(clientSessionSettings);
        }
    }