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

Provide an additional way to configure the RecordFormatter when using the FluencyBuilderForFluentd #365

Closed
frosiere opened this issue Jan 6, 2022 · 4 comments
Assignees

Comments

@frosiere
Copy link
Contributor

frosiere commented Jan 6, 2022

In some situations, it may be required to customize the RecordFormatter (additional Jackson modules, etc). There is currently a method taking a RecordFormatter and an Ingester (FluencyBuilderForFluentd#buildFromIngester(RecordFormatter recordFormatter, Ingester ingester)) which is nice is some cases.

The configuration becomes complex and requires a bunch of copy/paste when used in combination with the FluencyLogbackAppender

Therefore, to ease the configuration, and dissociate the customization of the recorder from the ingester, it would be great to update the FluencyBuilderForFluentd as follow

public class FluencyBuilderForFluentd {
   ...
   private RecordFormatter recordFormatter = new FluentdRecordFormatter(); // default recorder like now - see buildRecordFormatter
   
   public void setRecordFormatter(RecordFormatter recordFormatter) {
      this.recordFormatter = recordFormatter;
   }
   ...
}

At appender level, this would allow the following customization

public class CustomFluencyLogbackAppender extends FluencyLogbackAppender<ILoggingEvent> {

    @Override
    protected FluencyBuilderForFluentd configureFluency() {
        final var builder = super.configureFluency();
        final var config = new FluentdRecordFormatter.Config();
        config.setJacksonModules(Lists.newArrayList(
                new Jdk8Module(),
                new JavaTimeModule()
        ));
        builder.setRecordFormatter(new FluentdRecordFormatter(config));
        return builder;
    }
}

Any other suggestions to support this use case would be appreciated.

Thanks a lot for your help and for the Fluency support.

@komamitsu komamitsu self-assigned this Jan 9, 2022
@komamitsu
Copy link
Owner

@frosiere Even with setRecordFormatter(), it looks still a bit complicated to inject Jackson modules. And I have another concern that a wrong RecordFormatter can be passed (e.g. passing JsonlRecordFormatter to FluencyBuilderForFluentd.)

How about this?

public class CustomFluencyLogbackAppender extends FluencyLogbackAppender<ILoggingEvent> {

    @Override
    protected FluencyBuilderForFluentd configureFluency() {
        final var builder = new FluencyBuilderForFluentd();
        // "setJacksonModulesForRecordFormatter" is too verbose...?
        builder.setJacksonModulesForRecordFormatter(Lists.newArrayList(
            new Jdk8Module(),
            new JavaTimeModule()
        ));
        return builder;
    }
}

@frosiere
Copy link
Contributor Author

frosiere commented Jan 10, 2022

Good point, proposal is nice and prevents using a wrong formatter. So, this may be the way to go.

An alternative would either be a method taking the FluentdRecordFormatter like
builder.setRecordFormatter(FluentdRecordFormatter formatter)
Or a method taking the record formatter config like
builder.setRecordFormatterConfig(RecordFormatter.Config config)
This would support possible extensions of the config in future releases.

From a more general point of view, only the solution exposing the formatter gives a way to "setup" (enable/disable feature, etc) the internal ObjectMapper by overriding the method AbstractRecordFormatter#registerObjectMapperModules.

Thanks a lot for your reply.

@komamitsu
Copy link
Owner

Yeah, setRecordFormatter() is more powerful. I'll go with it.
I'm a bit busy these days and it would take some time until the feature is released.

@frosiere
Copy link
Contributor Author

Perfect. If I drop a PR, would it be possible to have a 2.6.1 release soon?
Thanks a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants