[Proposal] Add token acquisition metrics that cover both successful and failed t… by ssmelov · Pull Request #5928 · AzureAD/microsoft-authentication-library-for-dotnet

ssmelov · 2026-04-15T15:18:44Z

Add token acquisition metrics that cover both successful and failed token acquisition outcomes and close an observability gap by recording IDP network latency for background refresh.

…oken acquisition outcomes

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds an opt-in “extended token acquisition metrics” mode to improve OpenTelemetry coverage across both successful and failed token acquisitions, and closes a gap by capturing IDP network latency for background refresh successes.

Changes:

Introduces V2 OTel histograms (total duration + HTTP duration) tagged with Succeeded / HttpStatusCode, gated by a new config flag.
Adds a public builder opt-in WithExtendedTokenAcquisitionMetrics() and threads the flag through OTel instrumentation APIs.
Logs HTTP duration for proactive background refresh successes.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/client/Microsoft.Identity.Client/TelemetryCore/TelemetryConstants.cs	Adds a new telemetry tag constant for HTTP status code.
src/client/Microsoft.Identity.Client/TelemetryCore/OpenTelemetry/NullOtelInstrumentation.cs	Updates no-op instrumentation to match new extended-metrics API surface.
src/client/Microsoft.Identity.Client/TelemetryCore/OpenTelemetry/IOtelInstrumentation.cs	Extends the instrumentation interface for V2 metrics + HTTP duration logging and failure context.
src/client/Microsoft.Identity.Client/Platforms/Features/OpenTelemetry/OtelInstrumentation.cs	Implements V2 histograms, extended/fallback emission logic, and updated failure metric recording.
src/client/Microsoft.Identity.Client/Internal/Requests/SilentRequestHelper.cs	Emits success HTTP duration for proactive background refresh; updates failure logging signature.
src/client/Microsoft.Identity.Client/Internal/Requests/RequestBase.cs	Captures total duration for failures and passes HTTP status code into failure telemetry.
src/client/Microsoft.Identity.Client/AppConfig/BaseAbstractApplicationBuilder.cs	Adds the public opt-in builder method for extended token acquisition metrics.
src/client/Microsoft.Identity.Client/AppConfig/ApplicationConfiguration.cs	Adds configuration toggle `IsExtendedTokenAcquisitionMetricsEnabled`.
src/client/Microsoft.Identity.Client/PublicApi/*/PublicAPI.Unshipped.txt	Declares the new public builder API across TFMs.

Copilot · 2026-04-15T15:26:58Z

+                int httpStatusCode = ex is MsalServiceException serviceEx ? serviceEx.StatusCode : 0;
+                LogFailureTelemetryToOtel(apiEvent, apiEvent.ApiErrorCode, httpStatusCode, requestStopwatch.ElapsedMilliseconds + measureTelemetryDurationResult.Milliseconds);


measureTelemetryDurationResult.Milliseconds is the millisecond component (0–999) of the TimeSpan, not the full duration. This will under-report total duration for telemetry whenever measureTelemetryDurationResult is >= 1 second. Use measureTelemetryDurationResult.TotalMilliseconds (cast/rounded to long as needed) instead of .Milliseconds when computing totalDurationInMs.

Copilot · 2026-04-15T15:26:59Z

+                    new(TelemetryConstants.Platform, platform),
+                    new(TelemetryConstants.ErrorCode, errorCode),
+                    new(TelemetryConstants.ApiId, apiEvent.ApiId),
+                    new(TelemetryConstants.CallerSdkId, apiEvent.CallerSdkApiId ?? string.Empty + "," + apiEvent.CallerSdkVersion ?? string.Empty),


The CallerSdkId value composition is incorrect due to ?? / + precedence and will also drop the version when CallerSdkApiId is non-null. Build the combined value explicitly (e.g., via interpolation or parentheses) so it always includes both parts with empty-string defaults.

Suggested change

new(TelemetryConstants.CallerSdkId, apiEvent.CallerSdkApiId ?? string.Empty + "," + apiEvent.CallerSdkVersion ?? string.Empty),

new(TelemetryConstants.CallerSdkId, $"{apiEvent.CallerSdkApiId ?? string.Empty},{apiEvent.CallerSdkVersion ?? string.Empty}"),

Copilot · 2026-04-15T15:26:59Z

+                s_durationInExtensionInMs.Value.Record(authResultMetadata.DurationCreatingExtendedTokenInUs,
+                    new(TelemetryConstants.MsalVersion, MsalIdHelper.GetMsalVersion()),
+                    new(TelemetryConstants.Platform, platform),
+                    new(TelemetryConstants.ApiId, apiId),
+                    new(TelemetryConstants.TokenSource, authResultMetadata.TokenSource),
+                    new(TelemetryConstants.CacheLevel, authResultMetadata.CacheLevel),
+                    new(TelemetryConstants.TokenType, authResultMetadata.TelemetryTokenType));


s_durationInExtensionInMs is defined as an “InMs” histogram (unit: "ms"), but the recorded value is DurationCreatingExtendedTokenInUs (microseconds). Convert to milliseconds before recording (or record into a microseconds-based histogram) to avoid emitting incorrect latency values.

Copilot · 2026-04-15T15:27:00Z

                    serviceBundle.PlatformProxy.OtelInstrumentation.LogFailureMetrics(
                        serviceBundle.PlatformProxy.GetProductName(),
+                        apiEvent,
                        ex.ErrorCode,
-                        apiEvent.ApiId,
-                        callerSdkId,
-                        callerSdkVersion,
-                        CacheRefreshReason.ProactivelyRefreshed,
-                        apiEvent.TokenType);
+                        ex.StatusCode,
+                        totalDurationInMs: 0,
+                        serviceBundle.Config.IsExtendedTokenAcquisitionMetricsEnabled);


In extended-metrics mode, OtelInstrumentation.LogFailureMetrics only records the V2 total-duration histogram when totalDurationInMs > 0. Passing totalDurationInMs: 0 here means background refresh failures won’t contribute to the new “cover failures” total-duration metric. If the goal is to cover failed outcomes for proactive refresh too, capture and pass a meaningful elapsed duration in this flow (or adjust the recording condition to allow 0 only if that’s an intentional signal).

Copilot · 2026-04-15T15:27:00Z

+            if (!isExtendedMetricsEnabled)
+            {
+                if (s_durationInHttp.Value.Enabled)
+                {
+                    s_durationInHttp.Value.Record(authResultMetadata.DurationInHttpInMs,
+                        new(TelemetryConstants.MsalVersion, MsalIdHelper.GetMsalVersion()),
+                        new(TelemetryConstants.Platform, platform),
+                        new(TelemetryConstants.ApiId, apiId),
+                        new(TelemetryConstants.TokenType, authResultMetadata.TelemetryTokenType));
+                }
+            }
+            else
+            {
+                if (s_durationInHttpV2.Value.Enabled)
+                {
+                    s_durationInHttpV2.Value.Record(authResultMetadata.DurationInHttpInMs,
+                        new(TelemetryConstants.MsalVersion, MsalIdHelper.GetMsalVersion()),
+                        new(TelemetryConstants.Platform, platform),
+                        new(TelemetryConstants.ApiId, apiId),
+                        new(TelemetryConstants.TokenType, authResultMetadata.TelemetryTokenType),
+                        new(TelemetryConstants.HttpStatusCode, 200));
+                }
+            }


The extended/non-extended branching and tag sets for HTTP duration are now duplicated between LogSuccessMetrics and LogSuccessHttpDuration. Consider centralizing the “record HTTP duration” logic into a single private helper to reduce the risk of the two paths drifting (e.g., tag changes, enablement checks, or future histogram revisions).

Copilot

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.

Copilot · 2026-04-16T18:18:14Z

                if (isBrokerConfigured && !isAccountSourceDeviceCodeFlow)
                {
                    _logger.Info("Broker is configured and enabled, attempting to use broker instead.");
+                    AuthenticationRequestParameters.RequestContext.ApiEvent.TokenSource = TokenSource.Broker;
                    var brokerResult = await _brokerStrategyLazy.Value.ExecuteAsync(cancellationToken).ConfigureAwait(false);

                    // fallback to local cache if broker fails


ApiEvent.TokenSource is set to Broker before attempting the broker flow, but it is never reset when the broker returns null and the code falls back to the local cache. This can mis-tag failure telemetry (and any other ApiEvent-based telemetry) as coming from the broker even when the cache path was used. Set TokenSource back to Cache before _clientStrategy.ExecuteAsync(...) or right before the "Attempting to acquire token using local cache" path.

Copilot · 2026-04-16T18:18:14Z

+                    case "MsalTotalDuration.2":
+                        Trace.WriteLine("Verify the metrics captured for MsalTotalDuration.2 histogram.");
+                        Assert.AreEqual(MetricType.Histogram, exportedItem.MetricType);
+
+                        expectedTags.Add(TelemetryConstants.MsalVersion);
+                        expectedTags.Add(TelemetryConstants.Platform);
+                        expectedTags.Add(TelemetryConstants.ApiId);
+                        expectedTags.Add(TelemetryConstants.CacheRefreshReason);
+                        expectedTags.Add(TelemetryConstants.TokenType);
+                        expectedTags.Add(TelemetryConstants.Succeeded);
+
+                        foreach (var metricPoint in exportedItem.GetMetricPoints())
+                        {
+                            AssertTags(metricPoint.Tags, expectedTags);
+                        }


VerifyMetrics expects MsalTotalDuration.2 to have only a small set of tags, but the implementation currently records additional tags (e.g., TokenSource/CacheLevel/ErrorCode). With the current AssertTags (exact tag count), this case will fail as soon as a test starts validating MsalTotalDuration.2. Align the expected tag list here with the actual emitted tags (or adjust the production tag set to match the intended contract).

Copilot · 2026-04-16T18:18:14Z

+                    new(TelemetryConstants.Platform, platform),
+                    new(TelemetryConstants.ErrorCode, errorCode),
+                    new(TelemetryConstants.ApiId, apiEvent.ApiId),
+                    new(TelemetryConstants.CallerSdkId, apiEvent.CallerSdkApiId ?? string.Empty + "," + apiEvent.CallerSdkVersion ?? string.Empty),


The CallerSdkId tag value construction uses ?? and string concatenation without parentheses, so it won’t reliably include both caller SDK id + version and can yield unexpected values (e.g., "," when id is null). Use an explicit format (e.g., interpolate both null-coalesced pieces) to avoid operator-precedence bugs and keep the tag value consistent.

Suggested change

new(TelemetryConstants.CallerSdkId, apiEvent.CallerSdkApiId ?? string.Empty + "," + apiEvent.CallerSdkVersion ?? string.Empty),

new(TelemetryConstants.CallerSdkId, $"{apiEvent.CallerSdkApiId ?? string.Empty},{apiEvent.CallerSdkVersion ?? string.Empty}"),

Copilot · 2026-04-16T18:18:15Z

                        new(TelemetryConstants.Platform, platform),
+                        new(TelemetryConstants.ApiId, apiEvent.ApiId),
+                        new(TelemetryConstants.TokenSource, apiEvent.TokenSource),
+                        new(TelemetryConstants.CacheLevel, string.Empty),


MsalTotalDuration.2 records the CacheLevel tag as string.Empty on failures. This makes the attribute type inconsistent with the success path (which uses the CacheLevel enum) and can split/invalidly aggregate metric streams in OTel exporters. Record CacheLevel.None (or another CacheLevel enum value) instead of a string.

Suggested change

new(TelemetryConstants.CacheLevel, string.Empty),

new(TelemetryConstants.CacheLevel, CacheLevel.None),

Copilot · 2026-04-16T18:18:15Z

+                        new(TelemetryConstants.TokenSource, authResultMetadata.TokenSource),
+                        new(TelemetryConstants.CacheLevel, cacheLevel),
+                        new(TelemetryConstants.CacheRefreshReason, authResultMetadata.CacheRefreshReason),
+                        new(TelemetryConstants.TokenType, authResultMetadata.TelemetryTokenType),
+                        new(TelemetryConstants.ErrorCode, string.Empty),


The extended MsalTotalDuration.2 histogram is documented/tested as being tagged mainly with Succeeded/CacheRefreshReason/TokenType, but the implementation also adds TokenSource, CacheLevel, and ErrorCode. If this is intentional, update the public XML docs/tests; if not, consider removing extra dimensions (especially ErrorCode) to avoid high-cardinality histograms and reduce telemetry cost.

Suggested change

new(TelemetryConstants.TokenSource, authResultMetadata.TokenSource),

new(TelemetryConstants.CacheLevel, cacheLevel),

new(TelemetryConstants.CacheRefreshReason, authResultMetadata.CacheRefreshReason),

new(TelemetryConstants.TokenType, authResultMetadata.TelemetryTokenType),

new(TelemetryConstants.ErrorCode, string.Empty),

new(TelemetryConstants.CacheRefreshReason, authResultMetadata.CacheRefreshReason),

new(TelemetryConstants.TokenType, authResultMetadata.TelemetryTokenType),

neha-bhargava · 2026-05-06T21:22:14Z

+                }
+                catch
+                {
+                    LastRequestDurationInMs = sw.ElapsedMilliseconds;


finally block might be a good place to place the duration here that will cover all the cases. Here and in the RequestBase.cs as well

I'd keep it scoped to the network‑bound portion — putting it in finally would also include response body read.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.

+                    serviceBundle.PlatformProxy.OtelInstrumentation.LogSuccessHttpDuration(
+                        serviceBundle.PlatformProxy.GetProductName(),
+                        apiEvent.ApiId,
+                        authResult.AuthenticationResultMetadata);


+                    if (requestContext.ApiEvent != null)
+                    {
+                        requestContext.ApiEvent.DurationInHttpInMs += _httpManager.LastRequestDurationInMs;


+
+            if (_isExtendedMetricsEnabled)
+            {
+                if (totalDurationInMs > 0 && s_durationTotalV2.Value.Enabled)


                        new(TelemetryConstants.Platform, platform),
+                        new(TelemetryConstants.ApiId, apiEvent.ApiId),
+                        new(TelemetryConstants.TokenSource, apiEvent.TokenSource),
+                        new(TelemetryConstants.CacheLevel, string.Empty),


+                CollectionAssert.Contains(
+                    _exportedMetrics.Select(m => m.Name).ToList(),
+                    "MsalDurationInHttp.1A",
+                    "Background refresh HTTP duration must be recorded in MsalDurationInHttp.1A");


+                        expectedTags.Add(TelemetryConstants.ApiId);
+                        expectedTags.Add(TelemetryConstants.CacheRefreshReason);
+                        expectedTags.Add(TelemetryConstants.TokenType);
+                        expectedTags.Add(TelemetryConstants.Succeeded);


+                }
+
+                bool isTimeout = string.Equals(errorCode, MsalError.RequestTimeout, StringComparison.OrdinalIgnoreCase);
+                if ((httpStatusCode > 0 || (isTimeout && apiEvent.DurationInHttpInMs > 0)) && s_durationInHttpV2.Value.Enabled)


+                // 2. Acquire token — failure
+                _harness.HttpManager.AddTokenResponse(TokenResponseType.InvalidClient);
+                await AssertException.TaskThrowsAsync<MsalServiceException>(
+                    () => cca.AcquireTokenForClient(TestConstants.s_scopeForAnotherResource)
+                        .WithExtraQueryParameters(extraQueryParams)
+                        .WithTenantId(TestConstants.Utid)
+                        .ExecuteAsync()).ConfigureAwait(false);


Add token acquisition metrics that cover both successful and failed t…

e6073dc

…oken acquisition outcomes

Copilot AI review requested due to automatic review settings April 15, 2026 15:18

Copilot AI reviewed Apr 15, 2026

View reviewed changes

Copilot started reviewing on behalf of ssmelov April 15, 2026 16:54 View session

ssmelov added 2 commits April 15, 2026 22:02

Add tests

43a844c

Update

e85108e

Copilot AI review requested due to automatic review settings April 16, 2026 18:10

Copilot started reviewing on behalf of ssmelov April 16, 2026 18:11 View session

Copilot AI reviewed Apr 16, 2026

View reviewed changes

gladjohn mentioned this pull request May 1, 2026

[Engineering task] Copilot PR Review: excessive noise from review_on_push and false positives #5967

Closed

neha-bhargava reviewed May 6, 2026

View reviewed changes

Update

f7251e0

Copilot AI review requested due to automatic review settings May 13, 2026 14:02

Copilot started reviewing on behalf of ssmelov May 13, 2026 14:03 View session

Copilot AI reviewed May 13, 2026

View reviewed changes

		int httpStatusCode = ex is MsalServiceException serviceEx ? serviceEx.StatusCode : 0;
		LogFailureTelemetryToOtel(apiEvent, apiEvent.ApiErrorCode, httpStatusCode, requestStopwatch.ElapsedMilliseconds + measureTelemetryDurationResult.Milliseconds);

	new(TelemetryConstants.CallerSdkId, apiEvent.CallerSdkApiId ?? string.Empty + "," + apiEvent.CallerSdkVersion ?? string.Empty),
	new(TelemetryConstants.CallerSdkId, $"{apiEvent.CallerSdkApiId ?? string.Empty},{apiEvent.CallerSdkVersion ?? string.Empty}"),

	new(TelemetryConstants.CacheLevel, string.Empty),
	new(TelemetryConstants.CacheLevel, CacheLevel.None),

Conversation

ssmelov commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

neha-bhargava May 6, 2026 • 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!

ssmelov May 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ssmelov commented Apr 15, 2026 •

edited

Loading

neha-bhargava May 6, 2026 •

edited

Loading