Don't write full stack trace to system console on exceptions while trying to send messages to IoT Hub by JMayrbaeurl · Pull Request #44 · Azure/azure-iot-sdk-java

JMayrbaeurl · 2017-03-10T10:48:02Z

Checklist

I have read the [contribution guidelines] (https://github.com/Azure/azure-iot-sdk-java/blob/master/.github/CONTRIBUTING.md).
I added or modified the existing tests to cover the change (we do not allow our test coverage to go down). No modification necessary
If this is a modification that impacts the behavior of a public API
- I edited the corresponding document in the devdoc folder and added or modified requirements.
I submitted this PR against the correct branch:
- This pull-request is submitted against the master branch.

Reference/Link to the issue solved with this PR (if any)

Description of the problem

Reported by Partner Bosch Rexroth: If there are exceptions on trying to send messages to IoT Hub with class IotHubSendTask, full stack traces will be written to the console. This causes problems on devices with few memory and disk space (when forwarding the console output to log files)

Description of the solution

Introduced new boolean class property 'verboseErrorMessaging' that defaults to false and is used in catch clause to determine if full stack traces should be written to the system console. This means that the default behavior of the class has been changed to NOT write full stack traces on exceptions.

msftclas · 2017-03-10T10:48:07Z

@JMayrbaeurl,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

msftclas · 2017-03-10T11:00:16Z

@JMayrbaeurl, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

prmathur-microsoft · 2017-03-14T23:43:19Z

+	            for (StackTraceElement el : e.getStackTrace())
+	            {
+	            	// JMayrbaeurl 2017-03-10: Should be replaced with real logging setup
+	                System.out.println(el);


I think we have logger use in this module and would recommend using logger here instead of println. So that interested users can debug it using log.
And if we go the route of logger you woudn't really need verboseErrMessaging anymore. By default you can always write fatal error message in this case.

prmathur-microsoft · 2017-03-14T23:44:22Z

Thanks for your PR. Can you please address my comment and then we pull it in.

Merged changes from Release 2017-03-10

Merged latest release changes

added configuration for stack trace writting

…sdk-java.git into bosch-dev

JMayrbaeurl · 2017-03-15T16:44:56Z

Addressed your comment. Added slf4j logging to the classes that send and receive messages to and from the IoT Hub. Full stack traces are only written to the log when using debug level on logging

prmathur-microsoft · 2017-03-17T00:37:30Z

+        <dependency>
+   			<groupId>org.slf4j</groupId>
+ 			<artifactId>slf4j-api</artifactId>
+    	</dependency>


Thank you for proving a sample. But for now we will investigate this option and add support to better logging in future. As of now if we let this change in then only part of the SDK will be supporting this method of logging and rest will still be dependent on log4j. So I will hold off on this change.

This is no breaking change and I really don't like to make changes that introduce an old error once again. Transitioning to logging facades is no enhancement, but a bug fix and has to be done asap. It's probably better to change the CustomLogger or even better completely remove it/exchange it by using slf4j logging facades. This can be done in an hour! If you don't accept the PR this way, I will close it and will open an according issue for it.

Sounds good. If you are willing to provide a PR then please do so. I will be happy to review it.
Just to let you know we expect the following from the PR-

Please remove any occurrences of log4J from requirement docs (https://github.com/Azure/azure-iot-sdk-java/tree/master/device/iot-device-client/devdoc/requirement_docs/com/microsoft/azure/iothub), unit tests(https://github.com/Azure/azure-iot-sdk-java/tree/master/device/iot-device-client/src/test/java/tests/unit/com/microsoft/azure/sdk/iot/device) and code.

Provide an implementation, requirements and unit tests for new logging method for the entire SDK and replace the occurrence of log4j logger with your approach.

Update github documents (https://github.com/Azure/azure-iot-sdk-java/tree/master/device/iot-device-samples#logging) and our send-receive sample (https://github.com/Azure/azure-iot-sdk-java/tree/master/device/iot-device-samples/send-receive-sample) with proper instructions for logging.

If you wish to it can be a separate PR as well.

I have done this in a separate PR #57 . I tried to follow the three steps described above as good as possible.

prmathur-microsoft · 2017-03-17T00:40:18Z

+     * Private logger for class
+     */
+    final Logger logger = LoggerFactory.getLogger(IotHubReceiveTask.class);
+


Can this be changed to the logger (CustomLogger) in use currently until we address proper logging method.

No. Can't be changed to CustomLogger. See comment above.

prmathur-microsoft · 2017-03-17T00:43:13Z

I'm very much interested in the original change that you mentioned is required by the customer related to printing stack trace and moving it to logger. If you can use custom logger at the moment until we address better logging across the SDK, I can pull this change in.

JMayrbaeurl · 2017-03-21T11:34:18Z

Since this is a critical bug fix for our partner I have changed back the logger implementation to use the CustomLogger class instead of slf4j (still believe that this is the only way to do logging).

Merged 1.0.22 release changes

prmathur-microsoft · 2017-03-27T23:10:52Z

+
+        if (transport == null)
+        	throw new IllegalArgumentException("Parameter 'transport' must not be null");
+


Move this check to line 22

@prmathur-microsoft : With current setup constructors of IotHubSendTask and IotHubReceiveTask will write a wrong log error message when parameter 'transport' is not null. If it's null, no log message will be written, since the code is no longer reachable

prmathur-microsoft · 2017-03-27T23:11:35Z

        this.transport = transport;
+
+        if (transport == null)
+        	throw new IllegalArgumentException("Parameter 'transport' must not be null");


Move this check to line 23

@prmathur-microsoft : With current setup constructors of IotHubSendTask and IotHubReceiveTask will write a wrong log error message when parameter 'transport' is not null. If it's null, no log message will be written, since the code is no longer reachable

prmathur-microsoft · 2017-03-28T00:52:48Z

This PR did not have the following :

Did not address CR
Went back and forth between log4j and slf4j without addressing the issue at hand which was blocking the customer.
Author was unwilling to address any CR changes given in the past, hence I have addressed it myself to unblock the customer and taken it forward.
PR was not split well between what was needed vs what is good to have.

Addressed all the above, squashed all the commits, pushed changes to unblock customer @commit d9cebc3

JMayrbaeurl · 2017-03-28T07:00:17Z

Concerning @prmathur-microsoft latest comments:

Did not address CR: First commit @commit fc925b3 in PR provided 18 days ago completely addressed CR and didn't introduce any kind of other changes.
Went back and forth between log4j and slf4j without addressing the issue at hand which was blocking the customer: Introducing logging was proposed by @prmathur-microsoft and changed several times to fulfill non-standard logging approach required by reviewer.
Author was unwilling to address any CR changes given in the past, hence I have addressed it myself to unblock the customer and taken it forward: Latest version of PR was done with CustomLogger usage according to CR of @prmathur-microsoft . No unblocking of the customer so far, since released versions of the SDKs still have this issue.
PR was not split well between what was needed vs what is good to have: : First version of PR, provided 18 days ago, simply solved the customer's problem. Discussions about additional changes was started by @prmathur-microsoft

JMayrbaeurl added 2 commits March 10, 2017 11:15

Added Eclipse project files to ignore settings

aff5a1d

added configuration for stack trace writting

fc925b3

msftclas added the cla-required label Mar 10, 2017

msftclas added cla-signed and removed cla-required labels Mar 10, 2017

prmathur-microsoft reviewed Mar 14, 2017

View reviewed changes

JMayrbaeurl added 6 commits March 15, 2017 06:46

Merge pull request #1 from Azure/master

40e7fa2

Merged changes from Release 2017-03-10

Merge pull request #2 from JMayrbaeurl/master

49f201d

Merged latest release changes

# WARNING: head commit changed in the meantime

96afcfe

added configuration for stack trace writting

Added release changes

9ae29a8

Merge branch 'bosch-dev' of https://github.com/JMayrbaeurl/azure-iot-…

14c59d9

…sdk-java.git into bosch-dev

Added slf4j logging and make stack trace output configurable

e7c0cfb

JMayrbaeurl mentioned this pull request Mar 15, 2017

Don't use log4j directly for logging - Replace with logging facade slf4j #50

Closed

prmathur-microsoft reviewed Mar 17, 2017

View reviewed changes

JMayrbaeurl mentioned this pull request Mar 17, 2017

Don't use log4j directly for logging - Replace with logging facade slf4j #57

Closed

4 tasks

Removed slf4j usage with CustomLogger class

ae72634

Merge pull request #3 from Azure/master

724df07

Merged 1.0.22 release changes

prmathur-microsoft reviewed Mar 27, 2017

View reviewed changes

prmathur-microsoft closed this Mar 28, 2017

JMayrbaeurl deleted the bosch-dev branch March 28, 2017 18:26


		if (transport == null)
		throw new IllegalArgumentException("Parameter 'transport' must not be null");

Conversation

JMayrbaeurl commented Mar 10, 2017 • edited by tameraw Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Reference/Link to the issue solved with this PR (if any)

Description of the problem

Description of the solution

Uh oh!

msftclas commented Mar 10, 2017

Uh oh!

msftclas commented Mar 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prmathur-microsoft commented Mar 14, 2017

Uh oh!

JMayrbaeurl commented Mar 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prmathur-microsoft commented Mar 17, 2017

Uh oh!

JMayrbaeurl commented Mar 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prmathur-microsoft commented Mar 28, 2017

Uh oh!

JMayrbaeurl commented Mar 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JMayrbaeurl commented Mar 10, 2017 •

edited by tameraw

Loading