exceptionfactory commented on code in PR #9014: URL: https://github.com/apache/nifi/pull/9014#discussion_r1657102139
########## nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/reporting/PerformanceMetricsUtil.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.reporting; + +import org.apache.nifi.controller.ProcessorNode; +import org.apache.nifi.controller.repository.FlowFileEvent; +import org.apache.nifi.controller.status.PerformanceMetrics; + +public class PerformanceMetricsUtil { + + public static PerformanceMetrics getPerformanceMetrics(final FlowFileEvent aggregatedEvent, final FlowFileEvent fileEvent, final ProcessorNode processorNode) { + long totalCpuNanos = aggregatedEvent.getCpuNanoseconds(); + long totalReadNanos = aggregatedEvent.getContentReadNanoseconds(); + long totalWriteNanos = aggregatedEvent.getContentWriteNanoseconds(); + long totalSessionCommitNanos = aggregatedEvent.getSessionCommitNanoseconds(); + long totalBytesRead = aggregatedEvent.getBytesRead(); + long totalBytesWritten = aggregatedEvent.getBytesWritten(); + long totalGcNanos = aggregatedEvent.getGargeCollectionMillis(); + + final PerformanceMetrics newMetrics = new PerformanceMetrics(); + + newMetrics.setIdentifier(processorNode.getProcessGroup().getIdentifier()); + newMetrics.setCpuTime(fileEvent.getCpuNanoseconds()); + newMetrics.setCpuTimePercentage(nanosToPercent(fileEvent.getCpuNanoseconds(), totalCpuNanos)); + newMetrics.setReadTime(fileEvent.getContentReadNanoseconds()); + newMetrics.setReadTimePercentage(nanosToPercent(fileEvent.getContentReadNanoseconds(), totalReadNanos)); + newMetrics.setWriteTime(fileEvent.getContentWriteNanoseconds()); + newMetrics.setWriteTimePercentage(nanosToPercent(fileEvent.getContentWriteNanoseconds(), totalWriteNanos)); + newMetrics.setCommitTime(fileEvent.getSessionCommitNanoseconds()); + newMetrics.setCommitTimePercentage(nanosToPercent(fileEvent.getSessionCommitNanoseconds(), totalSessionCommitNanos)); + newMetrics.setGcTime(fileEvent.getGargeCollectionMillis()); + newMetrics.setGcTimePercentage(nanosToPercent(fileEvent.getGargeCollectionMillis(), totalGcNanos)); + newMetrics.setBytesRead(fileEvent.getBytesRead()); + newMetrics.setBytesReadPercentage(nanosToPercent(fileEvent.getBytesRead(), totalBytesRead)); + newMetrics.setBytesWritten(fileEvent.getBytesWritten()); + newMetrics.setBytesWrittenPercentage(nanosToPercent(fileEvent.getBytesWritten(), totalBytesWritten)); + + return newMetrics; + } + + private static long nanosToPercent(final long a, final long b) { Review Comment: This method should be renamed since it sometimes takes nanos and sometimes takes bytes. Recommend something generic like `getPercent()`: ```suggestion private static long getPercent(final long actual, final long total) { ``` ########## nifi-api/src/main/java/org/apache/nifi/controller/status/PerformanceMetrics.java: ########## @@ -0,0 +1,234 @@ +/* + * 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.controller.status; + +public class PerformanceMetrics implements Cloneable { + + private String identifier; + private long cpuTime; + private long cpuTimePercentage; + private long readTime; + private long readTimePercentage; + private long writeTime; + private long writeTimePercentage; + private long commitTime; + private long commitTimePercentage; + private long gcTime; + private long gcTimePercentage; + private long bytesRead; + private long bytesReadPercentage; + private long bytesWritten; + private long bytesWrittenPercentage; + + public String getIdentifier() { + return identifier; + } + + public void setIdentifier(String identifier) { + this.identifier = identifier; + } + + public long getCpuTime() { + return cpuTime; + } + + public void setCpuTime(long cpuTime) { + this.cpuTime = cpuTime; + } + + public long getCpuTimePercentage() { + return cpuTimePercentage; + } + + public void setCpuTimePercentage(long cpuTimePercentage) { + this.cpuTimePercentage = cpuTimePercentage; + } + + public long getReadTime() { + return readTime; + } + + public void setReadTime(long readTime) { + this.readTime = readTime; + } + + public long getReadTimePercentage() { + return readTimePercentage; + } + + public void setReadTimePercentage(long readTimePercentage) { + this.readTimePercentage = readTimePercentage; + } + + public long getWriteTime() { + return writeTime; + } + + public void setWriteTime(long writeTime) { + this.writeTime = writeTime; + } + + public long getWriteTimePercentage() { + return writeTimePercentage; + } + + public void setWriteTimePercentage(long writeTimePercentage) { + this.writeTimePercentage = writeTimePercentage; + } + + public long getCommitTime() { + return commitTime; + } + + public void setCommitTime(long commitTime) { + this.commitTime = commitTime; + } + + public long getCommitTimePercentage() { + return commitTimePercentage; + } + + public void setCommitTimePercentage(long commitTimePercentage) { + this.commitTimePercentage = commitTimePercentage; + } + + public long getGcTime() { + return gcTime; + } + + public void setGcTime(long gcTime) { + this.gcTime = gcTime; + } + + public long getGcTimePercentage() { + return gcTimePercentage; + } + + public void setGcTimePercentage(long gcTimePercentage) { + this.gcTimePercentage = gcTimePercentage; + } + + public long getBytesRead() { + return bytesRead; + } + + public void setBytesRead(long bytesRead) { + this.bytesRead = bytesRead; + } + + public long getBytesReadPercentage() { + return bytesReadPercentage; + } + + public void setBytesReadPercentage(long bytesReadPercentage) { + this.bytesReadPercentage = bytesReadPercentage; + } + + public long getBytesWritten() { + return bytesWritten; + } + + public void setBytesWritten(long bytesWritten) { + this.bytesWritten = bytesWritten; + } + + public long getBytesWrittenPercentage() { + return bytesWrittenPercentage; + } + + public void setBytesWrittenPercentage(long bytesWrittenPercentage) { + this.bytesWrittenPercentage = bytesWrittenPercentage; + } + + public static void merge(final PerformanceMetrics target, final PerformanceMetrics toMerge) { + if (target == null || toMerge == null) { + return; + } + + target.setIdentifier(toMerge.getIdentifier()); + target.setCpuTime(target.getCpuTime() + toMerge.getCpuTime()); + target.setCpuTimePercentage(Math.min(100L, target.getCpuTimePercentage() + toMerge.getCpuTimePercentage())); Review Comment: Recommend adding a `getPercent()` method to encapsulate this Math calculation. ########## nifi-api/src/main/java/org/apache/nifi/controller/status/PerformanceMetrics.java: ########## @@ -0,0 +1,234 @@ +/* + * 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.controller.status; + +public class PerformanceMetrics implements Cloneable { + + private String identifier; + private long cpuTime; + private long cpuTimePercentage; + private long readTime; + private long readTimePercentage; Review Comment: Calculating all of these percent values could become expensive for flows thousands of Processors. In the interest of making this information available it seems like it would be better to avoid this percentage calculation entirely. ########## nifi-extension-bundles/nifi-sql-reporting-bundle/nifi-sql-reporting-tasks/src/main/java/org/apache/nifi/reporting/sql/datasources/ProcessorStatusDataSource.java: ########## @@ -53,7 +53,19 @@ public class ProcessorStatusDataSource implements ResettableDataSource { new ColumnSchema("invocations", int.class, false), new ColumnSchema("processingNanos", long.class, false), new ColumnSchema("runStatus", String.class, false), - new ColumnSchema("executionNode", String.class, false) + new ColumnSchema("executionNode", String.class, false), + new ColumnSchema("cpuTime", long.class, false), + new ColumnSchema("cpuOverallPct", long.class, false), Review Comment: Recommend spelling out `Percent` instead of using `Pct`: ```suggestion new ColumnSchema("cpuOverallPercent", long.class, false), ``` ########## nifi-api/src/main/java/org/apache/nifi/controller/status/PerformanceMetrics.java: ########## @@ -0,0 +1,234 @@ +/* + * 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.controller.status; + +public class PerformanceMetrics implements Cloneable { + + private String identifier; + private long cpuTime; + private long cpuTimePercentage; Review Comment: Recommend shortening this and others to `Percent`: ```suggestion private long cpuTimePercent; ``` ########## nifi-api/src/main/java/org/apache/nifi/controller/status/PerformanceMetrics.java: ########## @@ -0,0 +1,234 @@ +/* + * 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.controller.status; + +public class PerformanceMetrics implements Cloneable { Review Comment: The general pattern seems to be using `Status` in the class. It is also important to reflect that this relates to processing, so at the risk of making this longer, I recommend naming this `ProcessingPerformanceStatus`. ```suggestion public class ProcessingPerformanceStatus implements Cloneable { ``` ########## nifi-api/src/main/java/org/apache/nifi/controller/status/PerformanceMetrics.java: ########## @@ -0,0 +1,234 @@ +/* + * 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.controller.status; + +public class PerformanceMetrics implements Cloneable { + + private String identifier; + private long cpuTime; + private long cpuTimePercentage; + private long readTime; + private long readTimePercentage; + private long writeTime; + private long writeTimePercentage; + private long commitTime; + private long commitTimePercentage; + private long gcTime; + private long gcTimePercentage; + private long bytesRead; + private long bytesReadPercentage; + private long bytesWritten; + private long bytesWrittenPercentage; + + public String getIdentifier() { + return identifier; + } + + public void setIdentifier(String identifier) { + this.identifier = identifier; + } + + public long getCpuTime() { + return cpuTime; + } + + public void setCpuTime(long cpuTime) { + this.cpuTime = cpuTime; + } + + public long getCpuTimePercentage() { + return cpuTimePercentage; + } + + public void setCpuTimePercentage(long cpuTimePercentage) { + this.cpuTimePercentage = cpuTimePercentage; + } + + public long getReadTime() { + return readTime; + } + + public void setReadTime(long readTime) { + this.readTime = readTime; + } + + public long getReadTimePercentage() { + return readTimePercentage; + } + + public void setReadTimePercentage(long readTimePercentage) { + this.readTimePercentage = readTimePercentage; + } + + public long getWriteTime() { + return writeTime; + } + + public void setWriteTime(long writeTime) { + this.writeTime = writeTime; + } + + public long getWriteTimePercentage() { + return writeTimePercentage; + } + + public void setWriteTimePercentage(long writeTimePercentage) { + this.writeTimePercentage = writeTimePercentage; + } + + public long getCommitTime() { + return commitTime; + } + + public void setCommitTime(long commitTime) { + this.commitTime = commitTime; + } + + public long getCommitTimePercentage() { + return commitTimePercentage; + } + + public void setCommitTimePercentage(long commitTimePercentage) { + this.commitTimePercentage = commitTimePercentage; + } + + public long getGcTime() { + return gcTime; + } + + public void setGcTime(long gcTime) { + this.gcTime = gcTime; + } + + public long getGcTimePercentage() { + return gcTimePercentage; + } + + public void setGcTimePercentage(long gcTimePercentage) { + this.gcTimePercentage = gcTimePercentage; + } + + public long getBytesRead() { + return bytesRead; + } + + public void setBytesRead(long bytesRead) { + this.bytesRead = bytesRead; + } + + public long getBytesReadPercentage() { + return bytesReadPercentage; + } + + public void setBytesReadPercentage(long bytesReadPercentage) { + this.bytesReadPercentage = bytesReadPercentage; + } + + public long getBytesWritten() { + return bytesWritten; + } + + public void setBytesWritten(long bytesWritten) { + this.bytesWritten = bytesWritten; + } + + public long getBytesWrittenPercentage() { + return bytesWrittenPercentage; + } + + public void setBytesWrittenPercentage(long bytesWrittenPercentage) { + this.bytesWrittenPercentage = bytesWrittenPercentage; + } + + public static void merge(final PerformanceMetrics target, final PerformanceMetrics toMerge) { Review Comment: This should not be implemented in the class. Instead, it should be implemented in a separate `Merger` implementation inside the framework. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
