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

Adds middleware/serializers construction through Log4j configuration #196

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ppkarwasz
Copy link

If the HttpSenderMiddleware, EventBodySerializer and EventHeaderSerializer objects require additional configuration, instantiation by class name might not be enough (see this question on StackOverflow).

While maintaining backward compatibility, this PR adds the possibility to configure additional components through Log4j:

<SplunkHttp>
    <SomeMiddleware attributeA="a" attributeB="b" />
    <SomeBodySerializer ... />
    <SomeHeaderSerializer ... />
</SplunkHttp>

The elements SomeMiddleware, SomeBodySerializer and SomeHeaderSerializer must be Log4j plugins of element type middleware, eventBodySerializer and eventHeaderSerializer respectively.

This PR contains a simple unit test.

@lbruun
Copy link

lbruun commented Aug 27, 2021

This certainly seems like the way to do it .. and probably should have been like that from the beginning, i.e. since the concept of user configurable serializers was introduced in this project.

Backwards compatibility
When I read the PR it seems to maintain backwards compatibility, right? (i.e. users who have written serializers in the in past, not annotated with @org.apache.logging.log4j.core.config.plugins.Plugin will still be able to use those, albeit without having easy access to Log4j configuration?). If so, very good. Also the PR will not force users to change their existing log4j config ?

Documentation
To make the PR perfect it probably should be accompanied by some change to documentation which explains that from now onwards a serializer intended for use with Log4j can be annotated with @Plugin and the benefits that brings. The README states that the project provides "Example configuration files for all three frameworks that show how to configure the frameworks to write to HTTP Event Collector or TCP ports" but I wouldn't know where these can be found.

@ppkarwasz
Copy link
Author

When I read the PR it seems to maintain backwards compatibility, right? (i.e. users who have written serializers in the in past, not annotated with @org.apache.logging.log4j.core.config.plugins.Plugin will still be able to use those, albeit without having easy access to Log4j configuration?). If so, very good. Also the PR will not force users to change their existing log4j config ?

Yes, that is the idea. The old unit test HttpEventCollectorUnitTest#log4j_simple (which uses the old src/test/resources/log4j_template.xml configuration) should pass successfully.

The new unit test HttpEventCollectorUnitTest#log4j_subcomponents provides an example of new configuration src/test/resources/log4j2_subcomponents_template.xml. I am willing to modify the documentation, but I don't know where to find it.

@bparmar-splunk
Copy link
Contributor

Hi @ppkarwasz,
There are conflicts in this PR. Will you please resolve conflicts so that we can review your changes?

Thanks.

The current mechanism creates the middleware, eventBodySerializer and
eventHeaderSerializer using the default constructor of their classes,
without any configuration possibility.

While maintaining backward compatibility, we allow to configure those
components through Log4j plugin mechanism.
@ppkarwasz
Copy link
Author

@bparmar-splunk: I rebased the contribution and rewrote it to use a builder instead of a 33 parameter constructor (which I deprecated).

BTW: all *Test classes are excluded in your Maven Surefire configuration.

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

Successfully merging this pull request may close these issues.

None yet

3 participants