Skip to content

Upgrade AzureMonitor to Microsoft.ApplicationInsights 3.x - #566

Merged
elinohlsson merged 3 commits into
ActiveLogin:mainfrom
okalangkenneth:feature/applicationinsights-v3-556
Oct 16, 2026
Merged

elinohlsson merged 3 commits into
ActiveLogin:mainfrom
okalangkenneth:feature/applicationinsights-v3-556

Conversation

@okalangkenneth

Copy link
Copy Markdown
Contributor

Fixes #556

Application Insights 3.x removed the metrics parameter from TrackEvent and TrackException, so BankIdApplicationInsightsEventListener threw MissingMethodException when an app used 3.x. This upgrades the package to 3.x as agreed in the issue.

Changes

AzureMonitor package

  • Microsoft.ApplicationInsights 2.23.0 → 3.1.2
  • Listener uses TrackEvent(name, properties) and TrackException(exception, properties)
  • AL_User_AgeHint is sent as a string property (invariant culture) in customDimensions, still gated by LogUserPersonalIdentityNumberHints
  • Connection string overload keeps new TelemetryConfiguration() (not the shared CreateDefault()), and the configuration and TelemetryClient are created once via Lazy<TelemetryClient> since the listener is transient

Docs

  • BREAKINGCHANGES.md: new 13.0.0 section (heading marked TBD, please adjust) covering the 3.x requirement, the connection string requirement and the age hint move, with before/after KQL
  • monitor.md: "Average age" query reads customDimensions
  • bankid.md: InstrumentationKey wording changed to connection string

