NIFI-6628: Separate out logging of extensions vs. nifi framework

[adarmiento]

Description of PR

Updated main and test logback.xml

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

• Is there a JIRA ticket associated with this PR? Is it referenced
  in the commit message?

• Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.

• Has your PR been rebased against the latest commit within the target branch (typically master)?

• Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

For code changes:

• Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
• Have you written or updated unit tests to verify your changes?
• Have you verified that the full build is successful on both JDK 8 and JDK 11?
• If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
• If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
• If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
• If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

• Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

[adarmiento]

I am not sure about the semantic:
The new root appender is nifi-framework,
nifi-processors, LogMessage, and LogAttribute, use the old nifi-app appender instead.

I am investigating the other logging stuff (groovy and upgrade files in nifi-toolkit) which I think should be updated as well

[markap14]

@adarmiento Thanks for the PR! I went back & forth in my mind over what made sense here as far as configuring different elements. This PR goes the route of making the framework file the default, and then selectively choosing what goes into the app-log. But I think we have to go the other way. The app-log should be default, and then be selective about which packages we put into the framework log. This is because we processors/controller services/etc. do not just belong to the package "org.apache.nifi.processors". We have some that are just in "org.apache.nifi", some in "org.apache.nifi.json", etc., etc. And users can have their own custom processors, in a package of say "com.mycompany" and we don't want that going to the framework log. So we will need the default to remain the app-log and then select specific packages to put into the framework.log file. Does that make sense?

[adarmiento]

Hello @markap14
It sure does make sense, thanks for the clarification!

[adarmiento]

@markap14 as a quick job, I just reverted APP_FILE to be the main log file, and this time I selected the FRAMEWORK_FILE for all the currently existing non-app related loggers:

<logger name="org.apache.nifi" level="INFO">...</logger>
<logger name="org.apache.zookeeper.ClientCnxn" level="ERROR">...</logger>
<logger name="org.apache.zookeeper.server.NIOServerCnxn" level="ERROR">...</logger>
<logger name="org.apache.zookeeper.server.NIOServerCnxnFactory" level="ERROR">...</logger>
<logger name="org.apache.zookeeper.server.quorum" level="ERROR">...</logger>
<logger name="org.apache.zookeeper.ZooKeeper" level="ERROR">...</logger>
<logger name="org.apache.zookeeper.server.PrepRequestProcessor" level="ERROR">...</logger>
<logger name="org.apache.calcite.runtime.CalciteException" level="OFF">...</logger>
<logger name="org.apache.curator.framework.recipes.leader.LeaderSelector" level="OFF">...</logger>
<logger name="org.apache.curator.ConnectionState" level="OFF">...</logger>
<logger name="org.apache.nifi.cluster" level="INFO">...</logger>
<logger name="org.apache.nifi.server.JettyServer" level="INFO">...</logger>
<logger name="org.eclipse.jetty" level="INFO">...</logger>
<logger name="org.springframework" level="ERROR">...</logger>
<logger name="org.glassfish.jersey.internal.Errors" level="ERROR">...</logger>
<logger name="org.eclipse.jetty.annotations.AnnotationParser" level="ERROR">...</logger>

As you mentioned, there are Processors and Controller Services widespread among various packages which may not be encompassed by those existing loggers. Only editing those loggers would make those processors log lines end up in the FRAMEWORK.

Do you think it will be the correct choice to fathom the codebase in search of those Processor/Services and creating additional dedicated loggers in logback.xml file?

[markap14]

Thanks @adarmiento . I think this is getting closer. It looks like the latest commit puts org.apache.nifi into the framework, log, though, which I think is still not what we want, because we have some processors, services, etc. that even are in that package. For example, GetHTMLElement, ModifyHTMLElement, and PutHTMLElement are all in the org.apache.nifi package. I think we have to be even more specific. I would look at the nifi-nar-bundles/nifi-framework-bundle/nifi-framework module and its sub-modules. Especially nifi-framework-core, nifi-framework-core-api, nifi-framework-clustering and pick out any specific packages in those. For example, the org.apache.nifi.controller package contains a lot of the framework specific classes.

[adarmiento]

Hello @markap14, and sorry for my late reply.

I started looking at the nifi-nar-bundles/nifi-framework-bundle/nifi-framework, right now, I added some loggers for the packages in the nifi-framework-core module.
Being all the packages in the form of org.apache.nifi.[something] what I've done in this commit seems a really verbose and error-prone solution.
What do you think about it? Should I keep diving into packages looking for framework related ones, or could there be a less verbose way?
Thank you!

[adarmiento]

I've added further framework cases for other modules in the nar-bundle/framework-bundle

[markap14]

Thanks for the update! Unfortunately, I think this is going to be pretty verbose... it probably makes a lot of sense to insert a new package into the framework code and put things under org.apache.nifi.framework but that would touch hundreds if not thousands of classes, so it's probably better to do that in the next major release.

I think there are a few loggers included here that should go to app-log instead of framework-log (calcite for example). And some loggers should be in nifi-user rather than framework or app. And we really only need to update the main logback.xml in nifi-resources, not all of the toolkit ones. Let me put together another commit that moves a couple of the loggers around and we'll see where that gets us.

[adarmiento]

Hello @markap14, I've evaluated the middle package strategy too, but I immediately discarded because as you mentioned would be a massive refactoring.

Thanks for the tips, it's not an easy job discern for every package whether or not it's a framework or app feature.
I'll start by applying your suggestions, let me know what do you think!

[markap14]

OK things look much better there. I found a few duplicates and a couple of packages that we should add. Also updated the time to keep the framework log to 7 days by default, gzipped. And updated all of the loggers to set additivity="false" - otherwise, we get the logs in both nifi-framework and nifi-app. I'm not entirely sure the best way to build on top of your PR. So I'll just attach a .patch file and you can apply the patch file and update the PR, if you think it all makes sense. Then I'll merge the whole thing into master.

[markap14]

OK, I uploaded a .patch file to the Jira (https://issues.apache.org/jira/secure/attachment/12981600/0001-NIFI-6628-Updates-to-logback-to-ensure-that-app-log-.patch). Please do check it out and if you're happy push the changes to your PR and ping me so that I can merge it.

[adarmiento]

Hello @markap14, thanks for the patch!
I think it makes sense to edit the format and keep-time since we are also introducing the logger.
Since we are saving up to 7 days of logs, should we also raise the 100MB size limit?

I also added a little cleanup and reorder for better readability.

[adarmiento]

Hello @markap14
Some time has passed, could you please take a look again at this, maybe it's ready

[joewitt]

closing due to inactivity