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/03/30 18:14:39 UTC
[GitHub] [nifi] Lehel44 opened a new pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Lehel44 opened a new pull request #4952:
URL: https://github.com/apache/nifi/pull/4952
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
FlowFiles logged in logger Object[] param are now parsed and added to Bulletins.
**JIRA:** https://issues.apache.org/jira/browse/NIFI-8385
- Extended LogMessage with FlowFile attribute. Created LogMessage builder to avoid excessive method overloading in StandardLogRepository. Extended the classes with the new method implementing this interface: rules.handlers.MockComponentLog, util.MockComponentLog, MockComponentLogger, NopComponentLog, SimpleProcessLogger, TerminationAwareLogger.
- Extended LogRepository with new method to use LogMessage as parameter.
- In StandardLogRepository each method gets the flow file from the logger Object[] argument or from the LogMessage object.
- Created Bulletin builder to avoid excessive method overloading in BulletinFactory
- Removed Bulletin descendants: SystemBulletin, MockBulletin, ComponentBulletin
- Removed nifi-mock/src/main/java/org/apache/nifi/reporting/BulletinFactory
- Changed factory to builder in: MockReportingContext, StandardRemoteProcessGroup, BulletinAdapter, ControllerServiceLogObserver, ProcessorLogObserver, ReportingTaskLogObserver, TestNodeBulletinStrategy, StandardPublicPort, TestSiteToSiteBulletinReportingTask
- Extracted ID generation from BulletinFactory to BulletinIdProvider (used by Bulletin.Builder too)
- Moved LogMessage to nifi-api
- Code cleanup: ReportingContext, TestAlertHandler, ScriptedActionHandlerTest, SiteToSiteBulletinReportingTask
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] 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.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [ ] 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:
- [ ] 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?
- [ ] 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] markap14 commented on pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#issuecomment-953268279
Thanks for the updates @Lehel44 all looks good to me now. +1 will merge to main
--
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 change in pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
markap14 commented on a change in pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#discussion_r721564377
##########
File path: nifi-api/src/main/java/org/apache/nifi/logging/ComponentLog.java
##########
@@ -102,11 +117,128 @@
void debug(String msg);
- void log(LogLevel level, String msg, Throwable t);
-
- void log(LogLevel level, String msg, Object... os);
-
- void log(LogLevel level, String msg);
-
- void log(LogLevel level, String msg, Object[] os, Throwable t);
+ default void debug(LogMessage logMessage) {
+ logWithLogMessage(LogLevel.ERROR, logMessage);
+ }
+
+ default void log(LogLevel level, String msg, Throwable t) {
+ switch (level) {
+ case DEBUG:
+ debug(msg, t);
+ break;
+ case ERROR:
+ case FATAL:
+ error(msg, t);
+ break;
+ case INFO:
+ info(msg, t);
+ break;
+ case TRACE:
+ trace(msg, t);
+ break;
+ case WARN:
+ warn(msg, t);
+ break;
+ }
+ }
+
+ default void log(LogLevel level, String msg, Object... os) {
+ switch (level) {
+ case DEBUG:
+ debug(msg, os);
+ break;
+ case ERROR:
+ case FATAL:
+ error(msg, os);
+ break;
+ case INFO:
+ info(msg, os);
+ break;
+ case TRACE:
+ trace(msg, os);
+ break;
+ case WARN:
+ warn(msg, os);
+ break;
+ }
+ }
+
+ default void log(LogLevel level, String msg) {
+ switch (level) {
+ case DEBUG:
+ debug(msg);
+ break;
+ case ERROR:
+ case FATAL:
+ error(msg);
+ break;
+ case INFO:
+ info(msg);
+ break;
+ case TRACE:
+ trace(msg);
+ break;
+ case WARN:
+ warn(msg);
+ break;
+ }
+ }
+
+ default void log(LogLevel level, String msg, Object[] os, Throwable t) {
+ switch (level) {
+ case DEBUG:
+ debug(msg, os, t);
+ break;
+ case ERROR:
+ case FATAL:
+ error(msg, os, t);
+ break;
+ case INFO:
+ info(msg, os, t);
+ break;
+ case TRACE:
+ trace(msg, os, t);
+ break;
+ case WARN:
+ warn(msg, os, t);
+ break;
+ }
+ }
+
+ default void log(LogMessage message) {
+ switch (message.getLogLevel()) {
+ case DEBUG:
+ debug(message);
+ break;
+ case ERROR:
+ case FATAL:
+ error(message);
+ break;
+ case INFO:
+ info(message);
+ break;
+ case TRACE:
+ trace(message);
+ break;
+ case WARN:
+ warn(message);
+ break;
+ }
+ }
+
+ default void logWithLogMessage(LogLevel level, LogMessage logMessage) {
Review comment:
This should probably just be called `log`, rather than `logWithLogMessage`.
--
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 #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#issuecomment-894340947
Thanks @Lehel44. I took a look at this again. Somehow, it did not occur to be before that `Bulletin` is in nifi-api. This means we have to be very careful about what we do here. We must maintain backward compatibility.
So while I do think Bulletin should be immutable, we cannot make these changes until a 2.0 API. We can't remove methods like setters. So those changes need to be reverted. When these Bulletins are passed to a Reporting Task (I think that's the only extension point that has access to bulletins) it would probably make sense to wrap them in an `ImmutableBulletin` class that throws an Exception if any of the setters are called, because ReportingTasks are not intended to have the ability to modify existing Bulletins, only generate new ones.
Additionally, we should not be storing a reference to the FlowFile in the bulletin. The JIRA indicates that we want the FlowFile UUID, and that's fair game to store there. But not the FlowFile itself. The FlowFile can potentially take up a huge amount of heap space if it has really large attributes (or tons of attributes).
--
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] Lehel44 commented on a change in pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on a change in pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#discussion_r649466588
##########
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:
Yes, fixed it.
--
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 a change in pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
markap14 commented on a change in pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#discussion_r726516881
##########
File path: nifi-nar-bundles/nifi-site-to-site-reporting-bundle/nifi-site-to-site-reporting-task/src/main/java/org/apache/nifi/reporting/SiteToSiteBulletinReportingTask.java
##########
@@ -162,23 +161,21 @@ public void onTrigger(final ReportingContext context) {
final long transferMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start);
getLogger().info("Successfully sent {} Bulletins to destination in {} ms; Transaction ID = {}; First Event ID = {}",
- new Object[]{bulletins.size(), transferMillis, transactionId, bulletins.get(0).getId()});
- } catch (final Exception e) {
+ bulletins.size(), transferMillis, transactionId, bulletins.get(0).getId());
+ } catch (ProcessException e) {
+ throw e;
+ } catch (Exception e) {
if (transaction != null) {
transaction.error();
}
- if (e instanceof ProcessException) {
- throw (ProcessException) e;
- } else {
- throw new ProcessException("Failed to send Bulletins to destination due to IOException:" + e.getMessage(), e);
- }
+ throw new ProcessException("Failed to send Bulletins to destination due to IOException:" + e.getMessage(), e);
Review comment:
@Lehel44 the code was structured that way because if there is ever an Exception thrown, we must evaluate `if (transaction != null) { transaction.error(); }`. By refactoring this, we no longer evaluate that if `ProcessException` is thrown - only if any other Exception is thrown.
--
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 change in pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
markap14 commented on a change in pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#discussion_r726499496
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/logging/repository/StandardLogRepository.java
##########
@@ -46,43 +53,62 @@
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) {
+ 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<String> flowFileUuid = getFirstFlowFileUuidFromObjects(params);
final String formattedMessage = MessageFormatter.arrayFormat(format, params).getMessage();
- addLogMessage(level, formattedMessage);
+ final LogMessage logMessage = new LogMessage.Builder(System.currentTimeMillis(), level)
+ .message(formattedMessage)
+ .flowFileUuid(flowFileUuid.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<String> flowFileUuid = getFirstFlowFileUuidFromObjects(params);
final String formattedMessage = MessageFormatter.arrayFormat(format, params, t).getMessage();
- addLogMessage(level, formattedMessage, t);
+ final LogMessage logMessage = new LogMessage.Builder(System.currentTimeMillis(), level)
+ .message(formattedMessage)
+ .throwable(t)
+ .flowFileUuid(flowFileUuid.orElse(null))
+ .createLogMessage();
+ addLogMessage(logMessage);
+ }
+
+ private Optional<String> getFirstFlowFileUuidFromObjects(Object[] params) {
+ final List<FlowFile> flowFiles = Arrays.stream(params)
+ .filter(FlowFile.class::isInstance)
+ .map(FlowFile.class::cast)
+ .collect(Collectors.toList());
Review comment:
We need to avoid creation of a `Stream` here. `Stream` creation is very costly, and this is called for every log message. Should instead just loop over the array. Perhaps something like:
```
int flowFileCount = 0;
FlowFile flowFileFound = null;
for (final Object param : params) {
if (param instanceof FlowFile) {
if (++flowFileCount > 1) {
return Optional.empty();
}
flowFileFound = (FlowFile) param;
}
}
return Optional.ofNullable(flowFileFound).map(ff -> ff.getAttribute(CoreAttributes.UUID.key()));
```
##########
File path: nifi-nar-bundles/nifi-site-to-site-reporting-bundle/nifi-site-to-site-reporting-task/src/main/java/org/apache/nifi/reporting/SiteToSiteBulletinReportingTask.java
##########
@@ -162,23 +161,21 @@ public void onTrigger(final ReportingContext context) {
final long transferMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start);
getLogger().info("Successfully sent {} Bulletins to destination in {} ms; Transaction ID = {}; First Event ID = {}",
- new Object[]{bulletins.size(), transferMillis, transactionId, bulletins.get(0).getId()});
- } catch (final Exception e) {
+ bulletins.size(), transferMillis, transactionId, bulletins.get(0).getId());
+ } catch (ProcessException e) {
+ throw e;
+ } catch (Exception e) {
if (transaction != null) {
transaction.error();
}
- if (e instanceof ProcessException) {
- throw (ProcessException) e;
- } else {
- throw new ProcessException("Failed to send Bulletins to destination due to IOException:" + e.getMessage(), e);
- }
+ throw new ProcessException("Failed to send Bulletins to destination due to IOException:" + e.getMessage(), e);
Review comment:
This logic should not be changed. Previously, when any Exception was caught, it called `transaction.error()` (assuming transaction != null). With this change, that doesn't happen when `ProcessException` is thrown, but it needs to.
--
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] Lehel44 commented on pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#issuecomment-860720397
Okay, I'm not sure what exactly happened here... I will create a new PR soon...
--
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] Lehel44 removed a comment on pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
Lehel44 removed a comment on pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#issuecomment-860720397
Okay, I'm not sure what exactly happened here... I will create a new PR soon...
--
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] Lehel44 commented on a change in pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on a change in pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#discussion_r721624269
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-nar-utils/src/main/java/org/apache/nifi/mock/MockComponentLogger.java
##########
@@ -116,9 +114,8 @@ public void info(String msg) {
@Override
public void info(String msg, Object[] os, Throwable t) {
- logger.trace(msg, os);
- logger.trace("", t);
-
+ logger.info(msg, os);
+ logger.info("", t);
Review comment:
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] Lehel44 commented on pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#issuecomment-953268847
Great, thanks for the review!
--
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 #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#issuecomment-818743932
Thanks for the contribution @Lehel44 , will review.
--
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] Lehel44 commented on a change in pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on a change in pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#discussion_r726514279
##########
File path: nifi-nar-bundles/nifi-site-to-site-reporting-bundle/nifi-site-to-site-reporting-task/src/main/java/org/apache/nifi/reporting/SiteToSiteBulletinReportingTask.java
##########
@@ -162,23 +161,21 @@ public void onTrigger(final ReportingContext context) {
final long transferMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start);
getLogger().info("Successfully sent {} Bulletins to destination in {} ms; Transaction ID = {}; First Event ID = {}",
- new Object[]{bulletins.size(), transferMillis, transactionId, bulletins.get(0).getId()});
- } catch (final Exception e) {
+ bulletins.size(), transferMillis, transactionId, bulletins.get(0).getId());
+ } catch (ProcessException e) {
+ throw e;
+ } catch (Exception e) {
if (transaction != null) {
transaction.error();
}
- if (e instanceof ProcessException) {
- throw (ProcessException) e;
- } else {
- throw new ProcessException("Failed to send Bulletins to destination due to IOException:" + e.getMessage(), e);
- }
+ throw new ProcessException("Failed to send Bulletins to destination due to IOException:" + e.getMessage(), e);
Review comment:
I removed the positive branch of the if statement on line 170 because it appears to always be false. On line 165 all the ProcessExceptions will be caught already and will be thrown to the next caller method. Therefore the check on 170 for ProcessException to me seems unnecessary. Did I miss anything?
--
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] Lehel44 commented on pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#issuecomment-861289957
Hi @markap14
I got some problems with git, but finally managed to fix it. I fixed the review items and extended the Bulletin builder with the timestamp option to fix the timestamp override in BulletinAdapter.
--
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] Lehel44 commented on a change in pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on a change in pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#discussion_r726514475
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/logging/repository/StandardLogRepository.java
##########
@@ -46,43 +53,62 @@
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) {
+ 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<String> flowFileUuid = getFirstFlowFileUuidFromObjects(params);
final String formattedMessage = MessageFormatter.arrayFormat(format, params).getMessage();
- addLogMessage(level, formattedMessage);
+ final LogMessage logMessage = new LogMessage.Builder(System.currentTimeMillis(), level)
+ .message(formattedMessage)
+ .flowFileUuid(flowFileUuid.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<String> flowFileUuid = getFirstFlowFileUuidFromObjects(params);
final String formattedMessage = MessageFormatter.arrayFormat(format, params, t).getMessage();
- addLogMessage(level, formattedMessage, t);
+ final LogMessage logMessage = new LogMessage.Builder(System.currentTimeMillis(), level)
+ .message(formattedMessage)
+ .throwable(t)
+ .flowFileUuid(flowFileUuid.orElse(null))
+ .createLogMessage();
+ addLogMessage(logMessage);
+ }
+
+ private Optional<String> getFirstFlowFileUuidFromObjects(Object[] params) {
+ final List<FlowFile> flowFiles = Arrays.stream(params)
+ .filter(FlowFile.class::isInstance)
+ .map(FlowFile.class::cast)
+ .collect(Collectors.toList());
Review comment:
Thanks for the suggestion, it's a good point.
--
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 #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
markap14 merged pull request #4952:
URL: https://github.com/apache/nifi/pull/4952
--
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] Lehel44 commented on a change in pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on a change in pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#discussion_r726514279
##########
File path: nifi-nar-bundles/nifi-site-to-site-reporting-bundle/nifi-site-to-site-reporting-task/src/main/java/org/apache/nifi/reporting/SiteToSiteBulletinReportingTask.java
##########
@@ -162,23 +161,21 @@ public void onTrigger(final ReportingContext context) {
final long transferMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start);
getLogger().info("Successfully sent {} Bulletins to destination in {} ms; Transaction ID = {}; First Event ID = {}",
- new Object[]{bulletins.size(), transferMillis, transactionId, bulletins.get(0).getId()});
- } catch (final Exception e) {
+ bulletins.size(), transferMillis, transactionId, bulletins.get(0).getId());
+ } catch (ProcessException e) {
+ throw e;
+ } catch (Exception e) {
if (transaction != null) {
transaction.error();
}
- if (e instanceof ProcessException) {
- throw (ProcessException) e;
- } else {
- throw new ProcessException("Failed to send Bulletins to destination due to IOException:" + e.getMessage(), e);
- }
+ throw new ProcessException("Failed to send Bulletins to destination due to IOException:" + e.getMessage(), e);
Review comment:
I removed the positive branch of the if statement on line 170 because it appears to always be false. On line 165 all the ProcessExceptions will be caught already and will be thrown to the next caller method. Therefore the check on 170 for ProcessException to me seems unnecessary. Did I miss anything?
--
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] Lehel44 commented on pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#issuecomment-858954203
Thanks, @markap14 for the comments!
--
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 a change in pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
markap14 commented on a change in pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#discussion_r721563791
##########
File path: nifi-api/src/main/java/org/apache/nifi/logging/ComponentLog.java
##########
@@ -102,11 +117,128 @@
void debug(String msg);
- void log(LogLevel level, String msg, Throwable t);
-
- void log(LogLevel level, String msg, Object... os);
-
- void log(LogLevel level, String msg);
-
- void log(LogLevel level, String msg, Object[] os, Throwable t);
Review comment:
This is a public API so we cannot remove any methods.
--
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 change in pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
markap14 commented on a change in pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#discussion_r721573245
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/logging/repository/StandardLogRepository.java
##########
@@ -27,14 +29,16 @@
import java.util.ArrayList;
import java.util.Collection;
+import java.util.EnumMap;
import java.util.HashMap;
import java.util.Map;
+import java.util.Optional;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
public class StandardLogRepository implements LogRepository {
- private final Map<LogLevel, Collection<LogObserver>> observers = new HashMap<>();
+ private final EnumMap<LogLevel, Collection<LogObserver>> observers = new EnumMap<>(LogLevel.class);
Review comment:
Good idea to use an `EnumMap` here instead of a `HashMap`. Should use a declaration of `Map`, though.
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/logging/repository/StandardLogRepository.java
##########
@@ -46,43 +50,60 @@
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) {
+ 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<String> flowFileUUID = getFlowFileUUIDFromObjects(params);
final String formattedMessage = MessageFormatter.arrayFormat(format, params).getMessage();
- addLogMessage(level, formattedMessage);
+ final LogMessage logMessage = new LogMessage.Builder(System.currentTimeMillis(), level)
+ .message(formattedMessage)
+ .flowFileUUID(flowFileUUID.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<String> flowFileUUID = getFlowFileUUIDFromObjects(params);
final String formattedMessage = MessageFormatter.arrayFormat(format, params, t).getMessage();
- addLogMessage(level, formattedMessage, t);
+ final LogMessage logMessage = new LogMessage.Builder(System.currentTimeMillis(), level)
+ .message(formattedMessage)
+ .throwable(t)
+ .flowFileUUID(flowFileUUID.orElse(null))
+ .createLogMessage();
+ addLogMessage(logMessage);
+ }
+
+ private Optional<String> getFlowFileUUIDFromObjects(Object[] params) {
Review comment:
Might make sense to name this method `getFirstFlowFileUuidFromObjects` for clarity - it's definitely possible that there could be multiple FlowFile objects. Or, it might make more sense to return `Optional.empty()` if there's more than one FlowFile, as we really don't know which FlowFile the log message is really focused on. For example, the log message `Created {} from {} but failed to send {}` may take 3 FlowFiles are arguments. The first one would be the most important FlowFile there. But for a message like `Created 3 FlowFiles from {} but {} could not be sent`, the most important FlowFile for the log message is the second. Can't really know which is the main theme of the log message.
##########
File path: nifi-nar-bundles/nifi-site-to-site-reporting-bundle/nifi-site-to-site-reporting-task/src/main/java/org/apache/nifi/reporting/SiteToSiteBulletinReportingTask.java
##########
@@ -195,6 +192,7 @@ private JsonObject serialize(final JsonBuilderFactory factory, final JsonObjectB
addField(builder, "bulletinSourceName", bulletin.getSourceName(), allowNullValues);
addField(builder, "bulletinSourceType", bulletin.getSourceType() == null ? null : bulletin.getSourceType().name(), allowNullValues);
addField(builder, "bulletinTimestamp", df.format(bulletin.getTimestamp()), allowNullValues);
+ addField(builder, "bulletinFlowFileUUID", bulletin.getFlowFileUUID(), allowNullValues);
Review comment:
Should probably also change this to `bulletinFlowFileUuid`
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/events/BulletinFactory.java
##########
@@ -27,30 +27,28 @@
private static final AtomicLong currentId = new AtomicLong(0);
+ private BulletinFactory() {
+ }
+
public static Bulletin createBulletin(final Connectable connectable, final String category, final String severity, final String message) {
final ComponentType type;
Review comment:
Is a good idea to break that into its own method - would make sense then to also collapse the declaration and assignment into a single statement as well now. (I.e., `final ComponentType type = getComponentType(connectable);`)
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-nar-utils/src/main/java/org/apache/nifi/mock/MockComponentLogger.java
##########
@@ -116,9 +114,8 @@ public void info(String msg) {
@Override
public void info(String msg, Object[] os, Throwable t) {
- logger.trace(msg, os);
- logger.trace("", t);
-
+ logger.info(msg, os);
+ logger.info("", t);
Review comment:
Wow. That's a good catch :)
--
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 change in pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [nifi] Lehel44 commented on pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#issuecomment-810473431
I will squash the commits after fixing review items.
--
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] Lehel44 commented on a change in pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on a change in pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#discussion_r721643760
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/logging/repository/StandardLogRepository.java
##########
@@ -46,43 +50,60 @@
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) {
+ 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<String> flowFileUUID = getFlowFileUUIDFromObjects(params);
final String formattedMessage = MessageFormatter.arrayFormat(format, params).getMessage();
- addLogMessage(level, formattedMessage);
+ final LogMessage logMessage = new LogMessage.Builder(System.currentTimeMillis(), level)
+ .message(formattedMessage)
+ .flowFileUUID(flowFileUUID.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<String> flowFileUUID = getFlowFileUUIDFromObjects(params);
final String formattedMessage = MessageFormatter.arrayFormat(format, params, t).getMessage();
- addLogMessage(level, formattedMessage, t);
+ final LogMessage logMessage = new LogMessage.Builder(System.currentTimeMillis(), level)
+ .message(formattedMessage)
+ .throwable(t)
+ .flowFileUUID(flowFileUUID.orElse(null))
+ .createLogMessage();
+ addLogMessage(logMessage);
+ }
+
+ private Optional<String> getFlowFileUUIDFromObjects(Object[] params) {
Review comment:
That's a good idea!
--
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 change in pull request #4952: NIFI-8385: Add FlowFiles from logging to bulletins
Posted by GitBox <gi...@apache.org>.
markap14 commented on a change in pull request #4952:
URL: https://github.com/apache/nifi/pull/4952#discussion_r721568610
##########
File path: nifi-api/src/main/java/org/apache/nifi/reporting/Bulletin.java
##########
@@ -131,6 +132,14 @@ public void setSourceType(ComponentType sourceType) {
this.sourceType = sourceType;
}
+ public String getFlowFileUUID() {
+ return flowFileUUID;
+ }
+
+ public void setFlowFileUUID(String flowFileUUID) {
+ this.flowFileUUID = flowFileUUID;
+ }
Review comment:
Java naming conventions are generally accepted to use mixed case with acronyms (i.e., getFlowFileUuid(), setFlowFileUuid()). This is definitely the pattern we tend to follow in NiFi. Probably best to use that naming convention here.
--
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