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/06/09 21:04:38 UTC
[GitHub] [nifi] markap14 commented on a change in pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
markap14 commented on a change in pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#discussion_r648612574
##########
File path: nifi-api/src/main/java/org/apache/nifi/logging/LogMessage.java
##########
@@ -26,18 +28,66 @@
public class LogMessage {
private final String message;
- private final LogLevel level;
+ private final LogLevel logLevel;
private final Throwable throwable;
- private final long time;
+ private final FlowFile flowFile;
+ private final Object[] objects;
+ private long time;
Review comment:
Why is this timestamp being made mutable? A LogMessage should not be a mutable object.
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/logging/repository/StandardLogRepository.java
##########
@@ -48,43 +49,57 @@
private volatile ComponentLog componentLogger;
@Override
- public void addLogMessage(final LogLevel level, final String message) {
- addLogMessage(level, message, (Throwable) null);
- }
-
- @Override
- public void addLogMessage(final LogLevel level, final String message, final Throwable t) {
- final LogMessage logMessage = new LogMessage(System.currentTimeMillis(), level, message, t);
+ public void addLogMessage(LogMessage logMessage) {
+ logMessage.setTime(System.currentTimeMillis());
+ LogLevel logLevel = logMessage.getLogLevel();
- final Collection<LogObserver> logObservers = observers.get(level);
+ final Collection<LogObserver> logObservers = observers.get(logLevel);
if (logObservers != null) {
for (LogObserver observer : logObservers) {
try {
observer.onLogMessage(logMessage);
- } catch (final Throwable observerThrowable) {
+ } catch (final Exception observerThrowable) {
logger.error("Failed to pass log message to Observer {} due to {}", observer, observerThrowable.toString());
}
}
}
+
}
@Override
public void addLogMessage(final LogLevel level, final String format, final Object[] params) {
replaceThrowablesWithMessage(params);
+ final Optional<FlowFile> flowFile = getFlowFileFromObjects(params);
final String formattedMessage = MessageFormatter.arrayFormat(format, params).getMessage();
- addLogMessage(level, formattedMessage);
+ final LogMessage logMessage = new LogMessage.Builder()
+ .setLevel(level)
+ .setMessage(formattedMessage)
+ .setFlowFile(flowFile.orElse(null))
+ .createLogMessage();
+ addLogMessage(logMessage);
}
@Override
public void addLogMessage(final LogLevel level, final String format, final Object[] params, final Throwable t) {
replaceThrowablesWithMessage(params);
+ final Optional<FlowFile> flowFile = getFlowFileFromObjects(params);
final String formattedMessage = MessageFormatter.arrayFormat(format, params, t).getMessage();
- addLogMessage(level, formattedMessage, t);
+ final LogMessage logMessage = new LogMessage.Builder()
+ .setLevel(level)
+ .setMessage(formattedMessage)
+ .setThrowable(t)
+ .setFlowFile(flowFile.orElse(null))
+ .createLogMessage();
+ addLogMessage(logMessage);
+ }
+
+ private Optional<FlowFile> getFlowFileFromObjects(Object[] params) {
+ return Arrays.stream(params).filter(s -> s instanceof FlowFile).map(f -> (FlowFile) f).findFirst();
Review comment:
We need to avoid using Streams here. The creation of the Stream is very expensive and should not be used in performance-critical sections of code.
##########
File path: nifi-api/src/main/java/org/apache/nifi/logging/LogMessage.java
##########
@@ -26,18 +28,66 @@
public class LogMessage {
private final String message;
- private final LogLevel level;
+ private final LogLevel logLevel;
private final Throwable throwable;
- private final long time;
+ private final FlowFile flowFile;
+ private final Object[] objects;
+ private long time;
public static final String DATE_TIME_FORMAT = "yyyy-MM-dd HH:mm:ss.SSS";
public static final String TO_STRING_FORMAT = "%1$s %2$s - %3$s";
- public LogMessage(final long millisSinceEpoch, final LogLevel level, final String message, final Throwable throwable) {
- this.level = level;
+ public static class Builder {
+
+ private long millisSinceEpoch;
+ private LogLevel level;
+ private String message;
+ private Throwable throwable;
+ private FlowFile flowFile;
+ private Object[] objects;
+
+ public Builder setMillisSinceEpoch(long millisSinceEpoch) {
Review comment:
Similar to the comment in the other Builder, the pattern that we usually use in the NiFi codebase is to omit the `set` prefix on Builder methods and instead go with `.level(...).message(...).timestamp(...)` etc.
##########
File path: nifi-api/src/main/java/org/apache/nifi/reporting/Bulletin.java
##########
@@ -16,119 +16,180 @@
*/
package org.apache.nifi.reporting;
+import org.apache.nifi.flowfile.FlowFile;
+
import java.util.Date;
/**
* A Bulletin is a construct that represents a message that is to be displayed
* to users to notify of a specific (usually fleeting) event.
*/
-public abstract class Bulletin implements Comparable<Bulletin> {
+public class Bulletin implements Comparable<Bulletin> {
private final Date timestamp;
private final long id;
- private String nodeAddress;
- private String level;
- private String category;
- private String message;
+ private final String nodeAddress;
+ private final String level;
+ private final String category;
+ private final String message;
+
+ private final String groupId;
+ private final String groupName;
+ private final String groupPath;
+ private final String sourceId;
+ private final String sourceName;
+ private final ComponentType sourceType;
+ private final FlowFile flowFile;
+
+ private Bulletin(Builder builder) {
+ timestamp = builder.timestamp;
+ id = builder.id;
+ nodeAddress = builder.nodeAddress;
+ level = builder.level;
+ category = builder.category;
+ message = builder.message;
+
+ groupId = builder.groupId;
+ groupName = builder.groupName;
+ groupPath = builder.groupPath;
+ sourceId = builder.sourceId;
+ sourceName = builder.sourceName;
+ sourceType = builder.sourceType;
+ flowFile = builder.flowFile;
+ }
+
+ public static class Builder {
+
+ private final Date timestamp;
+ private final long id;
+ private String nodeAddress;
+ private String level;
+ private String category;
+ private String message;
+
+ private String groupId;
+ private String groupName;
+ private String groupPath;
+ private String sourceId;
+ private String sourceName;
+ private ComponentType sourceType;
+ private FlowFile flowFile;
+
+ public Builder() {
+ this.id = BulletinIdProvider.getUniqueId();
Review comment:
Not sure there's really a need for this separate, statically defined class. Instead, I'd recommend just having a `private static final AtomicLong` defined in the builder and then having the calling `getAndIncrement()` in the call to `build()` and providing that to the `Bulletin` constructor. This would be a little cleaner. It also avoids a problem that exists with the current pattern: as-is, if an instance of Builder is reused, each Bulletin will have the same ID. For example:
```
final Bulletin.Builder builder = new Bulletin.Builder();
builder.level(...).category(...).message(....);
final Bulletin b1 = builder.build();
builder.message(...);
final Bulletin b2 = builder.build();
// b1 and b2 have now both been constructed with the same ID
```
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-nar-utils/src/main/java/org/apache/nifi/mock/MockComponentLogger.java
##########
@@ -118,7 +131,23 @@ public void info(String msg) {
public void info(String msg, Object[] os, Throwable t) {
logger.trace(msg, os);
logger.trace("", t);
+ }
+ @Override
+ public void info(LogMessage logMessage) {
Review comment:
I see this implementation for info/warn/error/etc. in 3 places. And it simply delegates to other methods. Probably makes sense to move these up as default methods in the interface.
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/TerminationAwareLogger.java
##########
@@ -271,6 +340,11 @@ public void debug(String msg) {
logger.debug(msg);
}
+ @Override
+ public void debug(LogMessage message) {
+
Review comment:
Did you overlook this method?
##########
File path: nifi-nar-bundles/nifi-rules-action-handler-bundle/nifi-rules-action-handler-service/src/test/java/org/apache/nifi/rules/handlers/MockComponentLog.java
##########
@@ -207,6 +233,33 @@ public void log(LogLevel level, String msg, Object[] os, Throwable t) {
}
+ @Override
+ public void log(LogMessage msg) {
Review comment:
Probably makes sense to move this up as a default method as well
##########
File path: nifi-api/src/main/java/org/apache/nifi/logging/LogMessage.java
##########
@@ -26,18 +28,66 @@
public class LogMessage {
private final String message;
- private final LogLevel level;
+ private final LogLevel logLevel;
private final Throwable throwable;
- private final long time;
+ private final FlowFile flowFile;
+ private final Object[] objects;
+ private long time;
public static final String DATE_TIME_FORMAT = "yyyy-MM-dd HH:mm:ss.SSS";
public static final String TO_STRING_FORMAT = "%1$s %2$s - %3$s";
- public LogMessage(final long millisSinceEpoch, final LogLevel level, final String message, final Throwable throwable) {
- this.level = level;
+ public static class Builder {
+
+ private long millisSinceEpoch;
+ private LogLevel level;
+ private String message;
+ private Throwable throwable;
+ private FlowFile flowFile;
+ private Object[] objects;
+
+ public Builder setMillisSinceEpoch(long millisSinceEpoch) {
+ this.millisSinceEpoch = millisSinceEpoch;
+ return this;
+ }
+
+ public Builder setLevel(LogLevel level) {
+ this.level = level;
+ return this;
+ }
+
+ public Builder setMessage(String message) {
+ this.message = message;
+ return this;
+ }
+
+ public Builder setThrowable(Throwable throwable) {
+ this.throwable = throwable;
+ return this;
+ }
+
+ public Builder setFlowFile(FlowFile flowFile) {
+ this.flowFile = flowFile;
+ return this;
+ }
+
+ public Builder setObjectsFile(Object[] objects) {
Review comment:
`setObjectsFile` - guessing that was a copy/paste mistake?
##########
File path: nifi-api/src/main/java/org/apache/nifi/reporting/Bulletin.java
##########
@@ -16,119 +16,180 @@
*/
package org.apache.nifi.reporting;
+import org.apache.nifi.flowfile.FlowFile;
+
import java.util.Date;
/**
* A Bulletin is a construct that represents a message that is to be displayed
* to users to notify of a specific (usually fleeting) event.
*/
-public abstract class Bulletin implements Comparable<Bulletin> {
+public class Bulletin implements Comparable<Bulletin> {
private final Date timestamp;
private final long id;
- private String nodeAddress;
- private String level;
- private String category;
- private String message;
+ private final String nodeAddress;
+ private final String level;
+ private final String category;
+ private final String message;
+
+ private final String groupId;
+ private final String groupName;
+ private final String groupPath;
+ private final String sourceId;
+ private final String sourceName;
+ private final ComponentType sourceType;
+ private final FlowFile flowFile;
+
+ private Bulletin(Builder builder) {
+ timestamp = builder.timestamp;
+ id = builder.id;
+ nodeAddress = builder.nodeAddress;
+ level = builder.level;
+ category = builder.category;
+ message = builder.message;
+
+ groupId = builder.groupId;
+ groupName = builder.groupName;
+ groupPath = builder.groupPath;
+ sourceId = builder.sourceId;
+ sourceName = builder.sourceName;
+ sourceType = builder.sourceType;
+ flowFile = builder.flowFile;
+ }
+
+ public static class Builder {
+
+ private final Date timestamp;
+ private final long id;
+ private String nodeAddress;
+ private String level;
+ private String category;
+ private String message;
+
+ private String groupId;
+ private String groupName;
+ private String groupPath;
+ private String sourceId;
+ private String sourceName;
+ private ComponentType sourceType;
+ private FlowFile flowFile;
+
+ public Builder() {
+ this.id = BulletinIdProvider.getUniqueId();
+ timestamp = new Date();
+ }
- private String groupId;
- private String groupName;
- private String groupPath;
- private String sourceId;
- private String sourceName;
- private ComponentType sourceType;
+ public Builder setNodeAddress(String nodeAddress) {
+ this.nodeAddress = nodeAddress;
+ return this;
+ }
- protected Bulletin(final long id) {
- this.timestamp = new Date();
- this.id = id;
- }
+ public Builder setLevel(String level) {
+ this.level = level;
+ return this;
+ }
- public String getNodeAddress() {
- return nodeAddress;
- }
+ public Builder setCategory(String category) {
+ this.category = category;
+ return this;
+ }
- public void setNodeAddress(String nodeAddress) {
- this.nodeAddress = nodeAddress;
- }
+ public Builder setMessage(String message) {
Review comment:
A bit of a nit-pick but generally the pattern that we use for Builders in the NiFi codebase is to not use setXYZ() and instead use xyz()
--
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