microsoft / ApplicationInsights-ServiceFabric

ApplicationInsights SDK for ServiceFabric projects
MIT License
63 stars 26 forks source link

Update ServiceFabric packages to latest public NuGet package #85

Closed lucas-burdell-karmak closed 6 years ago

msftclas commented 6 years ago

CLA assistant check
All CLA requirements met.

yantang-msft commented 6 years ago

@lucas-burdell-karmak Could you tell me what's the intention of this change? Is it because something is not working for you?

lucas-burdell-karmak commented 6 years ago

@yantang-msft I am unable to add AppInsights to an existing ServiceFabric application that is using the latest ServiceFabric packages (adding AI using the "Add AppInsights Telemetry" context menu option on Connected Services). The AI installer says something like "NuGet restore failed, rolling back changes" and then fails to add AppInsights.

yantang-msft commented 6 years ago

What's the exact error message? Are you able to resolve that by explicitly referencing the package in your project? We have seen couple cases regarding nuget restore failure, e.g., https://github.com/Microsoft/ApplicationInsights-ServiceFabric/issues/79. That's unfortunately a limitation of nuget itself. But it could be different in your case.

lucas-burdell-karmak commented 6 years ago

Here's the error message in a test project:

image

Removing all existing ServiceFabric and AppInsights NuGet packages allows me to successfully install AppInsights. I will try referencing this project directly to confirm that the issue involves the new ServiceFabric packages.

yantang-msft commented 6 years ago

@lucas-burdell-karmak This error message is too short for figuring out the real problem... It's good you find the workaround. And I confirm I can repro this issue too. Basically it I install this AI.SF package first and then goes to this UI, it will fail. @gardnerjr Do you know who owns this UI and can take a look?

gardnerjr commented 6 years ago

If that's what the ui shows, then that's the exact error message we got from the provider that installed the packages (in this case the service fabric extension to AI?) this generally means that some dependency was missing or there were version mismatches somewhere?

the extension might be trying to install an older version than you installed manually so the versions don't matchup?

@vutran01 could maybe take a look?

yantang-msft commented 6 years ago

I think this error message is trimmed. For other cases we see that failed to restore package, we see this kind of message Error NU1107 Version conflict detected for Microsoft.ServiceFabric.Diagnostics.Internal. Install/reference Microsoft.ServiceFabric.Diagnostics.Internal 3.2.176 directly to project Web1 to resolve this issue. Web1 -> Microsoft.ServiceFabric.AspNetCore.Kestrel 3.2.176 -> Microsoft.ServiceFabric.AspNetCore.Abstractions 3.2.176 -> Microsoft.ServiceFabric.Services 3.2.176 -> Microsoft.ServiceFabric.Diagnostics.Internal (= 3.2.176) Web1 -> Microsoft.ApplicationInsights.ServiceFabric.Native 2.1.1 -> Microsoft.ServiceFabric.Services, which gives us the exact information where the conflict is. @vutran01 Can you see if you can get more information under the cover?

vutran01 commented 6 years ago

The extension is probably trying to install an older appinsights nuget version than the one you already installed. What's your scenario? You could try to add appinsights first and then update to the latest ServiceFabric packages. You could check the output window for any additional errors but from the bitmap there doesn't seem to be any.

yantang-msft commented 6 years ago

@vutran01 You're right, this happens if I install AI.SF first (which pulls AI 2.4), and the extension is trying to install 2.3. Yes, there is a workaround, but the extension should just handle this basic case. And that's one of the core functionality of nuget, i.e., resolve the version automatically.

gardnerjr commented 6 years ago

"The extension" here is not directly the application insights tools though. There's a ServiceFabric extension to the application insights extension that we ask "are your packages installed?" and it must be saying "no", so we tell it to install itself, which then fails since it is already installed.

the fix here appears to be many things: 1) the service fabric provider is improperly checking for if it is already installed/can be updated. 2) the service fabric provider should update to a newer version of its own nuget packages

yantang-msft commented 6 years ago

@BigMorty Do you know who can help with the issue in SF tool?

BigMorty commented 6 years ago

@dbreshears - Any idea on this issue?

BigMorty commented 6 years ago

Adding @ravipal who may have some insight into this.

ravipal commented 6 years ago

As part of "Add AppInsights Telemetry" SF tools extension installs the 2.3 version of Microsoft.ApplicationInsights. How is the service project getting the 2.4 version of this nuget package before invoking the "Add AppInsights Telemetry"?

yantang-msft commented 6 years ago

@ravipal By installing the AI.SF nuget, which depends on AI 2.4.

ravipal commented 6 years ago

Why is AI.SF nuget package added to service project? May be I don't get the overall scenario. What needs to happen on SF tooling side to fix this? Should we update to use 2.4 version of AI? This update will address only the future version of SF Tools extension, users using old version of the tooling will still run into this issue.

yantang-msft commented 6 years ago

Customer added this package manually. Since the AI 2.4 package is already there, ideally the enablement scenario should pass instead of pop up the error dialog and roll back changes. According to this comment, this error is thrown from SF, but I could be wrong.

ravipal commented 6 years ago

If AI package is already installed, we don't show the "Add AppInsights Telemetry" option. image

This is for the project that didn't have AI nuget package installed. image

ravipal commented 6 years ago

@gardnerjr IAddApplicationInsightsProvider.CanAddToProject(IVsHierarchy vsHierarchy) implemented as "return _packageInstallerServices.IsPackageInstalled(hierarchy.DteProject, "Microsoft.ApplicationInsights");", so if the package is installed, the option to add AI is disabled.

yantang-msft commented 6 years ago

@ravipal In this particular case, the "AI" package is not installed directly, but a dependency of AI.SF package, and the "Add AppInsights Telemetry" option is there. So I believe this API _packageInstallerServices.IsPackageInstalled(hierarchy.DteProject, "Microsoft.ApplicationInsights"); is indeed returning false. However the installation failed for some reason (which looks like we are enforcing installing AI 2.3, instead of 2.3+).

ravipal commented 6 years ago

Got it. I will open a bug in SF tooling to address this.

yantang-msft commented 6 years ago

Close this PR as we identified this as a bug in VS extension.

lucas-burdell-karmak commented 6 years ago

Thank you all for the help!