Populate telemetry metadata on proactive-refresh results (3707191) - #6157
Merged
Neha Bhargava (neha-bhargava) merged 3 commits intoAug 6, 2026
Merged
Neha Bhargava (neha-bhargava) merged 3 commits into
Neha Bhargava (neha-bhargava) merged 3 commits into
Conversation
Proactive (background) refresh runs the fetch outside RequestBase.RunAsync, so the AuthenticationResultMetadata that RunAsync normally fills in (TokenEndpoint, DurationTotalInMs, DurationInHttpInMs, CacheRefreshReason, RegionDetails, etc.) was left at its constructor defaults. Consumers of the OnBackgroundTokenRefreshCompleted callback, and the LogSuccessHttpDuration OTel emission, therefore saw stripped telemetry (null TokenEndpoint, 0 duration). Extract the metadata-population core of UpdateTelemetry into a shared static RequestBase.PopulateSuccessMetadata and call it from the background success path in SilentRequestHelper.ProcessFetchInBackground, backfilling from the shared apiEvent and using the background operation's own elapsed time as the total duration. Mirrors the existing failure-path treatment via CreateFailureMetadata. The global Metrics total-duration counter stays foreground-only so background latency does not inflate the user-facing aggregate. Strengthen the background-refresh success test to assert the metadata is populated (fails against the pre-fix implementation). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 766dbf9f-f979-44b5-9362-a1a1efae90ea
Copilot started reviewing on behalf of
Neha Bhargava (neha-bhargava)
August 3, 2026 23:46
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes missing telemetry metadata on proactive (background) token refresh AuthenticationResults by sharing the same metadata-population logic used by the foreground (RequestBase.RunAsync) path.
Changes:
- Refactors the “success metadata” portion of
RequestBase.UpdateTelemetryinto a sharedinternal static RequestBase.PopulateSuccessMetadata. - Populates proactive-refresh success results’ metadata in
SilentRequestHelper.ProcessFetchInBackground, using the background operation’s own elapsed time forDurationTotalInMs. - Strengthens the unit test to assert deterministic metadata fields are populated on proactive-refresh results (regression coverage for AB#3707191).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/Microsoft.Identity.Test.Unit/PublicApiTests/RefreshInTests.cs | Adds regression assertions ensuring proactive-refresh results contain populated telemetry metadata. |
| src/client/Microsoft.Identity.Client/Internal/Requests/SilentRequestHelper.cs | Measures background refresh duration and backfills success metadata onto the background AuthenticationResult. |
| src/client/Microsoft.Identity.Client/Internal/Requests/RequestBase.cs | Extracts shared success-metadata copy logic into PopulateSuccessMetadata and reuses it from UpdateTelemetry. |
Make the added comments concise. Also strengthen the managed identity background-refresh test to assert the proactive-refresh result carries backfilled telemetry (CacheRefreshReason, TokenSource) - MI is the other primary OnBackgroundTokenRefreshCompleted consumer and flows through the same ProcessFetchInBackground path. TokenEndpoint stays null for MI (no TokenClient), so only the applicable fields are asserted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 766dbf9f-f979-44b5-9362-a1a1efae90ea
Copilot started reviewing on behalf of
Neha Bhargava (neha-bhargava)
August 4, 2026 06:33
View session
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 766dbf9f-f979-44b5-9362-a1a1efae90ea
Copilot started reviewing on behalf of
Neha Bhargava (neha-bhargava)
August 4, 2026 06:36
View session
Bogdan Gavril (bgavrilMS)
approved these changes
Aug 4, 2026
Gladwin Johnson VR (gladjohn)
approved these changes
Aug 4, 2026
Travis Walker (trwalke)
approved these changes
Aug 4, 2026
Neha Bhargava (neha-bhargava)
deleted the
nebharg/fix-proactive-refresh-telemetry-3707191
branch
August 6, 2026 00:41
This was referenced Aug 20, 2026
fix: Bump Microsoft.Identity.Client from 4.87.0 to 4.88.0
Halceyon/open-telemetry-trace-listener#289
Closed
Merged
This was referenced Sep 13, 2026
Closed
Bump Microsoft.Identity.Client and Microsoft.Identity.Client.Extensions.Msal
teglsbo/DataverseDuck#5
Merged
This was referenced Sep 20, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Proactive (background) token refresh produced
AuthenticationResults with stripped telemetry metadata. Fixes AB#3707191.The proactive-refresh path (
SilentRequestHelper.ProcessFetchInBackground) runs the fetch outsideRequestBase.RunAsync, so theAuthenticationResultMetadatathatRunAsync->UpdateTelemetrynormally fills in was left at its constructor defaults:TokenEndpointwasnullDurationTotalInMswas always0DurationInHttpInMs,CacheRefreshReason,RegionDetails,CacheLevel,CachedAccessTokenCountwere also unsetAffected consumers:
OnBackgroundTokenRefreshCompletedcallback (ExecutionResult.Result).LogSuccessHttpDurationOTel emission insideProcessFetchInBackground, which recordedDurationInHttpInMs == 0for every proactive refresh.Note: the token is cached, not the
AuthenticationResult, so a later foreground read still rebuilds correct metadata viaRunAsync. The bug affects the background result object specifically.Fix
UpdateTelemetryinto a sharedinternal static RequestBase.PopulateSuccessMetadata.apiEvent(whose HTTP fields the background call populates) and using the background operation's own elapsed time asDurationTotalInMs.CreateFailureMetadata) and avoids a third divergent field-copy.GetCacheLeveland the globalMetrics.IncrementTotalDurationInMsincrement stay on the foreground path only, so background latency does not inflate the user-facing aggregate.Why background duration (not the parent request's)?
DurationTotalInMsmeans "time to produce this result." The parent request returned a cached token in ~0ms; stamping that onto the background result would re-introduce the ~0 symptom and makeTotal < Http(sinceapiEvent.DurationInHttpInMsis overwritten by the background call) - internally inconsistent.Tests
ClientCredentials_BackgroundRefresh_Success_InvokesCallback_Asyncto assert the result carries populated metadata (CacheRefreshReason == ProactivelyRefreshed, non-emptyTokenEndpoint,TokenSource == IdentityProvider).RefreshInTests(13 passed) andTelemetryTests(28 passed) green on net8.0.DurationTotalInMs > 0is intentionally not asserted - a sub-millisecond in-memory mock round-trip makes it flaky. The deterministic backfilled fields prove the fix.