Implementing Retry logic [Reliable Channel] [STABLE Branch] #561

debugthings · 2018-02-14T21:08:46Z

Completion of PR #556 into Stable Branch

Added retry logic that is similar to the .NET framework.

Removes logic from NetworkTransmissionOutput
Uses observers to handle error conditions
Added failed transmission retry
Added partial transmission retry
Aligned response codes to match .NET
Uses existing ExponentialBackOffTimesPolicy

NOTE This clobbers the change in #385 referenced here

https://github.com/Microsoft/ApplicationInsights-Java/blob/7a5e572b2ffd3256898c6bd05dc3bec6754e3629/core/src/main/java/com/microsoft/applicationinsights/internal/channel/common/TransmissionNetworkOutput.java#L172

All exceptions are now handled by ErrorHandler in this line.

https://github.com/Microsoft/ApplicationInsights-Java/blob/094383a25b0f98fe6aa2dd6dd58bae10259492e6/core/src/main/java/com/microsoft/applicationinsights/channel/concrete/inprocess/ErrorHandler.java#L39-L42

For significant contributions please make sure you have completed the following items:

Design discussion issue #
Changes in public surface reviewed
CHANGELOG.md updated

… sender channel.

…ity. Added extension to return the timeout values as expected before.

Moved the Handlers out of the concrete package to the common package to keep the same consistency. Removed a couple of unessecary methods. Added docs.

Added MaxInstantRetry configuration to allow for instantaneous retry on a failed transmission.

Javadoc and formatting updates

grlima · 2018-02-14T21:55:30Z

.../com/microsoft/applicationinsights/channel/concrete/inprocess/InProcessTelemetryChannel.java

+			throttling = Boolean.valueOf(namesAndValues.get("Throttling"));
+			developerMode = Boolean.valueOf(namesAndValues.get(DEVELOPER_MODE_NAME));
+			try {
+				maxInstantRetries = Integer.parseInt(INSTANT_RETRY_NAME);


I think this will throw. Shouldn't it be Integer.parseInt(namesAndValues.get(INSTANT_RETRY_NAME)); ?

Yes good catch. This will blow. But I am not quite sure, it seems that this value comes as string from XML. In case of user specifies wrong value which cannot be converted to string then this would throw. Let's have try catch around this.

Yes very good catch. Fixed and added null check.

Integer.parseInt does throw NumberFormatException which I already had a catch for. It should only log if we have ERROR tracing on.

dhaval24 · 2018-02-14T22:03:07Z

@debugthings just wanted to understand did you changed / added anything into this PR which is different from the previous one which we reviewed extensively? Do I need to go through this PR again?

dhaval24 · 2018-02-15T04:18:27Z

.../com/microsoft/applicationinsights/channel/concrete/inprocess/InProcessTelemetryChannel.java

+
+	private final static String INSTANT_RETRY_NAME = "MaxInstantRetry";
+	private final static int DEFAULT_MAX_INSTANT_RETRY = 3;
+	private final static int DEFAULT_MAX_TELEMETRY_BUFFER_CAPACITY = 500;


This number also seems low to me. I think default max capacity can be double

It's the same as before, so we should not change it in this PR.

dhaval24 · 2018-02-15T04:18:58Z

.../com/microsoft/applicationinsights/channel/concrete/inprocess/InProcessTelemetryChannel.java

+	private final static int DEFAULT_MAX_INSTANT_RETRY = 3;
+	private final static int DEFAULT_MAX_TELEMETRY_BUFFER_CAPACITY = 500;
+	private final static int MIN_MAX_TELEMETRY_BUFFER_CAPACITY = 1;
+	private final static int MAX_MAX_TELEMETRY_BUFFER_CAPACITY = 1000;


This is also definitely less. I think this should be definitely increased.

It's the same as before, so we should not change it in this PR.

dhaval24 · 2018-02-15T04:24:21Z

.../com/microsoft/applicationinsights/channel/concrete/inprocess/InProcessTelemetryChannel.java

+			telemetry.getContext().getProperties().put("DeveloperMode", "true");
+		}
+
+		if (telemetrySampler != null) {


Is this just kept for maintaining backward compatibility with Sampling V1? Nonetheless, with this major version we should definitely scrap off the old sampling algorithm as it uses user id based sampling which is not desirable for sure.

@dhaval24 - Again, nothing new here. The same code was there before. We can address unrelated changes as separate PR's.

dhaval24 · 2018-02-15T04:25:38Z

.../com/microsoft/applicationinsights/channel/concrete/inprocess/InProcessTelemetryChannel.java

+	 *            - The sampler
+	 */
+	@Override
+	public void setSampler(TelemetrySampler telemetrySampler) {


This should be deprecated. It is the old sampling strategy which we do not encourage any more. If we are not scrapping it off entirely then atleast please use @depracted tag here

@dhaval24 - same here. Unrelated to this PR.

dhaval24 · 2018-02-15T04:30:48Z

...ava/com/microsoft/applicationinsights/internal/channel/common/TransmissionPolicyManager.java

+		}
+	}
+
+	public void setMaxInstantRetries(int maxInstantRetries) {


Java docs are still missing here!

dhaval24 · 2018-02-15T04:31:13Z

...ava/com/microsoft/applicationinsights/internal/channel/common/TransmissionPolicyManager.java

+		}
+	}
+
+	public int getMaxInstantRetries() {


same here please update java docs here.

dhaval24

overall looks good to me. Just some minor nit-pickings

grlima

One change needed when reading instant retry configuration. Need to get value from "namesAndValues" map. See comment. Thanks!

littleaj · 2018-02-16T01:49:04Z

...ava/com/microsoft/applicationinsights/internal/channel/common/TransmissionPolicyManager.java

-
+public final class TransmissionPolicyManager implements Stoppable, TransmissionHandlerObserver {
+
+	private int INSTANT_RETRY_AMOUNT = 3; // Should always be set by the creator of this class


this is using the java naming convention for constants. For member variables, camelCase is preferred, but I'm not going to block the PR for this :)

Fair enough.

Added null check

debugthings · 2018-02-16T02:10:51Z

@dhaval24 I did add the ability to set the max retry amount from the XML so it will be available if that number is too high or low. Outside of that, it was adding javadocs or simple refactor

dhaval24 · 2018-02-16T02:24:34Z

@debugthings for me it's approved :-) I do not have any additional concerns

AlexKlimovMS

Still reviewing

AlexKlimovMS · 2018-02-16T23:53:25Z

...ava/com/microsoft/applicationinsights/internal/channel/common/TransmissionPolicyManager.java

+        if (backOffMillis > 0)
+        {
+        	long backOffSeconds = backOffMillis / 1000;
+        	InternalLogger.INSTANCE.logAlways(InternalLogger.LoggingLevel.TRACE, "App is throttled, telemetry will be blocked for %s seconds.", backOffSeconds);


Would this call become very noisy with no option to turn off on big load?

Possibly, I can remove the log always, The backoff is called in large increments so it would become chatty if there were multiple threads and if we were unable to send data for a large amount of time.

AlexKlimovMS · 2018-02-16T23:54:52Z

...ava/com/microsoft/applicationinsights/internal/channel/common/TransmissionPolicyManager.java

+
+	// List of transmission policies implemented as handlers
+	private List<TransmissionHandler> transmissionHandlers;
+
    // The future date the the transmission is blocked


Cleaning spaces irregularities is always good idea. Code should be clean logically and stylistically =)

AlexKlimovMS · 2018-02-16T23:55:05Z

...ava/com/microsoft/applicationinsights/internal/channel/common/TransmissionPolicyManager.java

+     */
+    public void clearBackoff() {
+    	policyState.setCurrentState(TransmissionPolicy.UNBLOCKED);
+        backoffManager.onDoneSending();


AlexKlimovMS · 2018-02-17T00:17:41Z

...in/java/com/microsoft/applicationinsights/internal/channel/common/PartialSuccessHandler.java

+				}
+				if (bufferedReader != null) {
+					bufferedReader.close();
+				}


This should be closed in finally statement?

Yes this if should definitely go in finally!

lines 125--130 should be wrapped in the finally block below

Yup, moved to finally and added try/catch around those close() statements as well.

@dhaval24

* Fix null ref check in telemetry correlation Utils (#541) * Fix null ref check in TelemetryCorrelationUtils * Modifying log level to warning * Updating Changelog * Fix handling of NaN and +/-Infinity in JSON serializer (#499) * Handle NaN and +/-Infinity in metrics * Default NaN/Infinity serialization to 0 to be consistent with other AI SDKs and make the code compatible with Java 6 * fixed javadoc errors and added section to generate pom.xml with all builds (#551) * Updating version number to 2.0.0 * Implementing Retry logic [Reliable Channel] [STABLE Branch] (#561) * Initial commit of retry and backoff logic fixes * Fixing warnings on files I touched this round * Fix the eclipse UI from screaming about the docker Contstants * Fixed backoff logic to use existing method. Added more logging to the sender channel. * Added the partial response handler, more logging * Added gson to core. Fixed backoff manager to keep original functionality. Added extension to return the timeout values as expected before. * Added unit tests. * Fixing string typed ArrayList<> to List<> per Dhaval * Missed one * Making tests consistent. * Added javadoc comments, simplified logic for a few methods * Added exception logging per @dhaval24. Fixed formatting on touched files * Updates per last round of commits Moved the Handlers out of the concrete package to the common package to keep the same consistency. Removed a couple of unessecary methods. Added docs. * Latest fixes * Add MaxInstantRetry Added MaxInstantRetry configuration to allow for instantaneous retry on a failed transmission. * Javadoc Updates Javadoc and formatting updates * NumberFormatException fix Added null check * JavaDocs for TPM * Fixing FixedRateSampling to work in old and new version of sampling (#540) Overriding default sampling percentage when programatically specified sampling percentage by user. * upgrade to logback v1.2.3 (#565) * Reliable channel: replacing logAlways "TRACE" statements with "info" (#571) * Reliable channel: close resources in finally block. (#572) * Reliable channel: close resources in finally block. * change logging to warning when closing resources * Bugfix against retry logic (#576) * Refactor * BUGFIX Logic would never backoff After adding the instant retry amount logic to the code this line of code could cause the transmissions to not back off. * Changes requested * Fixed javadocs tags, that caused build errors while executing `javadoc` gradle task (#578) * Update Changelog * Fix link in changelog * Fix another link in changelog * Update gradle.properties * Fix customizing pom.xml in Gradle build (#582) * Fix customizing pom.xml in Gradle build * Insert license after 1. row in pom.xml * Filter artifacts relocated by shadow task from pom dependencies - match artifacts by groupId - fixes #583 * Generate a pom file "beside" the artifact jar file

debugthings and others added 16 commits February 14, 2018 12:28

Initial commit of retry and backoff logic fixes

310fb54

Fixing warnings on files I touched this round

677de5c

Fix the eclipse UI from screaming about the docker Contstants

7d48b9d

Fixed backoff logic to use existing method. Added more logging to the…

795c287

… sender channel.

Added the partial response handler, more logging

ca6a0fa

Added gson to core. Fixed backoff manager to keep original functional…

b852d36

…ity. Added extension to return the timeout values as expected before.

Added unit tests.

6c62ff8

Fixing string typed ArrayList<> to List<> per Dhaval

3c8c727

Missed one

5d36b51

Making tests consistent.

ec27f1c

Added javadoc comments, simplified logic for a few methods

09229f5

Added exception logging per @dhaval24. Fixed formatting on touched files

893c7ea

Updates per last round of commits

623cdda

Moved the Handlers out of the concrete package to the common package to keep the same consistency. Removed a couple of unessecary methods. Added docs.

Latest fixes

7e5ca9d

Add MaxInstantRetry

b592077

Added MaxInstantRetry configuration to allow for instantaneous retry on a failed transmission.

Javadoc Updates

0d63c88

Javadoc and formatting updates

grlima reviewed Feb 14, 2018

View reviewed changes

dhaval24 reviewed Feb 15, 2018

View reviewed changes

dhaval24 approved these changes Feb 15, 2018

View reviewed changes

grlima suggested changes Feb 15, 2018

View reviewed changes

littleaj reviewed Feb 16, 2018

View reviewed changes

debugthings added 2 commits February 15, 2018 21:02

NumberFormatException fix

80861f4

Added null check

JavaDocs for TPM

9c162fb

grlima approved these changes Feb 16, 2018

View reviewed changes

grlima merged commit 1fad688 into microsoft:2.0.0-STABLE Feb 16, 2018

AlexKlimovMS reviewed Feb 17, 2018

View reviewed changes

dhaval24 mentioned this pull request Feb 19, 2018

Added application insights starter for spring boot #518

Merged

3 tasks

grlima mentioned this pull request Feb 23, 2018

Reliable channel: close resources in finally block. #572

Merged

3 tasks

dhaval24 mentioned this pull request Feb 24, 2018

Added a simple transmission retry mechanism and capturing of exception stacktrace #501

Closed

3 tasks

grlima mentioned this pull request Feb 27, 2018

Implementing Retry logic [Reliable Channel] #556

Closed

3 tasks

dhaval24 mentioned this pull request Mar 3, 2018

TransmissionNetworkOutput loops forever if encountering java.lang.IllegalStateException #406

Closed

dhaval24 mentioned this pull request Mar 13, 2018

add MaxInstantRetry XML Element in Channel configuration #604

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing Retry logic [Reliable Channel] [STABLE Branch] #561

Implementing Retry logic [Reliable Channel] [STABLE Branch] #561

debugthings commented Feb 14, 2018

grlima Feb 14, 2018 •

edited

Loading

dhaval24 Feb 14, 2018 •

edited

Loading

debugthings Feb 16, 2018

debugthings Feb 16, 2018

dhaval24 commented Feb 14, 2018

dhaval24 Feb 15, 2018

grlima Feb 15, 2018

dhaval24 Feb 15, 2018

grlima Feb 15, 2018

dhaval24 Feb 15, 2018

grlima Feb 15, 2018

dhaval24 Feb 15, 2018

grlima Feb 15, 2018

dhaval24 Feb 15, 2018

dhaval24 Feb 15, 2018

dhaval24 left a comment

grlima left a comment

littleaj Feb 16, 2018

debugthings Feb 16, 2018

debugthings commented Feb 16, 2018

dhaval24 commented Feb 16, 2018

AlexKlimovMS left a comment

AlexKlimovMS Feb 16, 2018

debugthings Feb 20, 2018

AlexKlimovMS Feb 16, 2018

AlexKlimovMS Feb 16, 2018

AlexKlimovMS Feb 17, 2018

dhaval24 Feb 17, 2018

littleaj Feb 17, 2018

debugthings Feb 20, 2018


		public final class TransmissionPolicyManager implements Stoppable, TransmissionHandlerObserver {

		private int INSTANT_RETRY_AMOUNT = 3; // Should always be set by the creator of this class

Implementing Retry logic [Reliable Channel] [STABLE Branch] #561

Implementing Retry logic [Reliable Channel] [STABLE Branch] #561

Conversation

debugthings commented Feb 14, 2018

grlima Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

dhaval24 Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhaval24 commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhaval24 left a comment

Choose a reason for hiding this comment

grlima left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debugthings commented Feb 16, 2018

dhaval24 commented Feb 16, 2018

AlexKlimovMS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grlima Feb 14, 2018 •

edited

Loading

dhaval24 Feb 14, 2018 •

edited

Loading