[BUG] Failed XHR request after ever tracked item when gathered logs exceed maxBatchSizeInBytes while offline

[peitschie]

Description/Screenshot
I'm using ApplicationInsights-JS from within a cordova-based application, which often operates offline for reasonable periods of time.

I've discovered that when the collected offline logs exceed the configured maxBatchSizeInBytes (default is ~10kb), the sender starts trying to post records via XHR after every new log item added. This isn't really expected behaviour, as the Sender knows it's currently offline, and should be using the backoff policies.

Steps to Reproduce

• OS/Browser: Any. These profiles were captured with Android 12's WebView class
• SDK Version [e.g. 22]: 2.8.3
• How you initialized the SDK: All default settings
• Track ~200 items while being offline (need enough to exceed maxBatchSizeInBytes)

Expected behavior
When offline, no immediate send batches should be attempted, as they can't possibly succeed.

I suspect a check for the offline state should be added here: https://github.com/microsoft/ApplicationInsights-JS/blob/master/channels/applicationinsights-channel-js/src/Sender.ts#L372

[MSNev]

I've just checked the code and the "offline" detection is only used to stop retries, it appears that any event will always be batched for sending and the issue your hitting here is because the maxBatchSizeInBytes is being exceeded which effective is causing a flush of the batched events.

To add support for full offline detection and the dropping of "old" batch events is not a small task. Tagging this as an enhancement. Not sure of the timeframe for scheduling something like this as the SDK is primarily designed as an Online SDK....

But this is not the first ask for this.

[peitschie]

Would there be any disadvantage to adding an offline check before the forced send?

I'm happy to raise a PR for this... it seems like a fairly safe fix, that eliminates the unnecessary call failure?

[MSNev]

That is one of the locations to put the check, but once we do that we start to violate the maxBatchSizeInBytes and this could easily lead to memory exhaustion. So I'm not sure it's that simply.

In terms of PR's yes, we are happy to take them.

Just be aware that we are about to release the beta (which has now been promoted to main) and we will be switching the default branch very shortly from master (v2.x) to main (v3.x)

[peitschie]

Yeah... the maxBatchSizeInBytes is an issue, but fortunately (or unfortunately 😅 ) it's an existing issue unaffected by this fix 🙂

Which branch should I raise a PR against for adding the single check? We're still on the 2.X series internally, so I have a mild preference to getting the fix in that series if possible... completely understand however if that's not an option!

[MSNev]

Go for master for now, we will need to cherry-pick across to main, We may need to add some additional code around cleanup (dropping older events) and I suspect that when you go to create the PR (after we switch to main) it might cause some grief (creating the PR).

The current plan is to do this most likely by the end of this week (PST) and maybe even tomorrow. We are only release v3.x to NPM to start with.

[peitschie]

Sounds like a plan! I'll raise something shortly, and am happy to address other aspects if you'd prefer it to be part of the PR.

I'm conscious though that sometimes that's slower than just doing the work yourself though, so happy to work however suits you best here 😄

[peitschie]

Just logging a workaround that's helped here:

I've just increased that maxBatchSizeInBytes to ~1Mb. This puts the message cap up quite a bit higher, and though the re-serialization is still expensive, it avoids the very very expensive send attempt and subsequent re-queueing that happens otherwise.

[MSNev]

Apart from releasing the ApplicationInsights v3.x and handling any required hotfixes we still have quite a lot of work booked in April / May to complete the process which includes

• Updating and releasing all of the additional plugins (react, react-native, angular)
• We also have an internal extension that needs to be updated to use v3.x and then sent though the same release process (by the end of April)
• Moving around some of those internal extensions into this repo

So at the moment I'm pretty sure we are not going to get this this before at least the middle of the year (as there is not the complete list). So it's the perfect time for you to jump in, I'll apologize now if I become tardy in responding to any PR's

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.