search

Azure / azure-functions-host

The host/runtime that powers Azure Functions
https://functions.azure.com
MIT License
1.94k stars 441 forks source link

Assemblies with types added to DI container are not marked as runtime assemblies, making them potentially inaccessible #4952

Open AtOMiCNebula opened 5 years ago

AtOMiCNebula commented 5 years ago

When APPINSIGHTS_INSTRUMENTATIONKEY is set, the WebJobs SDK adds AppInsights types to the dependency injection container. However, Microsoft.ApplicationInsights is not listed in the function host's runtimeassemblies.json, so FunctionAssemblyLoadContext does not ensure the runtime's version is always the one that's used.

The right thing happens in the default case, but in the event that the function app is including a function.deps.json in its bundle (which my app is for other reasons :smile:), then function.deps.json takes priority and the runtime's Microsoft.ApplicationInsights assembly is ignored. This effectively blocks AppInsights types from being retrieved via DI (making https://docs.microsoft.com/en-us/azure/azure-functions/functions-monitoring#log-custom-telemetry-in-c-functions a hard doc to follow :innocent:).

I wrote a function to look at all ServiceTypes added to the DI container that are from a non-runtime assembly, and found the following list of types:

I think that the three assemblies above need to be added to runtimeassemblies.json, in order to unblock scenarios

Investigative information

Template fields didn't apply, but I did put together a minimal function app repro here: https://github.com/AtOMiCNebula/FunctionHostAssemblyWoes (make sure you add an APPINSIGHTS_INSTRUMENTATIONKEY value to your local.settings.json/etc!)

The BrokenConsumer function shows the issue with TelemetryConfiguration.

The DIChecker function enumerates all service types added to the IServiceCollection and lists out types that are in assemblies not listed in runtimeassemblies.json.

Repro steps

Provide the steps required to reproduce the problem:

  1. Create new HTTP triggered function in empty project.
  2. Add APPINSIGHTS_INSTRUMENTATIONKEY to relevant places.
  3. Follow https://docs.microsoft.com/en-us/azure/azure-functions/functions-monitoring#log-custom-telemetry-in-c-functions to pull a TelemetryConfiguration from DI.
  4. Update function project to include a function.deps.json.

Expected behavior

Provide a description of the expected behavior.

Function should run successfully.

Actual behavior

Provide a description of the actual behavior observed.

Through step 2, the function app does not have trouble acquiring a TelemetryConfiguration from the DI container. After step 3, function launch fails.

Known workarounds

fabiocav commented 5 years ago

Removing triage labels while we work through the proposed changes, but this may need to catch the 3.x release train

/cc @brettsam

AtOMiCNebula commented 5 years ago

Alrighty. I ended up finding a workaround that will work for my scenario, by adding this to my project's Startup.cs file:

builder.Services.AddSingleton(TelemetryConfiguration.CreateDefault());

I can still consume a TelemetryConfiguration the way the docs page suggests, though it won't have any of the WebJobs-specific telemetry initializers/etc. It does still link up with operations/activity contexts properly, and that's the main thing I was not excited to have to recreate myself. Hopefully when this gets fixed for real, then I can remove this kludge and/or the real injected type will just come through over mine automatically. :)

nhwilly commented 5 years ago

FWIW. I don't have anything special at all. Just following the instructions and I can't make any of these examples work.

If I don't put anything in startup, the DI fails.

Everything I put in startup gets this:

Microsoft.ApplicationInsights: The type initializer for 'Microsoft.ApplicationInsights.Extensibility.TelemetryConfiguration' threw an exception. Microsoft.ApplicationInsights: Method not found: 'Boolean System.Diagnostics.Activity.get_ForceDefaultIdFormat()'.

I wanted to use the TrackEvent capability, but I'm having real trouble getting any of this to work in Functions v2 and .net Core.

Please tell me I don't have to write a custom logger.

brettsam commented 5 years ago

Are you referencing ApplicationInsights 2.11? If so, roll back to 2.10 for now. We're in the process of moving forward.

I'd recommend referencing our package so you don't accidentally move forward past what is supported by Functions: https://www.nuget.org/packages/Microsoft.Azure.WebJobs.Logging.ApplicationInsights/

nhwilly commented 5 years ago

@brettsam I'll try that. I was just following the steps and used the package manager to install 2.8.0

Should I use be using Microsoft.ApplicationInsights or Microsoft.ApplicationInsights.AspNetCore?

nhwilly commented 5 years ago

OK, it's working again. That's what I needed, I just upgraded the Cosmos SDK today and was showing two sets of AppInsights in my Nuget list. I decided to tidy up. Thanks for the quick response.

nhwilly commented 5 years ago

Are you referencing ApplicationInsights 2.11? If so, roll back to 2.10 for now. We're in the process of moving forward.

I'd recommend referencing our package so you don't accidentally move forward past what is supported by Functions: https://www.nuget.org/packages/Microsoft.Azure.WebJobs.Logging.ApplicationInsights/

Wow. Missed that somehow and just was following the basic AI instructions. I'll bet when I migrated from Asp.Net Core to Functions, I just dragged over the same approach.

Huge help. Working great now.

brettsam commented 5 years ago

Glad you're unblocked! This is an issue every time App Insights moves ahead. We're trying to update quickly, but there's always at least a couple of weeks where they don't match as we wait for our deployment to go out to production. That's why referencing our package (and not App Insights directly) can help. We'll move our package forward when the latest is deployed.

AtOMiCNebula commented 5 years ago

@brettsam / @fabiocav with .NET Core 3.0 in preview for Functions today, was this the opening of the new host version that you wanted to wait on this until? Is this being addressed some other way, or is now a good time to pursue the fix? I want to make sure this issue doesn't get lost. :)

AtOMiCNebula commented 4 years ago

@fabiocav / @brettsam Any updates here? This still repros in 3.0, and I suspect will become more important with the function.deps.json being placed automatically now.

AtOMiCNebula commented 4 years ago

@fabiocav / @brettsam It looks like you've started to address this with #5551, but there are still other assemblies affected by this. I reran my checker on v3.0.13107, and it looks like all three as originally quoted are still missing, so even with #5551, Microsoft.AI.PerfCounterCollector and Microsoft.Azure.WebJobs.Host.Storage will still be missing.

Should I update the PR I had originally offered up for this (#4953) to match what was merged in #5551 (minorMatchOrLower -> runtimeVersion), or do you still want to pursue this fix yourselves?

AtOMiCNebula commented 4 years ago

@fabiocav / @brettsam ping, my most recent comment still appears to be the current state of things.

AtOMiCNebula commented 4 years ago

@fabiocav closed my PR despite repeated pings here for attention, so I am taking the hint and am going to stop pushing on this. I am leaving the issue open though, since the issue is not fixed. ☹️

As of this writing, 3.0.13139.0 is still injecting types into the service container that are not being redirected properly, making them effectively uninjectable. Specifically:

fabiocav commented 4 years ago

@AtOMiCNebula I did go through a few PRs that were stale in a batch and had to close a few. We can absolutely review this, but with recent branch target changes, we needed to make sure PRs were targeting the right branches (i.e. v3 is now dev).

This was absolutely no intended as an indication of our intention to take that work. Happy to iterate on that with you.