Ensure Heartbeat properties for App Services is included in AspNetCore by d3r3kk · Pull Request #627 · microsoft/ApplicationInsights-aspnetcore

d3r3kk · 2018-03-30T16:34:15Z

Extension of issue microsoft/ApplicationInsights-dotnet-server#868 "Enabling Heartbeat in App Services..." in .NET Core scenarios.

I ran Unit Tests locally.
Changes in public surface reviewed
- Add EnableHeartbeat to ApplicationInsightsServiceOptions, reviewed with @cijothomas
CHANGELOG.md updated with one line description of the fix, and a link to the original issue.

d3r3kk · 2018-03-30T16:52:23Z

Note that PR microsoft/ApplicationInsights-dotnet-server#873 must be submitted and available on myget before I will be able to successfully build this one. I've started this PR to facilitate code review prior to that condition being met.

EDIT: Changed PR to more current one.

d3r3kk · 2018-03-30T16:53:48Z

src/Microsoft.ApplicationInsights.AspNetCore/Microsoft.ApplicationInsights.AspNetCore.csproj

+    <PackageReference Include="Microsoft.ApplicationInsights.DependencyCollector" Version="2.6.0-beta2-build12842" />
+    <PackageReference Include="Microsoft.ApplicationInsights.PerfCounterCollector" Version="2.6.0-beta2-build12842" />
+    <PackageReference Include="Microsoft.ApplicationInsights.WindowsServer" Version="2.6.0-beta2-build12842" />
+    <PackageReference Include="Microsoft.ApplicationInsights.WindowsServer.TelemetryChannel" Version="2.6.0-beta2" />


These changes will not be submitted. Once the PR in -dotnet-server completes I will update these references to the latest release there.

d3r3kk · 2018-03-30T16:54:23Z

NuGet.config

    <clear />
    <add key="NuGet" value="https://api.nuget.org/v3/index.json" />
    <add key="applicationinsights" value="https://www.myget.org/F/applicationinsights/api/v3/index.json" />
+    <add key="Local" value="D:\dev\github\ai\bin\Debug\Nuget" />


This change will not be submitted. Only here for local development work while I wait for -dotnet-server to get posted to myget.

I've removed this now, and have updated the dependencies to a myget package representing a more valid candidate for the next beta release here.

d3r3kk · 2018-04-04T14:57:43Z

@cijothomas if you could take a look at this and provide a review I'd be grateful. Would very much like this to hit 2.6.0-beta3 timeframe.

Note the build should complete properly now, it's based off of MyGet packages from my PR in -dotnet-server.

EDIT: Added Note.

SergeyKanzhelev

CC: @cijothomas

SergeyKanzhelev · 2018-04-04T16:13:24Z

src/Microsoft.ApplicationInsights.AspNetCore/Extensions/ApplicationInsightsExtensions.cs


-                services.AddSingleton<ITelemetryInitializer, AzureWebAppRoleEnvironmentTelemetryInitializer>();
-                services.AddSingleton<ITelemetryInitializer, DomainNameRoleInstanceTelemetryInitializer>();
+                services.AddSingleton<ITelemetryInitializer, ApplicationInsights.AspNetCore.TelemetryInitializers.AzureWebAppRoleEnvironmentTelemetryInitializer>();


Can this be taken from the WindowsServer package? If not - aspnetcore and web might set different values for role name and role instance

It most certainly can. I've added issue #624 to track this as a separate issue. There will need to be some work to consolidate the modules properly as the implementations have diverged.

