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

Prevent Logback from starting JsonProvider (remove Lifecycle interface) #680

Closed
brenuart opened this issue Oct 14, 2021 · 0 comments · Fixed by #681
Closed

Prevent Logback from starting JsonProvider (remove Lifecycle interface) #680

brenuart opened this issue Oct 14, 2021 · 0 comments · Fixed by #681
Assignees
Milestone

Comments

@brenuart
Copy link
Collaborator

The JsonProvider interface currently extends Lifecycle. This means that Logback will call its start() method just after configuration properties are set. This is a standard pattern in the Logback ecosystem and works ok for most JsonProvider except a few like GlobalCustomFieldsJsonProvider and PatternJsonProvider.

These two providers need a JsonFactory to parse the value of some of their configuration properties. The factory is not provided by Logback but by the component to which the JsonProvider will be assigned. It is therefore not available at the time Logback calls the start() method. Parsing of the JSON properties is therefore delayed until a JsonFactory is set and will never happen during the lifecycle of component. As a comment already says in the source code, "parsing cannot occur during start() because the jsonFactory is not already set when Logback invokes the method".

Lifecycle of JsonProvider should be entirely delegated to the CompositeJsonFormatter or NestedJsonProvider to which they are assigned without Logback interfering at any time. These two components already call the start/stop method at the appropriate time and take care of assigning a JsonFactory before start() is invoked.

The conclusion is that JsonProvider(s) should not implement the Logback Lifecycle interface at all. This is totally useless since nothing will be done because the JsonFactory is not already set. This also makes the code more complex and moves the validation of the component state outside of the start() method where it usually goes.

brenuart added a commit that referenced this issue Oct 14, 2021
…tring

Issue #680: no auto-start of JsonProviders
- remove Lifecycle from the JsonProvider interface to prevent Logstash from auto-starting the provider
- declare the start/stop method initially defined by the Lifecycle interface in the JsonProvider interface
- now that start() is called only once, parse the JSON string during start()

Issue #679: remaining characters after reading JSON string
- raise an error if some characters remain after reading a JsonNode out of the string
@brenuart brenuart self-assigned this Oct 18, 2021
brenuart added a commit that referenced this issue Oct 29, 2021
Issue #680: no auto-start of JsonProviders
- remove Lifecycle from the JsonProvider interface to prevent Logstash from auto-starting the provider
- declare the start/stop method initially defined by the Lifecycle interface in the JsonProvider interface
- now that start() is called only once, parse the JSON string during start()

Issue #679: remaining characters after reading JSON string
- raise an error if some characters remain after reading a JsonNode out of the string
@philsttr philsttr added this to the 7.0 milestone Nov 13, 2021
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 a pull request may close this issue.

2 participants