-
Notifications
You must be signed in to change notification settings - Fork 2.9k
NIFI-15531 Repositioned log statements to indicate how long map extensions take. #10836
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
base: main
Are you sure you want to change the base?
Conversation
exceptionfactory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the proposing these additional logs @dan-s1.
On initial review, the new Started log does not seem necessary, it contains no details about the particular extension, and it is duplicative of existing debug logs.
The time calculation for each NAR is something to consider, but it would also produce a lot of additional info messages. This might be something to consider at the debug level.
Is there a particular reason you would like to time this operation?
|
This was added per a coworker's frustration as to what what is happening in that gap of time. |
Thanks @dan-s1. In that case, I'm not sure there is a worthwhile addition. As mentioned, the existing debug-level logging provides more information if desired. Simply writing more log messages to indicate additional detail is not necessarily the best path forward. If there is a problem with the behavior, that may be worth addressing. The fact that the NAR unpacking described on the Jira issue takes almost 30 seconds is notable, as it seems to indicate either slow decompression or write operations, among some possibilities. |
|
Adding 2 INFO log messages seems reasonable, if we can identify and describe what NiFi is doing. We do account for "Expanding NAR files" which took 4.873 seconds in the example below. What about the 4.052 seconds after that and before the 117 lines of "Loaded NAR file:"? What is NiFi doing? Maybe we can do one INFO message with "Loading 117 NAR files", change the 117 "Loaded NAR file" messages to the DEBUG level, and finish up with one INFO message "Loaded 117 NAR files in ??? seconds (??? ns)". For example: |
|
Or, as a completely different idea, move the "Expanded 117 NAR files in 4.873 seconds" INFO message farther down in the unpackNars() method, to include the calls to mapExtension()s and populateExtensionMapping() in the timer? |
exceptionfactory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dan-s1 Does the existing debug-level log for each NAR file meet the need described? As mentioned, I don't think it makes sense to introduce more verbose info-level logging for each NAR, and the current state of the PR logging the time spent mapping extensions does not seem needed.
|
The debug messages do not really address the issue and really are not necessary. The main concern was the hang time that occurs after the log message on line 205: My coworker did suggest perhaps pushing up lines 120-121 above line 119 after line 212 This does not add any extra logging just places the same logging a few lines later. |
|
Thanks @dan-s1, moving the existing logging lines as suggested sounds like a good plan, that way it encompasses the totality of the method. |
Summary
NIFI-15531
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000VerifiedstatusPull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation