Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unncessary response deserializations in PartialSuccessTransmissionPolicy for ingestion sampling cases #2445

Closed
vangarp opened this issue Oct 19, 2021 · 4 comments · Fixed by #2500
Milestone

Comments

@vangarp
Copy link
Contributor

vangarp commented Oct 19, 2021

  • List of NuGet packages and version that you are using: Microsoft.ApplicationInsights.AspNetCore v2.16.0
  • Runtime version (e.g. net461, net48, netcoreapp2.1, netcoreapp3.1, etc. You can find this information from the *.csproj file): netcoreapp3.1
  • Hosting environment (e.g. Azure Web App, App Service on Linux, Windows, Ubuntu, etc.): Windows

Describe the bug

Due to the way the code at

if (backendResponse.ItemsAccepted != backendResponse.ItemsReceived)
{
string[] items = JsonSerializer
.Deserialize(initialTransmission.Content)
.Split(new[] { Environment.NewLine }, StringSplitOptions.RemoveEmptyEntries);
foreach (var error in backendResponse.Errors)
{
if (error != null)
{
is, we are deserializing the initialTransmission.Content any time the items backendResponse.ItemsAccepted != backendResponse.ItemsReceived regardless of the error codes (which is checked later). In cases when there is ingestion time sampling enabled, backendResponse.ItemsAccepted != backendResponse.ItemsReceived due to the sampling and we end up deserializing the items even though the error is not a retryable error code in such cases (statusCode 206). This results in unncessary memory allocations and wasted CPU cycles.

To Reproduce

In your AI resource just turn on ingestion side sampling to a relatively high degree and collect a memory dump using Perfview etc.

@vangarp vangarp added the bug label Oct 19, 2021
@cijothomas cijothomas removed the bug label Oct 19, 2021
@vangarp
Copy link
Contributor Author

vangarp commented Oct 19, 2021

We do the deserialization and other checks like error.Index >= items.Length once we confirm that this is due to a retryable error. Please let me know if there is some case I am missing else I can send out a PR

@cijothomas
Copy link
Contributor

I am not sure what is the bug here.

  1. PartialSuccessTransmissionPolicy is only going to examine/parse response for 206 status code.
  2. Not sure how is this related to ingestion sampling. SDK doesn't know anything about ingestion sampling.

@vangarp
Copy link
Contributor Author

vangarp commented Oct 19, 2021

@cijothomas my understanding is that PartialSuccessTransmissionPolicy is for the cases when response is 206, but the underlying error (which is present in the response.Errors[i].statusCode) is amongst the retryable error status codes like 503, 429, 408 etc. When server side ingestion is turned on, we get a 206 from the server, but the response payload looks like this:
{"itemsReceived": 1, "itemsAccepted": 0, "errors": [{ "index": 0, "statusCode": 206, "message": "Telemetry sampled out." }]}

In such cases we do not need to deserialize the response object because we can examine that response.Errors[i].statusCode == 206 and in such cases skip deserializing the initialTransmission.Content completely.

@vangarp
Copy link
Contributor Author

vangarp commented Oct 20, 2021

@TimothyMothra @cijothomas have submitted a PR for this, please take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants