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

Ensure Logback is refreshed before application starts #8238

Merged
merged 8 commits into from Dec 16, 2022

Conversation

driverpt
Copy link
Contributor

Attempts to fix #8139

@graemerocher
Copy link
Contributor

I believe this will be fixed by #8235 which moves logback to runtime intiailization

@driverpt
Copy link
Contributor Author

driverpt commented Oct 27, 2022

@graemerocher , with this modification you can keep build time initialization, it just resets all internal logback caches when Micronaut Inits.

The issue is that Logback is initialized when JVM Starts. The LoggerContext#reset only resets it's internal caches, so we can take advantage of Logback being previously initialized at build time. Won't improve much in terms of performance, but will add something

@graemerocher
Copy link
Contributor

This seems safe enough to target 3.8.x branch, can you do that?

@driverpt driverpt changed the base branch from 4.0.x to 3.8.x October 27, 2022 14:06
@driverpt
Copy link
Contributor Author

Done. Can you unblock the CI? I suspect that the test will fail

@graemerocher
Copy link
Contributor

not sure why it is blocked 🤔

@graemerocher graemerocher added this to the 3.8.0 milestone Oct 27, 2022
@graemerocher graemerocher added the type: improvement A minor improvement to an existing feature label Oct 27, 2022
@graemerocher
Copy link
Contributor

@n0tl3ss this might help with the logging issue for AWS/OCI

@driverpt
Copy link
Contributor Author

Is there any ETA on this?

@graemerocher
Copy link
Contributor

waiting for feedback from @n0tl3ss

@driverpt
Copy link
Contributor Author

driverpt commented Oct 31, 2022

@n0tl3ss , can you help out here? This is currently the only blocker we have to go live with Micronaut

@graemerocher
Copy link
Contributor

@driverpt from the looks of the change it seems like you could wire this into your existing application without a new version of Micronaut necessarily blocking you?

@n0tl3ss
Copy link
Member

n0tl3ss commented Nov 2, 2022

Hi, @driverpt. I created copy of this pull request: #8263.

@driverpt
Copy link
Contributor Author

driverpt commented Nov 2, 2022

@n0tl3ss , fixed checkstyle issue

@n0tl3ss
Copy link
Member

n0tl3ss commented Nov 2, 2022

@n0tl3ss this might help with the logging issue for AWS/OCI

I tried but without success

@n0tl3ss n0tl3ss mentioned this pull request Nov 4, 2022
@graemerocher
Copy link
Contributor

please fix the broken tests

@driverpt
Copy link
Contributor Author

driverpt commented Nov 4, 2022

I will, but at the end of the day

@driverpt
Copy link
Contributor Author

driverpt commented Nov 4, 2022

@graemerocher @n0tl3ss , GitHub Workflow needs a maintainers approval to run

String logbackXml = logbackXmlLocation.orElse(DEFAULT_LOGBACK_LOCATION);

try {
URL resource = getClass().getClassLoader().getResource(logbackXml);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@graemerocher @n0tl3ss , please review this part. I need your opinion on this one

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think something like

    @Override
    public void refresh() {
        getLoggerContext().reset();
        try {
            new ContextInitializer(getLoggerContext()).autoConfig();
        } catch (JoranException e) {
            throw new LoggingSystemException("Error while refreshing Logback", e);
        }
    }

would do a trick.

@driverpt driverpt requested review from n0tl3ss and removed request for dstepanov November 4, 2022 16:16
Copy link
Member

@n0tl3ss n0tl3ss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is io.micronaut.management.endpoint.loggers.impl.LogbackLoggingSystem class that might replace io.micronaut.logging.impl.LogbackLoggingSystem. Should it also override refresh() method?

String logbackXml = logbackXmlLocation.orElse(DEFAULT_LOGBACK_LOCATION);

try {
URL resource = getClass().getClassLoader().getResource(logbackXml);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think something like

    @Override
    public void refresh() {
        getLoggerContext().reset();
        try {
            new ContextInitializer(getLoggerContext()).autoConfig();
        } catch (JoranException e) {
            throw new LoggingSystemException("Error while refreshing Logback", e);
        }
    }

would do a trick.

@driverpt
Copy link
Contributor Author

driverpt commented Nov 5, 2022

There is io.micronaut.management.endpoint.loggers.impl.LogbackLoggingSystem class that might replace io.micronaut.logging.impl.LogbackLoggingSystem. Should it also override refresh() method?

I'm not a fan of copy + paste the same code in 2 classes. What do you suggest?

@driverpt
Copy link
Contributor Author

@n0tl3ss , were you able to get this "Fix" running smoothly?

I've been testing this and it seems like after I call loggerContext.reset() that it goes completely silent.

@driverpt
Copy link
Contributor Author

@n0tl3ss , can you please test if the logging works in AWS/OCI ?

Thanks

@driverpt
Copy link
Contributor Author

UPDATE: This seems to work in AWS Environment. But the downside is that we will need to add a reflect-config.json for all Logback components that we are using.

E.g.: In my case, the logstash-logback-encoder. But seems to be working with the simple Console Appender

@driverpt
Copy link
Contributor Author

@graemerocher @n0tl3ss , Checkstyle is failing due to NettyServer.java issue

@graemerocher graemerocher merged commit 214051e into micronaut-projects:3.8.x Dec 16, 2022
@graemerocher
Copy link
Contributor

Thanks for the contribution!

@driverpt driverpt deleted the logging-fix branch December 21, 2022 16:44
@driverpt driverpt restored the logging-fix branch December 21, 2022 16:44
@driverpt driverpt deleted the logging-fix branch December 21, 2022 16:44
@driverpt driverpt restored the logging-fix branch December 21, 2022 16:44
@driverpt driverpt deleted the logging-fix branch January 19, 2023 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: improvement A minor improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GRAALVM] Logback not fetching Environment Variable
3 participants