Samples

  • Microsoft.ApplicationInsights.AspNetCore 2.23.0 → 3.1.2, and Microsoft.Extensions.Logging.ApplicationInsights removed from the ServerSample (no 3.x release; Microsoft's migration guide says to remove it)
  • 3.x throws at startup without a connection string, so the samples now only register Application Insights and the event listener when one is configured. appsettings use ConnectionString instead of InstrumentationKey, with fallback to APPLICATIONINSIGHTS_CONNECTION_STRING
  • ServerSample _LayoutMaster.cshtml only renders the JavaScript snippet when it's registered
  • AzureProvisioningSample ARM template sets APPLICATIONINSIGHTS_CONNECTION_STRING instead of APPINSIGHTS_INSTRUMENTATIONKEY

Testing

  • Solution builds with 0 warnings, all 319 tests pass
  • Standalone.MvcSample starts and serves pages with no connection string configured
  • Ran Standalone.MvcSample against a test Application Insights resource with the simulated BankID environment, once with the default overload (TelemetryClient from DI) and once with the connection string overload. Both exported all events, and CollectCompleted / AspNetAuthenticateSuccess carried AL_User_AgeHint = 27 in customDimensions for test PIN 990807-2391:
ActiveLogin_BankId

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The dedicated telemetry pipeline needs lifecycle ownership for shutdown flushing and disposal.

1 open finding
What changed in this PR

Migrates BankID AzureMonitor integration to Application Insights 3.x, addressing the compatibility failure reported in #556.

Changes:

  • Updates telemetry APIs and sends age hints as string properties.
  • Makes sample telemetry conditional on connection-string configuration.
  • Updates provisioning and migration documentation.
File Description
src/​ActiveLogin.Authentication.BankId.AzureMonitor/​BankIdBuilderAzureMonitorExtensions.cs Shares the dedicated telemetry client.
src/​ActiveLogin.Authentication.BankId.AzureMonitor/​BankIdApplicationInsightsEventListener.cs Updates tracking APIs and age-hint storage.
src/​ActiveLogin.Authentication.BankId.AzureMonitor/​ActiveLogin.Authentication.BankId.AzureMonitor.csproj Upgrades Application Insights.
samples/​Standalone.MvcSample/​Standalone.MvcSample.csproj Upgrades telemetry dependency.
samples/​Standalone.MvcSample/​Program.cs Registers telemetry conditionally.
samples/​Standalone.MvcSample/​appsettings.json Uses connection-string configuration.
samples/​IdentityServer.ServerSample/​Views/​Shared/​_LayoutMaster.cshtml Makes telemetry script optional.
samples/​IdentityServer.ServerSample/​Program.cs Registers telemetry conditionally.
samples/​IdentityServer.ServerSample/​IdentityServer.ServerSample.csproj Upgrades telemetry and removes legacy logging package.
samples/​IdentityServer.ServerSample/​appsettings.json Uses connection-string configuration.
samples/​IdentityServer.ClientSample/​Program.cs Registers telemetry conditionally.
samples/​IdentityServer.ClientSample/​IdentityServer.ClientSample.csproj Upgrades telemetry dependency.
samples/​IdentityServer.ClientSample/​appsettings.json Uses connection-string configuration.
samples/​AzureProvisioningSample/​ActiveLogin.json Provisions the connection-string setting.
docs/​articles/​monitor.md Updates the age-hint query.
docs/​articles/​bankid.md Documents connection-string configuration.
BREAKINGCHANGES.md Documents migration requirements and query changes.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The migration and resource ownership changes are consistent, with no unresolved blocking findings.

1 open finding

🧠 Review effort: Balanced

@elinohlsson elinohlsson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. I checked out the branch and tested it, including the latest commit (5f992a6), on a deployed Azure App Service (.NET 10) with a real Application Insights resource. I used the connection-string overload (via a temporary local change to the Standalone.MvcSample, not part of the PR) and disabled the platform's auto-instrumentation agent.

Verified at runtime

  • Events are exported on Microsoft.ApplicationInsights 3.1.2 with no MissingMethodException:
    • AspNetChallengeSuccess
    • AuthSuccess
    • CollectCompleted
    • AspNetAuthenticateSuccess
    • CollectFailure (user cancel)
    • CancelFailure
  • AL_User_AgeHint arrives in customDimensions as a string and casts back with toint(customDimensions.AL_User_AgeHint). Other properties (product name and version, API environment, order reference, device details) are also present.
  • Shutdown flush: in 3 of 3 runs, a login followed by an immediate app stop had its final event present. The 3.x exporter also sends in the background, so this is consistent with the fix but not conclusive on its own, since I didn't compare against the earlier build.

Not verified

  • The failing TrackException(exception, properties) path was verified by build only, not exercised at runtime.
  • I haven't deployed the AzureProvisioningSample ARM template. The change there (APPLICATIONINSIGHTS_CONNECTION_STRING from reference(...).ConnectionString) looks right, but I didn't check that the value resolves against the 2015-05-01 Application Insights API version.

Also

  • The 13.0.0 heading in BREAKINGCHANGES.md is marked TBD. I'll set it when bumping the version number in a separate PR, so this can be merged as is.
  • Out of scope for this PR, and fine to handle separately: AzureProvisioningSample still targets .NET 6.0 (netFrameworkVersion is v6.0 and the client site installs the ASP.NET Core 6.0 runtime extension), while the samples target .NET 10.
  • The dashboard queries in ActiveLogin-Monitor.json already use customDimensions, so the age hint change doesn't affect them.

@elinohlsson

Copy link
Copy Markdown
Contributor

Thanks a lot for the contribution, @okalangkenneth! 🙏

This was a thorough upgrade. Moving to Application Insights 3.x, updating the docs and BREAKINGCHANGES.md, and adapting the samples were all handled carefully. The PR description was clear, which made the review much easier. Thanks for also picking up the review feedback on the telemetry lifecycle so quickly.

Merging now. 🚀

@elinohlsson
elinohlsson merged commit f3db645 into ActiveLogin:main Oct 16, 2026
17 checks passed
@elinohlsson elinohlsson added this to the Next Release milestone Oct 16, 2026
@okalangkenneth
okalangkenneth deleted the feature/applicationinsights-v3-556 branch October 20, 2026 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upgrade ActiveLogin.Authentication.BankId.AzureMonitor to use Microsoft.ApplicationInsights v3

3 participants