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

Microsoft.Extensions.Http.Resilience Breaks Application Insights Integration #5421

Closed
phil000 opened this issue Sep 12, 2024 · 15 comments
Closed
Assignees
Labels
area-resilience bug This issue describes a behavior which is not expected - a bug. bug-external

Comments

@phil000
Copy link

phil000 commented Sep 12, 2024

Description

This may seem like an obscure one, but it's easily reproducible.

2 months ago we upgraded 2 solutions to Microsoft.Extensions.Http.Resilience 8.9.0. We didn't notice at the time but we have now noticed both projects have had all App Insights integration suppressed - all Traces, Requests and Dependencies stopped.

We have upgrade to Microsoft.Extensions.Http.Resilience 8.9.1 and the issue is reproducible in the local development environment.

NuGetSolver tool didn't really add much info.

Reproduction Steps

Add Microsoft.Extensions.Http.Resilience 8.9.1 to solution. App insights integration is disabled.
Remove Microsoft.Extensions.Http.Resilience 8.9.0 from solution App insights integration is working again.

Expected behavior

App insights integration is working

Actual behavior

App insights integration is disabled

Regression?

No

Known Workarounds

Remove Microsoft.Extensions.Http.Resilience

Configuration

.NET Core
Version: 8.0.8
Architecture: x64

Other information

No response

@phil000 phil000 added bug This issue describes a behavior which is not expected - a bug. untriaged labels Sep 12, 2024
@RussKie
Copy link
Member

RussKie commented Sep 12, 2024

2 months ago we upgraded 2 solutions to Microsoft.Extensions.Http.Resilience 8.9.0.

😲 8.9.0 was only released a week ago...

image

@RussKie
Copy link
Member

RussKie commented Sep 12, 2024

Could you please provide a small stand-alone repro?

@RussKie RussKie added area-resilience waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. labels Sep 12, 2024
@phil000
Copy link
Author

phil000 commented Sep 12, 2024

Sorry the original upgrade was to "Microsoft.Extensions.Http.Resilience" Version="8.6.0"
That had a bug where synchronous methods of HttpClient were not incorporated into retries so we waited for that fix (#5236).
We are now on version 8.9.1 and the issue of breaking App Insights that started for us with 8.6.0 still persists.
I will see if I can provide a small web project that exhibits the issue.

@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Sep 12, 2024
@phil000
Copy link
Author

phil000 commented Sep 13, 2024

Can you provide an email address where I can send the repo?
It isn't something we can post publicly.

@RussKie
Copy link
Member

RussKie commented Sep 13, 2024

You can submit your repro sample via the VS Feedback (an icon on the top RHS in VS or via via https://developercommunity.visualstudio.com/dotnet directly). There you'll be able to attach a zip archive and all the necessary repro instructions. Once submitted, please link the issue here so we can route it to our team.

Please note, that we're not able to debug customer apps (unless you have a service agreement, in which case you need to speak with your CSR), that's why we ask for small standalone repros.

@phil000
Copy link
Author

phil000 commented Sep 13, 2024

The issue comes down to adding or removing this line, in a very minimal solution with only 3 NuGet package references.

With this AddResilienceHandler() all App Insights integration stops. Without it, the logging works as expected.

//TODO: Add or remove the 'AddResilienceHandler' call and it prevents App Insights integration from working.
bool wantAppInsightsWorking = false;
if (!wantAppInsightsWorking)
{
	builder.AddResilienceHandler(typeof(TClass).Name, builder =>
	{
		builder.AddRetry(new HttpRetryStrategyOptions
		{
			MaxRetryAttempts = 6,
			UseJitter = true,
			ShouldRetryAfterHeader = true,
			Delay = TimeSpan.FromMilliseconds(100),
			MaxDelay = TimeSpan.FromSeconds(10),
			BackoffType = DelayBackoffType.Exponential
		});
	});
}
<PackageReference Include="Microsoft.ApplicationInsights.AspNetCore" Version="2.22.0" />
<PackageReference Include="Microsoft.Extensions.Http.Resilience" Version="8.9.1" />
<PackageReference Include="Newtonsoft.Json" Version="13.0.3" />

@phil000
Copy link
Author

phil000 commented Sep 13, 2024

Here's the minimalist repo.

Steps to Reproduce:

  1. Add or remove the call to AddResilienceHandler found in WebsiteStartupBuilder.cs
  2. Run the web app
  3. Observe that App Insights logging is \ is not in the VS Output window. You'll see something like "Application Insights Telemetry (unconfigured):" when it's logging successfully, but Output window is completely empty when it is not working.

Repo.Web.zip

@RussKie
Copy link
Member

RussKie commented Sep 13, 2024

It looks like this call somehow affects the AI registrations:

_ = builder.Services.AddResiliencePipeline<HttpKey, HttpResponseMessage>(key, (builder, context) => configure(builder, new ResilienceHandlerContext(context)));

@martintmk @iliar-turdushev any thoughts?

@iliar-turdushev
Copy link
Contributor

@phil000 Thank you for reporting this. We'll investigate and get back to you.

@iliar-turdushev
Copy link
Contributor

The ticket is a duplicate of the following one #5222. The issue is caused by AppInsights. Here is detailed explanation of the issue #5222 (comment).

To fix the issue you have to reorder registration of AppInsights and Http.Resilience, in particular, first register AppInsights, and then Http.Resilience. For example, in the sample you provided you need to use the following order:

public void ConfigureServices(IServiceCollection services, IConfiguration configuration, bool isDevelopment)
{
    AddControllers(services);

    // First - AppInsights.
    AddTelemetry(services, isDevelopment);

    // Then - Http.Resilience and any other services that use Keyed Services feature of DI
    // https://learn.microsoft.com/en-us/aspnet/core/fundamentals/dependency-injection?view=aspnetcore-8.0#keyed-services.
    AddHttpClients(services);
}

@phil000 Please, confirm that the workaround works for you. If it does then we'll close the issue. Thank you.

@phil000
Copy link
Author

phil000 commented Sep 15, 2024

Confirming that reordering the dependency registration solves the issue for us.
We have added a unit test to check for the presence of TelemetryClient in the collection after the service provider is built.
Please close the case.

@RussKie
Copy link
Member

RussKie commented Sep 15, 2024

The AI bug is here: microsoft/ApplicationInsights-dotnet#2879

@RussKie
Copy link
Member

RussKie commented Sep 15, 2024

Duplicate of #5222

@RussKie RussKie marked this as a duplicate of #5222 Sep 15, 2024
@RussKie
Copy link
Member

RussKie commented Sep 15, 2024

@iliar-turdushev could you please update the package's readme with this known issue?

@iliar-turdushev
Copy link
Contributor

Sure, I will. Two customers already faced the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-resilience bug This issue describes a behavior which is not expected - a bug. bug-external
Projects
None yet
Development

No branches or pull requests

3 participants