- Fixing forever loop if something is in illegal state. … #405

andresol · 2017-08-07T07:37:19Z

E.g. Web app is stopped and some code tries to access classes that is removed from class path.

…Eg. Web app is stopped and some code tries to access classes that is on in classpath.

Increasing version number to 1.0.10

Adding ThirdPartyNotices.txt

Dmitry-Matveev · 2017-08-17T01:38:50Z

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

@@ -176,6 +176,9 @@ public boolean send(Transmission transmission) {
                if (ioe instanceof ConnectTimeoutException) {
                    transmissionPolicyManager.suspendInSeconds(TransmissionPolicy.BLOCKED_BUT_CAN_BE_PERSISTED, DEFAULT_BACKOFF_TIME_SECONDS);
                }
+            } catch (IllegalStateException e) {
+                suspendTransmissions(TransmissionPolicy.BLOCKED_AND_CANNOT_BE_PERSISTED, response);


It looks like suspendTransmission() will exit at the very first line. In case of the IllegalStateException there is no RetryAfter header to read and get the time to retry from. This would mean that the behavior will be just to return here which is not different from general catch (Exception e) block. Are you sure that this suspension fixes the issue with the forever loop?

This is correct, and my changes will not work. I fixed the "forever loop bug" in 1.0.8 by adding transmissionPolicyManager.suspendInSeconds(...) I did not notice that suspendTransmissions() is very different from transmissionPolicyManager.suspendInSeconds(..) that is master branch, but I have tested and the forever loop also occure with latest code (10.07.2017)

Would you think replacing it with suspendInSeconds() or just dropping the telemetry item if an IllegalStateException happens is the right choice here to continue with this PR?

Sorry for late reply. I was on vacation. I think dropping the telemetry item is the right choice. The java environment is not in an appropriate state to continue. Is the best way to just shut down by calling TransmissionPolicyManager.stop()?

Do not set telemetry timestamp if it already exists

None of the other SDKs use the `APPLICATION_INSIGHTS_IKEY` environment variable to configure the instrumentation key for Application Insights and instead use `APPINSIGHTS_INSTRUMENTATIONKEY`. For consistency with the other SDKs (e.g. Dot-Net https://aka.ms/n2ruy1 or Node https://aka.ms/v8r0pk) this Java SDK should also support the more common environment variable name to avoid confusion for developers who have experience with the non-Java SDKs.

Add second environment variable to configure ikey

Fixes custom telemetry timestamp bug

…cure.

Update ChangeLog

Dmitry-Matveev · 2017-08-24T02:54:22Z

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

@@ -176,6 +177,9 @@ public boolean send(Transmission transmission) {
                if (ioe instanceof ConnectTimeoutException) {
                    transmissionPolicyManager.suspendInSeconds(TransmissionPolicy.BLOCKED_BUT_CAN_BE_PERSISTED, DEFAULT_BACKOFF_TIME_SECONDS);
                }
+            } catch (IllegalStateException e) {
+                transmissionPolicyManager.stop(SHUTDOWN_TIME, TimeUnit.SECONDS);


@andresol , Is there a way this code (or even app) can get out of the illegal state with one of the next method invocations being successful?

I know I suggested stop() myself, but, on the second thought, I now realize that it leads to all sender threads being stopped and never trying again if they are not re-initialized. So, a single IllegalStateException will stop telemetry from the app up until the next app restart. With this, if there is a possibility that the code may recover on its own - we'd better let it try (unobtrusively of course, without infinite loop :) )

Sorry that it turned out to be a lengthy discussion with back and forth, I just do not want to jump into the conclusion with the severe side effects. Let me know your thoughts.

@Dmitry-Matveev i agree. It is better to let it try. DEFAULT_BACKOFF_TIME_SECONDS may be a bit long time. It would be a very "big" bug if the app can recover from a illegall state exception.

I had to fix the problem in 1.0.8 with transmissionPolicyManager.suspendInSeconds() because in just mater of minutes it filled like 60 gig of diskspace. It works very well. Not filling up my server:) I will make new changes.

…likely not recover. This will prevent forever loop.

…com/Microsoft/ApplicationInsights-Java into dhaval/RevertSetSampleRateNullCheck

…eRateNullCheck Dhaval/revert set sample rate null check

Dmitry-Matveev · 2017-09-08T20:35:18Z

@andresol, there is another PR coming into the same area that will enable back-off for all exceptions to make the "while(true)" 100% CPU loop in case of networking issues less pronounced. In that PR all exceptions will have the same back-off. Would you like to keep 30 seconds or 300 looks OK in this case? If yes, can you please switch to `DEFAULT_BACKOFF" to simplify merge for the second PR above coming after this one?

* Jedis Visitor Changes * Hooking up Jedis visitor * remove printstack * Update changelog * fixed typo

… all

…g truncate method

Merge Dhdoshi/sanitize json into master

andresol · 2017-09-15T10:38:31Z

Sorry for late answer. Have been very busy. I think DEFAULT_BACKOFF is ok, but shorter back off time is in my opinion to be prefered. I have changed back too DEFAULT_BACKOFF and if we have a lot of complains about server is hanging for long time and so on we could change it.

Delay retrying to send telemetry if an exception is hit

…Eg. Web app is stopped and some code tries to access classes that is on in classpath.

…likely not recover. This will prevent forever loop.

# Conflicts: # core/src/main/java/com/microsoft/applicationinsights/internal/channel/common/TransmissionNetworkOutput.java

Dmitry-Matveev · 2017-09-20T01:12:10Z

@andresol , change looks good, however I see that this merge is now trying to update many files, have you pulled/merged master locally?

andresol · 2017-09-20T08:53:05Z

yes. I merged in master branch locally. Too keep my branch up to date and too update the code to match the merge of #420.

dhaval24 · 2017-11-15T06:37:09Z

@andresol we would really like to merge this contribution but there seems to be too many recreated files in this PR where as change is not that big. Would it be possible for you to create a new PR from the latest master with the required set of changes so we can merge it. Let me know if that is not possible because the only fear here is that it can affect the linear flow of commits on the repository.

gavlyukovskiy · 2017-11-17T10:50:56Z

core/src/main/java/com/microsoft/applicationinsights/telemetry/JsonTelemetryDataSerializer.java

@@ -295,9 +296,11 @@ public void write(String name, String value) throws IOException {
    }

    private void writeName(String name) throws IOException {
+        String truncatedString = truncate(name, 150);
+        String sanitizedName = SanitizationUtils.sanitizeStringForJSON(truncatedString, true);


I'd recommend to look at existing libraries for json processing, such as Jackson where you can use ObjectMapper to write string/object as json.

littleaj · 2018-01-10T00:15:52Z

#501 addresses this issue

- Fixing potentially for ever loop if something is in illegal state. …

fcdd087

…Eg. Web app is stopped and some code tries to access classes that is on in classpath.

msftclas added the cla-not-required label Aug 7, 2017

andresol closed this Aug 7, 2017

andresol reopened this Aug 8, 2017

msftclas added the cla-not-required label Aug 8, 2017

andresol mentioned this pull request Aug 8, 2017

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

Closed

Dmitry-Matveev added 5 commits August 15, 2017 12:05

Increasing version number to 1.0.10

2281a85

Merge pull request microsoft#408 from Microsoft/dmitmatv_110

d7d800d

Increasing version number to 1.0.10

Adding ThirdPartyNotices.txt

1e28bc3

Addressing comments in TPN.txt

958eee5

Merge pull request microsoft#409 from Microsoft/dmitmatv_TPN

21d4b1f

Adding ThirdPartyNotices.txt

Dmitry-Matveev requested changes Aug 17, 2017

View reviewed changes

adnanyaqoobvirk and others added 10 commits August 19, 2017 18:57

fixes custom timestamp bug

d7959b6

Do not set telemetry timestamp if it already exists

Fix header markdown for rendering on Github

27ad0a3

Add changelog entry for new environment variable

83bb635

Merge pull request microsoft#413 from c-w/feature/second-ikey-envvar

28dec6f

Add second environment variable to configure ikey

adds custom telemetry timestamp test

e9118ac

Merge pull request microsoft#412 from adnanyaqoobvirk/master

39f473b

Fixes custom telemetry timestamp bug

Update ChangeLog

a0e8f43

Just shutting down the transmissing when a illegal state exception oc…

2559a4c

…cure.

Merge pull request microsoft#414 from Microsoft/dmitmatv_changelog110

46fdca0

Update ChangeLog

Dmitry-Matveev reviewed Aug 24, 2017

View reviewed changes

andresol and others added 7 commits August 24, 2017 10:25

If a IllegalStateException occurs, we need to suspend. App will most …

4dcc3e7

…likely not recover. This will prevent forever loop.

Reverting remove null check in setSampleRate()

2aa19f9

fixing duration write method

2fcfc95

Updating readme for fixing telemetry pushing issue

7d02375

update null check location

70eb0d2

Merge branch 'dhaval/RevertSetSampleRateNullCheck' of https://github.…

8e221cf

…com/Microsoft/ApplicationInsights-Java into dhaval/RevertSetSampleRateNullCheck

Merge pull request microsoft#415 from Microsoft/dhaval/RevertSetSampl…

420ace2

…eRateNullCheck Dhaval/revert set sample rate null check

Dmitry-Matveev mentioned this pull request Sep 8, 2017

Delay retrying to send telemetry if an exception is hit #420

Merged

dhaval24 and others added 12 commits September 10, 2017 14:36

Fixing custom event sending bug

cab4ab2

adding test case for sanitization

1f0805a

Fixing reliability issue with JedisMethodVisitor. (microsoft#423)

46fc0e4

* Jedis Visitor Changes * Hooking up Jedis visitor * remove printstack * Update changelog * fixed typo

adding missing escape character \' and missing test

cc310f7

fixing previous commit test case

bad01c5

resolving merge conflict

1fd87c2

Merge branch 'master' into dhdoshi/sanitizeJSON

7beafc5

Adding truncate methods removing unnecessary test as following covers…

1f60a64

… all

reducing scope to package-private, fixing corner case and generalizin…

819f72a

…g truncate method

Making CHANGELOG.md more human friendly

f33c220

Merge pull request microsoft#421 from Microsoft/dhdoshi/sanitizeJSON

0de2fc2

Merge Dhdoshi/sanitize json into master

Changed back to default backoff time.

0c99bb6

leantk and others added 6 commits September 15, 2017 09:42

Updated changelog

af83baf

Merge pull request microsoft#420 from leantk/users/leantk/backoff

cbdef0c

Delay retrying to send telemetry if an exception is hit

- Fixing potentially for ever loop if something is in illegal state. …

59ff210

…Eg. Web app is stopped and some code tries to access classes that is on in classpath.

If a IllegalStateException occurs, we need to suspend. App will most …

23b273e

…likely not recover. This will prevent forever loop.

Merge remote-tracking branch 'origin/master'

aeb8c38

# Conflicts: # core/src/main/java/com/microsoft/applicationinsights/internal/channel/common/TransmissionNetworkOutput.java

Removing unused parameter.

322f477

msftgits removed the cla-not-required label Sep 26, 2017

microsoft deleted a comment from msftclas Sep 26, 2017

gavlyukovskiy reviewed Nov 17, 2017

View reviewed changes

littleaj closed this Jan 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

- Fixing forever loop if something is in illegal state. … #405

- Fixing forever loop if something is in illegal state. … #405

andresol commented Aug 7, 2017

Dmitry-Matveev Aug 17, 2017 •

edited

Loading

andresol Aug 17, 2017

Dmitry-Matveev Aug 22, 2017

andresol Aug 23, 2017

Dmitry-Matveev Aug 24, 2017

andresol Aug 24, 2017

Dmitry-Matveev commented Sep 8, 2017

andresol commented Sep 15, 2017

Dmitry-Matveev commented Sep 20, 2017

andresol commented Sep 20, 2017

dhaval24 commented Nov 15, 2017

gavlyukovskiy Nov 17, 2017

littleaj commented Jan 10, 2018

- Fixing forever loop if something is in illegal state. … #405

- Fixing forever loop if something is in illegal state. … #405

Conversation

andresol commented Aug 7, 2017

Dmitry-Matveev Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

andresol Aug 17, 2017

Choose a reason for hiding this comment

Dmitry-Matveev Aug 22, 2017

Choose a reason for hiding this comment

andresol Aug 23, 2017

Choose a reason for hiding this comment

Dmitry-Matveev Aug 24, 2017

Choose a reason for hiding this comment

andresol Aug 24, 2017

Choose a reason for hiding this comment

Dmitry-Matveev commented Sep 8, 2017

andresol commented Sep 15, 2017

Dmitry-Matveev commented Sep 20, 2017

andresol commented Sep 20, 2017

dhaval24 commented Nov 15, 2017

gavlyukovskiy Nov 17, 2017

Choose a reason for hiding this comment

littleaj commented Jan 10, 2018

Dmitry-Matveev Aug 17, 2017 •

edited

Loading