You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "timeabarna (via GitHub)" <gi...@apache.org> on 2023/05/31 10:48:40 UTC

[GitHub] [nifi] timeabarna opened a new pull request, #7315: NIFI-3065 Per Process Group logging

timeabarna opened a new pull request, #7315:
URL: https://github.com/apache/nifi/pull/7315

   # Summary
   Add a toggle in Process Group configuration (default: off), that when turned on would generate a dedicated log file for the process group.
   
   The nifi-app.log file would not be changed and would still contain all of the data.
   
   The nifi_app_pg_<log file suffix>.log file would have the same properties as nifi-app.log file (roll over, size, etc) and would contain the logs generated by the components included in that process group.
   
   [NIFI-3065](https://issues.apache.org/jira/browse/NIFI-3065)
   
   # Tracking
   
   Please complete the following tracking steps prior to pull request creation.
   
   ### Issue Tracking
   
   - [ ] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
   
   ### Pull Request Tracking
   
   - [ ] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
   - [ ] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
   
   ### Pull Request Formatting
   
   - [ ] Pull Request based on current revision of the `main` branch
   - [ ] Pull Request refers to a feature branch with one commit containing changes
   
   # Verification
   
   Please indicate the verification steps performed prior to pull request creation.
   
   ### Build
   
   - [ ] Build completed using `mvn clean install -P contrib-check`
     - [ ] JDK 11
     - [ ] JDK 17
   
   ### Licensing
   
   - [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
   - [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
   
   ### Documentation
   
   - [ ] Documentation formatting appears as expected in rendered files
   


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 merged pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 merged PR #7315:
URL: https://github.com/apache/nifi/pull/7315


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1239039993


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/process-group-configuration.jsp:
##########
@@ -99,6 +99,17 @@
                             <span id="read-only-process-group-default-back-pressure-data-size-threshold" class="unset"></span>
                         </div>
                     </div>
+                    <div class="setting">
+                        <div class="setting-name">Log File Suffix
+                        <div class="fa fa-question-circle" alt="Info" title="Turns on dedicated logging. When left empty log messages will be logged only to the primary app log. When filled out messages logged by components in this group will be sent to the standard app log, as well as a separate log file, with the provided name, specific to this group."></div>

Review Comment:
   Rather than "When filled out messages logged..." should maybe say "When set, messages logged..."



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] turcsanyip commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "turcsanyip (via GitHub)" <gi...@apache.org>.
turcsanyip commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1212320791


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/processor/SimpleProcessLogger.java:
##########
@@ -559,4 +564,33 @@ private Throwable findLastThrowable(final Object[] arguments) {
         }
         return lastThrowable;
     }
+
+    private String getDiscriminatorKey() {
+        return loggingContext.getDiscriminatorKey();
+    }
+
+    private String getLogFileSuffix() {
+        return loggingContext.getLogFileSuffix().orElse(null);
+    }
+
+    private void log(final Level level, final String message, final Object argument) {
+        log(level, message, argument, null);
+    }
+
+    private void log(final Level level, final String message, final Object argument, final Throwable throwable) {
+        if (throwable == null) {
+            logger.makeLoggingEventBuilder(level)
+                    .setMessage(message)
+                    .addArgument(argument)
+                    .addKeyValue(getDiscriminatorKey(), getLogFileSuffix())
+                    .log();

Review Comment:
   @timeabarna Thanks for implementing the PG level logging!
   
   Without reviewing the whole feature, I would just like to add some comments on `SimpleProcessLogger` log methods.
   
   It seems `LoggingEventBuilder.addArgument()` cannot receive multiple arguments (in our case `Object argument` is an array with multiple arguments). As a result, the log messages are not rendered properly: the whole argument array is replaced in the first `{}` placeholder, while the remaining `{}` placeholders stay unresolved:
   ```
   2023-05-31 23:08:39,074 ERROR [Timer-Driven Process Thread-7] o.a.n.p.helloworld.HelloWorldProcessor [HelloWorldProcessor[id=01881001-eeed-1379-569e-d0e8dd187a7d], foo, bar] Fatal error, arg1={}, arg2={}
   ```
   `addArgument()` could be called in a loop for each item in the array but I think there is a simpler solution: using vararg parameter. The `Throwable` does not need to be handled separately either, it can be the last item in the vararg. The underlying logging framework already handles it.
   
   So the method would simply look like this:
   ```
       private void log(final Level level, final String message, final Object... arguments) {
           logger.makeLoggingEventBuilder(level)
                   .addKeyValue(getDiscriminatorKey(), getLogFileSuffix())
                   .log(message, arguments);
       }
   ```



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1229697680


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/logging/PerProcessGroupLoggable.java:
##########
@@ -0,0 +1,23 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.logging;
+
+import org.apache.nifi.groups.ProcessGroup;
+
+public interface PerProcessGroupLoggable {

Review Comment:
   Perhaps `GroupedComponent` or `LoggingComponent`? The name `PerProcessGroupLoggable` is very specific and describes the use case in which it's currently being used. It does not describe really what the Object is.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1213211136


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/logging/StandardLoggingContext.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.logging;
+
+import org.apache.nifi.groups.ProcessGroup;
+
+import java.util.Optional;
+import java.util.concurrent.atomic.AtomicReference;
+
+public class StandardLoggingContext<T extends PerProcessGroupLoggable> implements LoggingContext {
+    private static final String KEY = "logFileSuffix";
+    private final AtomicReference<T> component = new AtomicReference<>();

Review Comment:
   There's no need for an `AtomicReference` here, just a `volatile` should be sufficient.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1229678647


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -272,6 +274,25 @@
                     $('#process-group-default-back-pressure-object-threshold').removeClass('unset').val(processGroup.defaultBackPressureObjectThreshold);
                     $('#process-group-default-back-pressure-data-size-threshold').removeClass('unset').val(processGroup.defaultBackPressureDataSizeThreshold);
 
+                    $('#process-group-log-to-own-file-combo').removeClass('unset').combo({

Review Comment:
   We should use True / False here, rather than Yes / No, to be consistent with other areas of the application.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] turcsanyip commented on pull request #7315: NIFI-3065 Per Process Group logging

Posted by "turcsanyip (via GitHub)" <gi...@apache.org>.
turcsanyip commented on PR #7315:
URL: https://github.com/apache/nifi/pull/7315#issuecomment-1571982073

   Thanks @timeabarna! `SimpleProcessLogger` now renders the log messages as before.


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1213199524


##########
nifi-api/src/main/java/org/apache/nifi/flow/VersionedProcessGroup.java:
##########
@@ -47,6 +47,9 @@ public class VersionedProcessGroup extends VersionedComponent {
     private Long defaultBackPressureObjectThreshold;
     private String defaultBackPressureDataSizeThreshold;
 
+    private Boolean logToOwnFile;

Review Comment:
   I don't think we need this Boolean, do we? The presence or the absence of the log file suffix should be sufficient to determine 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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] pvillard31 commented on pull request #7315: NIFI-3065 Per Process Group logging

Posted by "pvillard31 (via GitHub)" <gi...@apache.org>.
pvillard31 commented on PR #7315:
URL: https://github.com/apache/nifi/pull/7315#issuecomment-1584177624

   Hey @aszkowski - I'm obviously not speaking in the name of the Apache NiFi community, but this pull request should be merged fairly soon as this is under active review. However it's very likely that this change will only be merged on the main branch (which means this change will be for the upcoming NiFi 2.0) and not the support branch for 1.x. In terms of release, the community is making great progress around the NiFi 2.0 goals and I think we can expect a first release of NiFi 2.0 in the next 2-3 months.


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] timeabarna commented on pull request #7315: NIFI-3065 Per Process Group logging

Posted by "timeabarna (via GitHub)" <gi...@apache.org>.
timeabarna commented on PR #7315:
URL: https://github.com/apache/nifi/pull/7315#issuecomment-1592917330

   Hello @markap14,
   
   Thanks for your review, I've modified the PR based on your recommendations.


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1213207373


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/groups/StandardProcessGroup.java:
##########
@@ -214,6 +214,9 @@ public final class StandardProcessGroup implements ProcessGroup {
     private static final long DEFAULT_BACKPRESSURE_OBJECT = 10_000L;
     private static final String DEFAULT_BACKPRESSURE_DATA_SIZE = "1 GB";
 
+    private Boolean logToOwnFile = Boolean.FALSE;

Review Comment:
   Again no need for the boolean.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] timeabarna commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "timeabarna (via GitHub)" <gi...@apache.org>.
timeabarna commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1219197176


##########
nifi-api/src/main/java/org/apache/nifi/flow/VersionedProcessGroup.java:
##########
@@ -47,6 +47,9 @@ public class VersionedProcessGroup extends VersionedComponent {
     private Long defaultBackPressureObjectThreshold;
     private String defaultBackPressureDataSizeThreshold;
 
+    private Boolean logToOwnFile;

Review Comment:
   Thanks @markap14 for your review.
   For better User experience I've separated the enabling and configuration. There is a combolist with 'Yes' and 'No' options to turn dedicated logging and there is an input field with Process Group name as a default value for the log file name. I think this way the UI is more intuitive and more convenient than typing/deleting log file suffix.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1229795456


##########
nifi-registry/nifi-registry-core/nifi-registry-flow-diff/src/main/java/org/apache/nifi/registry/flow/diff/StandardFlowComparator.java:
##########
@@ -522,6 +522,8 @@ private void compare(final VersionedProcessGroup groupA, final VersionedProcessG
         addIfDifferent(differences, DifferenceType.DEFAULT_BACKPRESSURE_OBJECT_COUNT_CHANGED, groupA, groupB, VersionedProcessGroup::getDefaultBackPressureObjectThreshold, true, 10_000L);
         addIfDifferent(differences, DifferenceType.DEFAULT_FLOWFILE_EXPIRATION_CHANGED, groupA, groupB, VersionedProcessGroup::getDefaultFlowFileExpiration, true, "0 sec");
         addIfDifferent(differences, DifferenceType.PARAMETER_CONTEXT_CHANGED, groupA, groupB, VersionedProcessGroup::getParameterContextName, true, null);
+        addIfDifferent(differences, DifferenceType.LOG_TO_OWN_FILE_CHANGED, groupA, groupB, VersionedProcessGroup::isLogToOwnFile, true, Boolean.FALSE);
+        addIfDifferent(differences, DifferenceType.LOG_FILE_SUFFIX_CHANGED, groupA, groupB, VersionedProcessGroup::getLogFileSuffix, true, groupA.getName());

Review Comment:
   While we do want to capture these as Flow Differences, they really are environmental. So these should be added to the `FlowDifferenceFilters.isEnvironmentalChange` method



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1213212094


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/logging/StandardLoggingContext.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.logging;
+
+import org.apache.nifi.groups.ProcessGroup;
+
+import java.util.Optional;
+import java.util.concurrent.atomic.AtomicReference;
+
+public class StandardLoggingContext<T extends PerProcessGroupLoggable> implements LoggingContext {

Review Comment:
   Is there a reason this needs to be generic? We should instead just reference the component as a `PerProcessGroupLoggable`. 



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1213208436


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/groups/StandardProcessGroup.java:
##########
@@ -243,6 +246,7 @@ public StandardProcessGroup(final String id, final ControllerServiceProvider ser
         this.defaultFlowFileExpiration = new AtomicReference<>();
         this.defaultBackPressureObjectThreshold = new AtomicReference<>();
         this.defaultBackPressureDataSizeThreshold = new AtomicReference<>();
+        this.logFileSuffix = getName();

Review Comment:
   We should not default to creating a separate log file per PG. We should default to this being `null`.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1213204117


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-client-dto/src/main/java/org/apache/nifi/web/api/dto/ProcessGroupDTO.java:
##########
@@ -39,6 +39,8 @@ public class ProcessGroupDTO extends ComponentDTO {
     private String defaultFlowFileExpiration;
     private Long defaultBackPressureObjectThreshold;
     private String defaultBackPressureDataSizeThreshold;
+    private Boolean logToOwnFile;

Review Comment:
   As with VersionedProcessGroup, I don't think this is necessary. The presence/absence of the suffix should be sufficient for determining 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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] pvillard31 commented on pull request #7315: NIFI-3065 Per Process Group logging

Posted by "pvillard31 (via GitHub)" <gi...@apache.org>.
pvillard31 commented on PR #7315:
URL: https://github.com/apache/nifi/pull/7315#issuecomment-1584179043

   As an additional note - this change is bringing a very nice feature to NiFi so any help to build the change and give this a try and share feedback is more than appreciated!


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1229680948


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -272,6 +274,25 @@
                     $('#process-group-default-back-pressure-object-threshold').removeClass('unset').val(processGroup.defaultBackPressureObjectThreshold);
                     $('#process-group-default-back-pressure-data-size-threshold').removeClass('unset').val(processGroup.defaultBackPressureDataSizeThreshold);
 
+                    $('#process-group-log-to-own-file-combo').removeClass('unset').combo({
+                        options: [{
+                            text: 'Yes',
+                            value: true,
+                            description: 'Logging will be generated to dedicated log file for the process group.'

Review Comment:
   "Messages logged by components in this group will be sent to the standard app log, as well as a separate log file specific to this group."?



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1229690788


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/groups/StandardProcessGroup.java:
##########
@@ -4422,6 +4426,26 @@ public QueueSize getQueueSize() {
         return new QueueSize(count, contentSize);
     }
 
+    @Override
+    public Boolean isLogToOwnFile() {
+        return logToOwnFile;
+    }
+
+    @Override
+    public void setLogToOwnFile(final Boolean logToOwnFile) {
+        this.logToOwnFile = logToOwnFile;
+    }
+
+    @Override
+    public String getLogFileSuffix() {
+        return logFileSuffix;
+    }
+
+    @Override
+    public void setLogFileSuffix(final String logFileSuffix) {
+        this.logFileSuffix = logFileSuffix;

Review Comment:
   We should disallow any characters that are not filename friendly. For example, we should not allow spaces, slashes (`/`) or backslashes (`\`)



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] aszkowski commented on pull request #7315: NIFI-3065 Per Process Group logging

Posted by "aszkowski (via GitHub)" <gi...@apache.org>.
aszkowski commented on PR #7315:
URL: https://github.com/apache/nifi/pull/7315#issuecomment-1583273718

   Hello, I don't know if this is the right place to comment this, but I couldn't find anywhere else to ask but my team and I are currently working on implementing a similar cluster monitoring system within our codebase however we have run into some issues. I understand that you have created this PR recently, but do you have an estimated ETA (Like months or years?) on how long you think this will hit production. Let me know if there's a better place to reach you/ask this. Thank you.


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1229692228


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/logging/LoggingContext.java:
##########
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.logging;
+
+import java.util.Optional;
+
+public interface LoggingContext {
+    Optional<String> getLogFileSuffix();
+    String getDiscriminatorKey();

Review Comment:
   Please add some javadocs explaining what these are



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1229679296


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -272,6 +274,25 @@
                     $('#process-group-default-back-pressure-object-threshold').removeClass('unset').val(processGroup.defaultBackPressureObjectThreshold);
                     $('#process-group-default-back-pressure-data-size-threshold').removeClass('unset').val(processGroup.defaultBackPressureDataSizeThreshold);
 
+                    $('#process-group-log-to-own-file-combo').removeClass('unset').combo({
+                        options: [{
+                            text: 'Yes',
+                            value: true,
+                            description: 'Logging will be generated to dedicated log file for the process group.'
+                        }, {
+                            text: 'No',
+                            value: false,
+                            description: 'Normal logging will be followed.'

Review Comment:
   "Log messages will be logged only to the primary app log"?



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1239041303


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/groups/StandardProcessGroup.java:
##########
@@ -214,6 +214,8 @@ public final class StandardProcessGroup implements ProcessGroup {
     private static final String DEFAULT_FLOWFILE_EXPIRATION = "0 sec";
     private static final long DEFAULT_BACKPRESSURE_OBJECT = 10_000L;
     private static final String DEFAULT_BACKPRESSURE_DATA_SIZE = "1 GB";
+    private static final String VALID_DIRECTORY_NAME_REGEX = "[\\s\\<\\>:\\'\\\"\\/\\\\\\|\\?\\*]";

Review Comment:
   I don't think `VALID_DIRECTORY_NAME_REGEX` is really what this is - it is more an `INVALID_DIRECTORY_NAME_CHARACTERS`. Also, the `Pattern` class is thread-safe so we should instead use:
   ```
   private static final Pattern INVALID_DIRECTORY_CHARACTER_REGEX = Pattern.compile("...");
   ```
   instead of calling `Pattern.compile` every time it's used.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1229681996


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -316,6 +337,23 @@
                         $('#read-only-process-group-default-flowfile-expiration').text(processGroup.defaultFlowFileExpiration);
                         $('#read-only-process-group-default-back-pressure-object-threshold').text(processGroup.defaultBackPressureObjectThreshold);
                         $('#read-only-process-group-default-back-pressure-data-size-threshold').text(processGroup.defaultBackPressureDataSizeThreshold);
+
+                        // Determine the user-friendly name for the selected Log To Own File
+                        var selectedLogToOwnFile;
+                        if (processGroup.logToOwnFile == true) {
+                            selectedLogToOwnFile = "Yes";
+                        } else {
+                            selectedLogToOwnFile = "No";
+                        }

Review Comment:
   We should use True/False, not Yes/No. This keeps us consistent with the rest of the app.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1229687291


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/process-group-configuration.jsp:
##########
@@ -99,6 +99,24 @@
                             <span id="read-only-process-group-default-back-pressure-data-size-threshold" class="unset"></span>
                         </div>
                     </div>
+                    <div class="setting">
+                         <div class="setting-name">Log to own file?</div>
+                         <div class="editable setting-field">
+                            <div id="process-group-log-to-own-file-combo"></div>
+                         </div>
+                         <div class="read-only setting-field">
+                            <span id="read-only-process-group-log-to-own-file" class="unset"></span>
+                         </div>
+                    </div>
+                    <div class="setting">
+                        <div class="setting-name">Log File Suffix</div>

Review Comment:
   We should have a tooltip explaining what this is.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1229686669


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/process-group-configuration.jsp:
##########
@@ -99,6 +99,24 @@
                             <span id="read-only-process-group-default-back-pressure-data-size-threshold" class="unset"></span>
                         </div>
                     </div>
+                    <div class="setting">
+                         <div class="setting-name">Log to own file?</div>

Review Comment:
   We should avoid using a question mark in the title. To clarify, maybe consider "Log to Dedicated File"



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on PR #7315:
URL: https://github.com/apache/nifi/pull/7315#issuecomment-1604967178

   Thanks @timeabarna I did some additional testing, etc. and all looks good to me now. I'm a +1. Looks like Github Actions hasn't yet run. So i kicked that off. Assuming that it's green, I should be able to merge today. Thanks!


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on PR #7315:
URL: https://github.com/apache/nifi/pull/7315#issuecomment-1603329607

   Thanks @timeabarna I think we're really close! Have been testing this, and everything is working well for the most part. I'll leave a couple of minor comments inline, but also I noticed an issue with regards to the synchronization. I created a group and set the Log File Suffix to "Main" and saved it to the registry a Version 1. I then made some other changes to the flow, and I changed the Log File Suffix to "Main22" and saved that as V2.
   
   Now, I can change the Log File Suffix to "SomeOtherFile" and change version to V1. This changed the Log File Suffix back to "Main". If I then change it again to "SomeOtherFile" and change to V2, it again gets overwritten to "Main22". So in the StandardFlowSynchronizer, we need to ensure that we don't overwrite the value if it's already been set.


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] timeabarna commented on pull request #7315: NIFI-3065 Per Process Group logging

Posted by "timeabarna (via GitHub)" <gi...@apache.org>.
timeabarna commented on PR #7315:
URL: https://github.com/apache/nifi/pull/7315#issuecomment-1604370894

   Thanks @markap14 for your recommendations, I've updated the PR accordingly.


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1213205285


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-client-dto/src/main/java/org/apache/nifi/web/api/entity/ProcessGroupEntity.java:
##########
@@ -56,6 +56,9 @@ public class ProcessGroupEntity extends ComponentEntity implements Permissible<P
 
     private ParameterContextReferenceEntity parameterContext;
 
+    private Boolean logToOwnFile;
+    private String logFileSuffix;

Review Comment:
   Is there a reason these are necessary at the Entity level? Having them at the DTO level should be sufficient.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] markap14 commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1213207734


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/groups/StandardProcessGroup.java:
##########
@@ -214,6 +214,9 @@ public final class StandardProcessGroup implements ProcessGroup {
     private static final long DEFAULT_BACKPRESSURE_OBJECT = 10_000L;
     private static final String DEFAULT_BACKPRESSURE_DATA_SIZE = "1 GB";
 
+    private Boolean logToOwnFile = Boolean.FALSE;
+    private String logFileSuffix;

Review Comment:
   StandardProcessGroup needs to be thread-safe, so this must be `volatile`



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] timeabarna commented on pull request #7315: NIFI-3065 Per Process Group logging

Posted by "timeabarna (via GitHub)" <gi...@apache.org>.
timeabarna commented on PR #7315:
URL: https://github.com/apache/nifi/pull/7315#issuecomment-1571921956

   Thanks @turcsanyip for your review, I've corrected SimpleProcessLogger.


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org