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

Improve logging in the alarm_logger #2083

Merged
merged 5 commits into from
Dec 14, 2021

Conversation

mgotz
Copy link
Contributor

@mgotz mgotz commented Dec 7, 2021

In configuring the alarm-logger at our site I stumbled a few times with getting logging configured the desired way, the amount of error and warning log messages, despite seemingly correct configuration, and non-working options in the alarm_logger.properties file.
This pull request tries to address these problems I had in the hope future users might have an easier start:

  • It adds a log4j2 to slf4j adapter to make log4j2 logging use the slf4j API, which then uses JUL for logging (previously log4j2 showed an error and was using some simple logger fallback)
  • Only the properties relevant for kafka are passed to the kafka-stream eliminating most of the "isn't a known config" warnings. (Remaining warnings are an internal problem of the kafka library).
  • Split alarm_logging.properties into an application.properties and alarm_logging.properties file:
    • application.properties contains properties for springboot. These must be loaded before the application context and thus can't be loaded through @PropertySource. This is necessary for the logging and the banner=off settings to have any effect. Those settings can't be changed at runtime.
    • alarm_properties.properties contains the settings for the actual alarm_logger. These can be overwritten with command line arguments or by providing another file with -properties. Now the alarm_logger.properties can be used as a template for providing your own file with the -properties argument and all settings therein should have an effect.
  • Use the alarm_logger_logging.properties file as the default logging configuration. That way there is a clear starting point for creating one's own logging configuration.

While stomping out error message I came accross the SearchController in the rest directory. It attempts to load preferences from alarm_logging_service.properties, which doesn't exist, creating an error message. The SearchController seems to wrap the elastic search REST API in its own REST API. I don't think it is used anywhere in Phoebus. Could it simply be removed?

mgotz added 4 commits December 3, 2021 16:08
split alarm_logger.properties in two files, one for spring boot and one for phoebus
use alarm_logger_logging.properties as default for logging
remove unused settings from properties files
@shroffk
Copy link
Member

shroffk commented Dec 7, 2021

The REST api is a place holder so I think we want to keep it.
I will add the missing properties files

@shroffk shroffk requested a review from georgweiss December 7, 2021 15:12
Copy link
Collaborator

@georgweiss georgweiss left a comment

Choose a reason for hiding this comment

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

My only comment would be that logging should maybe be a discussion topic for the entire code base. In the Phoebus application I think we at this point are consistently using the "native" java.util.logging API. Maybe we should consider using the same strategy for services to make things a bit more consistent and life easier for developers. Not sure how such a strategy works for Spring-based services, but I would be prepared to investigate in the context of the save-and-restore service.

@mgotz
Copy link
Contributor Author

mgotz commented Dec 8, 2021

Thank you for the quick answers.
Regarding the logging consistency, I had that same goald here: The alarm_logger code uses the java.util.logging API a.k.a. JUL. Then I redirected the logging from other libraries to JUL. Log4j2 gets redirected to the SLF4J API (log4j-to-slf4j) and SLF4J binds to JUL (slf4j-jdk14). Maybe there is better way to redirect that, but I think with those modules added developers don't need to worry about the other APIs anymore.

@georgweiss
Copy link
Collaborator

I'm a bit on thin ice here, but your change to redirect to JUL is maybe all we need.
Again, I'll spend some time later to revisit how things are logged in the save&restore service.

I'm fine approving this.

@mgotz
Copy link
Contributor Author

mgotz commented Dec 14, 2021

I am new to this process, so is there anything I still need to do to have this merged? I think the PR is still missing formal approval, however, I don't see anything I could do.

@georgweiss
Copy link
Collaborator

@mgotz, can you update this PR with latest changes on the main branch? Seems we both have removed duplicates in pom files.
I will approve once that is done and I've verified locally.

@mgotz
Copy link
Contributor Author

mgotz commented Dec 14, 2021

Sure, I merged the master back in.

@georgweiss georgweiss merged commit cc6d5bb into ControlSystemStudio:master Dec 14, 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 this pull request may close these issues.

3 participants