EDIT: Updated to correct issue (I'd opened a duplicate)

SergeyKanzhelev · 2018-04-04T16:14:24Z

...icrosoft.ApplicationInsights.AspNetCore/Implementation/TelemetryConfigurationOptionsSetup.cs

+                // Disable heartbeat if user sets it (by default it is on)
+                if (!this.applicationInsightsServiceOptions.EnableHeartbeat)
+                {
+                    foreach (var module in TelemetryModules.Instance.Modules)


Will this singleton be initialized at all in aspnetcore case?

Yes. I'm adding tests to prove that now.

Unfortunately ,no from my experiments and this issue
#506

Tests added. Singleton is there, DiagnosticsTelemetryModule is there, and that is the IHeartbeatPropertyManager implementation at present.

Let's talk about a better public API for heartbeat in 2.7 timeframe.

Just lucky(?) that only modules added from base sdk is present. (DiagnosticsTelemetryModule is added by base sdk)

Yes, this is just lucky and we are indeed relying on the implementation details in the base SDK here.

Tests that I added are failing outside of the Visual Studio environment, I'm investigating now.

cijothomas · 2018-04-04T16:41:37Z

CHANGELOG.md

 # Changelog

+## Version 2.6.0-beta1
+- [Add support for Heartbeat properties in AspNetCore. Add App Services and Azure Instanace Metedata heartbeat provider modules by default, support disable of heartbeats.](https://github.com/Microsoft/ApplicationInsights-aspnetcore/pull/627)


Metadata - spellcheck.
Add support for Heartbeat properties - we already had this support right? So just mention about the ability to turn-off hearbeat.

cijothomas · 2018-04-04T17:47:57Z

...icrosoft.ApplicationInsights.AspNetCore/Implementation/TelemetryConfigurationOptionsSetup.cs

+                    {
+                        if (module is IHeartbeatPropertyManager hbeatMan)
+                        {
+                            hbeatMan.IsHeartbeatEnabled = false;


Setting IsHeartbeatEnabled to false will stop the HeartBeats collected/sent by the AppServicesHeartbeatTelemetryModule, AzureInstanceMetadataTelemetryModule modules as well - right?

Yes, this is the intention. Setting IsHeartbeatEnabled to false should disable heartbeats altogether.

cijothomas

Blocking to prevent accidental merge.
Tests need to be addressed.

d3r3kk · 2018-04-05T07:20:51Z

How do I debug my xUnit tests in .NET Core?

I can see the tests failing on the command line dotnet test Microsoft.ApplicationInsights.AspNetCore.Tests.csproj -l trx --no-build but I cannot seem to get this working in Visual Studio (probably due to issue #547). Unfortunately I'm forced to give up at this point and will resume sorting it out on Friday.

I've instrumented a .NET Core web app and will see if the thing behaves as expected tomorrow.

Is there any guidance that I can refer to on this matter? Multi-targeted framework solutions should probably be working in Visual Studio 2017...

cijothomas · 2018-04-05T16:20:47Z

@d3r3kk What specific issue are you hitting with VS debugging? VS is not showing the tests in test runner/explorer? It shows the tets from the 1st target only. So if you are debugging net46, change the order so that net46 appears first.
https://github.com/Microsoft/ApplicationInsights-aspnetcore#testingdebugging

- only enable heartbeat collection for app services in this PR

- Using signed package based off of PR microsoft/ApplicationInsights-dotnet-server#873

…orks

- fix issues with static initialization in the unit test expectations in XUnit - we must sort this out in 2.7.0-betaX by ensuring we can initialize mutliple times during testing - (and ensure we can debug in VS during unit test passes)

d3r3kk · 2018-04-08T02:22:40Z

@cijothomas Please re-review this change. I think I understand why the tests are failing but have not been able to step through in the debugger. Not sure why we are having this trouble, but moving to vstest might be superior in the long run.

Tests are passing now. Other comments are addressed.

d3r3kk · 2018-04-10T01:12:08Z

We are in. @cijothomas and @MS-TimothyMothra to help sort out versioning and such...

d3r3kk requested a review from cijothomas March 30, 2018 16:52

d3r3kk self-assigned this Mar 30, 2018

d3r3kk added this to the 2.3.0-beta1 milestone Mar 30, 2018

d3r3kk commented Mar 30, 2018

View reviewed changes

SergeyKanzhelev approved these changes Apr 4, 2018

View reviewed changes

cijothomas reviewed Apr 4, 2018

View reviewed changes

cijothomas previously requested changes Apr 5, 2018

View reviewed changes

d3r3kk added 9 commits April 7, 2018 01:43

Add heartbeats to the aspdotnet service options for AppInsights

a3ced0b

Add heartbeat modules to the initialization of the AspNetCore libs

434694f

Fix issue with including the WindowsServer namespace in .NET Core builds

a75777f

Simplify the PR to only do what I need, nothing else

469360f

- only enable heartbeat collection for app services in this PR

Update PR with packages only from MyGet

be3bd3d

- Using signed package based off of PR microsoft/ApplicationInsights-dotnet-server#873

Update changelog

263a298

Fix CHANGELOG & add a couple of unit tests to confirm configuration w…

b52dd87

…orks

Update test for registration of modules

dc6e1e7

Correct tests for Heartbeat additions

491bcad

- fix issues with static initialization in the unit test expectations in XUnit - we must sort this out in 2.7.0-betaX by ensuring we can initialize mutliple times during testing - (and ensure we can debug in VS during unit test passes)

d3r3kk force-pushed the dekeeler/appservice-heartbeats branch from 9b326b6 to 491bcad Compare April 7, 2018 08:51

Remove extraneous whitespace

54d1211

d3r3kk merged commit a7c13ab into develop Apr 10, 2018

d3r3kk deleted the dekeeler/appservice-heartbeats branch April 10, 2018 01:11

Conversation

d3r3kk commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

d3r3kk commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d3r3kk commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d3r3kk Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cijothomas left a comment

Choose a reason for hiding this comment

Uh oh!

d3r3kk commented Apr 5, 2018

Uh oh!

cijothomas commented Apr 5, 2018

Uh oh!

d3r3kk commented Apr 8, 2018

Uh oh!

d3r3kk commented Apr 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

d3r3kk commented Mar 30, 2018 •

edited

Loading

d3r3kk commented Mar 30, 2018 •

edited

Loading

d3r3kk commented Apr 4, 2018 •

edited

Loading

d3r3kk Apr 4, 2018 •

edited

Loading