You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/01/16 15:00:05 UTC
[GitHub] [nifi] ChrisSamo632 opened a new pull request #4761: NIFI-7753 enable INFO logging for ExecuteGroovyScript by default
ChrisSamo632 opened a new pull request #4761:
URL: https://github.com/apache/nifi/pull/4761
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
Enable INFO logging for ExecuteGroovyScript by default
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] 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.
- [x] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [x] 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:
- [x] 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 JDK 8?~
- [x] Have you verified that the full build is successful on 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](http://www.apache.org/legal/resolved.html#category-a)?~
- ~[ ] 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 GitHub Actions CI for build issues and submit an update to your PR as soon as possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on pull request #4761: NIFI-7753 enable INFO logging for ExecuteGroovyScript by default
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on pull request #4761:
URL: https://github.com/apache/nifi/pull/4761#issuecomment-765905795
@mattyb149 realised I tagged the wrong person in my last comment
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on pull request #4761: NIFI-7753 enable INFO logging for ExecuteGroovyScript by default
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on pull request #4761:
URL: https://github.com/apache/nifi/pull/4761#issuecomment-763092626
> Split record indeed logs stuff like record counts at the INFO level, but the default logback.xml settings should suppress those like they would with any other processor. Are you seeing things logged at INFO level that should not be?
Looking again, you're quite right - `.processors` is set to WARN (I was probably getting confused while looking through the different package structures in the `nifi-scripting-processors`, my bad.
So yes, adding these changes would increase the `ScriptedTransformRecord` processor to output more by default than the "standard" Record processors. I agree that probably means that adding INFO for `ExecuteGroovyScript` by default starts to make less sense too. Maybe there should be an addition to the documentation for scripted components indicating that while they have the `log` component available, the default `logback.xml` config won't output anything below WARN? But I'll not do that as part of this Jira, it should probably be a separate one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mattyb149 commented on pull request #4761: NIFI-7753 enable INFO logging for ExecuteGroovyScript by default
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on pull request #4761:
URL: https://github.com/apache/nifi/pull/4761#issuecomment-763055762
Currently the only exceptions to the WARN level for processors are the ones that only log information (LogAttribute and LogMessage). Not sure why the scripting processors should default to INFO though. Is there something currently logged as INFO but should be WARN instead (i.e. any error/degraded conditions)?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mattyb149 commented on pull request #4761: NIFI-7753 enable INFO logging for ExecuteGroovyScript by default
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on pull request #4761:
URL: https://github.com/apache/nifi/pull/4761#issuecomment-763087937
Split record indeed logs stuff like record counts at the INFO level, but the default logback.xml settings should suppress those like they would with any other processor. Are you seeing things logged at INFO level that should not be?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on pull request #4761: NIFI-7753 enable INFO logging for ExecuteGroovyScript by default
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on pull request #4761:
URL: https://github.com/apache/nifi/pull/4761#issuecomment-781588348
> @ChrisSamo632 yeah i think i'd be in favor of simply documenting the fact that the default log level is set to WARN, and a simple explanation that the default log level used to be INFO and for many flows, just a single INFO-level log message per FlowFile per Processor resulted in tens or hundreds of GB of logs per day, and data provenance generally provides all of the same info as INFO-level logs but in a much more accessible way.
Makes sense. This PR would only enable INFO for the script components, but even then there could be flows where that spits out a lot of logs.
I'll close this PR and add a comment to the jira ticket, not sure where would be best to document the default logging levels (not something to do per-processor, so maybe the User Guide or Wiki)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on pull request #4761: NIFI-7753 enable INFO logging for ExecuteGroovyScript by default
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on pull request #4761:
URL: https://github.com/apache/nifi/pull/4761#issuecomment-763068680
> Currently the only exceptions to the WARN level for processors are the ones that only log information (LogAttribute and LogMessage). Not sure why the scripting processors should default to INFO though. Is there something currently logged as INFO but should be WARN instead (i.e. any error/degraded conditions)?
As described above, enabling INFO for the `ScriptedTransformRecord` would bring its defualt output more inline with `SplitRecord` (logging how many records have been transformed/dropped, etc.).
`ExecuteGroovyScript` (by default in the included NiFi code) only logs stuff at WARN/ERROR currently, so that's fair enough. Enabling INFO was something I raised because we included non-exception logging in some of our scripts then took a while to realise why INFO statements weren't being included in our logs.
If this isn't wanted behaviour, then fair enough, it can be left for individuals to enable INFO logging for the included processors that don't otherwise have it enabled by default (but the included logback.xml means some processors/controllers don't output INFO while others do - this PR doesn't necessarily rationalise all of them, but would for the scripted components at least).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] pvillard31 commented on pull request #4761: NIFI-7753 enable INFO logging for ExecuteGroovyScript by default
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on pull request #4761:
URL: https://github.com/apache/nifi/pull/4761#issuecomment-762655718
Yep makes sense.
@mattyb149 - any thoughts on this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on pull request #4761: NIFI-7753 enable INFO logging for ExecuteGroovyScript by default
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on pull request #4761:
URL: https://github.com/apache/nifi/pull/4761#issuecomment-763128157
@pvillard31 / @markap14 based on teh above discussion, should we reject/close this PR and Jira?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on pull request #4761: NIFI-7753 enable INFO logging for ExecuteGroovyScript by default
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on pull request #4761:
URL: https://github.com/apache/nifi/pull/4761#issuecomment-762640426
> If we're doing this, shouldn't we do it for all the "scripted" components?
I did wonder that myself after committing this change (I only really use the ExecuteGroovyScript, which is why I thought about the change). If it's thought worthwhile, then I'm happy to add more entries into the config for other scripted components.
My motivation for the change comes from using this processor and wanting to log some details when the component is triggered, but only realising after some time that it was logging config (within the docker container in my case) that needed to change in order to see any output that wasn't at warning/error levels. If the other scripted components don't include info logs in their base code already, then the same change might make sense
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 edited a comment on pull request #4761: NIFI-7753 enable INFO logging for ExecuteGroovyScript by default
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 edited a comment on pull request #4761:
URL: https://github.com/apache/nifi/pull/4761#issuecomment-763128157
@pvillard31 / @markap14 based on the above discussion, should we reject/close this PR and Jira?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on pull request #4761: NIFI-7753 enable INFO logging for ExecuteGroovyScript by default
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on pull request #4761:
URL: https://github.com/apache/nifi/pull/4761#issuecomment-762779571
Took a quick look at this and actually most of the Scripted processors/controllers/reporting tasks are already covered by the `org.apache.nifi` @ INFO entry in the `logback.xml` files.
The `org.apache.nifi.processors` @ WARN is what stopped the `ExecuteGroovyProcessor` from outputting @ INFO hence the first commit.
The only other processor I think that should probably have been outputting @ INFO but wouldn't by default is `org.apache.nifi.processors.script.ScriptedTransformRecord` - it contains an INFO log to report how many records it's transformed/dropped for a FlowFile, but this will be getting omitted from logs currently (unless someone updates their config). So I've added `org.apache.nifi.processors.script` @ INFO to capture this (and any other components in the same package, e.g. `ExecuteScript`).
N.B. the only `.info()` log I could see in the newly captured classes is the one mentioned above, which would bring `ScriptedTransformRecord` more inline with the default logging for other non-script record processors, e.g. `SplitRecord`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on pull request #4761: NIFI-7753 enable INFO logging for ExecuteGroovyScript by default
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #4761:
URL: https://github.com/apache/nifi/pull/4761#issuecomment-781552527
@ChrisSamo632 yeah i think i'd be in favor of simply documenting the fact that the default log level is set to WARN, and a simple explanation that the default log level used to be INFO and for many flows, just a single INFO-level log message per FlowFile per Processor resulted in tens or hundreds of GB of logs per day, and data provenance generally provides all of the same info as INFO-level logs but in a much more accessible way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] pvillard31 commented on pull request #4761: NIFI-7753 enable INFO logging for ExecuteGroovyScript by default
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on pull request #4761:
URL: https://github.com/apache/nifi/pull/4761#issuecomment-762636743
If we're doing this, shouldn't we do it for all the "scripted" components?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 closed pull request #4761: NIFI-7753 enable INFO logging for ExecuteGroovyScript by default
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 closed pull request #4761:
URL: https://github.com/apache/nifi/pull/4761
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org