Wire onboarding telemetry hooks into AzureActiveDirectoryWebViewClient, Fixes AB#3462876#3121
Wire onboarding telemetry hooks into AzureActiveDirectoryWebViewClient, Fixes AB#3462876#3121wzhipan wants to merge 6 commits into
Conversation
Adds setOnboardingTelemetryRecorder() setter and emission calls at the WebView URL-handling sites identified in the design spec (Mobile Onboarding Telemetry section 7.5.2). Both the OneAuth fragment (non-brokered flow) and the broker AuthorizationActivity (brokered flow) can now attach a recorder so WebView page transitions are captured into the onboarding blob. Hook points (all best-effort, no-op if no recorder attached): - onPageFinished -> setLastLoadedDomain(host) - processInstallRequest / processIntentToInstallBrokerApp -> STEP_BROKER_INSTALL_PROMPTED - processDeviceCaRequest -> STEP_MDM_ENROLLMENT_STARTED - launchCompanyPortal -> STEP_COMPANY_PORTAL_LAUNCHED - processWebCpEnrollmentUrl -> STEP_WEB_CP_ENROLLMENT_STARTED - openGoogleEnrollmentUrl -> STEP_GOOGLE_ENROLLMENT_STARTED - processAuthAppMFAUrl -> STEP_AUTHENTICATOR_MFA_LINKING_STARTED Includes 4 new unit tests in AzureActiveDirectoryWebViewClientTest covering the broker install hook (with and without recorder) and onPageFinished domain recording (real URL and blank URL). All 59 WebView tests pass. Note: This branch also includes the constants commit (C4) so this branch compiles standalone. When both PRs land, the duplicate commit will resolve naturally on rebase.
|
✅ Work item link check complete. Description contains link AB#3462876 to an Azure Boards work item. |
|
❌ Work item link check failed. Description contains AB#3462876 but the Bot could not link it to an Azure Boards work item. Click here to learn more. |
There was a problem hiding this comment.
Pull request overview
Adds optional onboarding-telemetry instrumentation to AzureActiveDirectoryWebViewClient so WebView navigation events can populate the mobile onboarding telemetry blob used downstream (OneAuth/broker) for CA-blocked remediation funnels.
Changes:
- Adds an optional
OnboardingTelemetryRecordersetter toAzureActiveDirectoryWebViewClientand emits onboarding step IDs at key URL-dispatch points. - Records the host of loaded pages into the onboarding blob as
last_loaded_domainfromonPageFinished. - Adds/updates unit tests and a changelog entry for the new onboarding telemetry hooks.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java | Adds nullable recorder attachment + best-effort step/domain recording hooks |
| common/src/test/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClientTest.java | Adds tests validating step emission and last-loaded-domain recording |
| changelog.txt | Documents the new onboarding telemetry hooks as a MINOR change |
- Use {@code lastLoadedDomain} in javadoc instead of markdown backticks
- Emit STEP_BROKER_INSTALL_PROMPTED from processPlayStoreURL and
processPlayStoreURLForBrokerApps so Play Store dispatch paths populate
the onboarding step timeline (previously only processInstallRequest and
processIntentToInstallBrokerApp did)
- Test: use OnboardingTelemetryConstants.STEP_BROKER_INSTALL_PROMPTED and
LAST_LOADED_DOMAIN constants instead of hardcoded string literals
- Test: clear OnboardingSessionCorrelationStore in @after teardown to keep
tests isolated from other onboarding tests
Prvnkmr337
left a comment
There was a problem hiding this comment.
Low priority — Consider using imports instead of fully-qualified class names
Throughout the new code, fully-qualified class names are used inline (e.g., com.microsoft.identity.common.java.telemetry.OnboardingTelemetryConstants.STEP_BROKER_INSTALL_PROMPTED), which makes the lines quite long and harder to scan.
Consider adding imports at the top of the file:
import com.microsoft.identity.common.internal.telemetry.OnboardingTelemetryRecorder;
import static com.microsoft.identity.common.java.telemetry.OnboardingTelemetryConstants.*;This would shorten every call site from:
recordOnboardingStep(com.microsoft.identity.common.java.telemetry.OnboardingTelemetryConstants.STEP_BROKER_INSTALL_PROMPTED);to:
recordOnboardingStep(STEP_BROKER_INSTALL_PROMPTED);Non-blocking — purely a readability suggestion.
Prvnkmr337
left a comment
There was a problem hiding this comment.
Low priority — @Nullable on recordLastLoadedDomain parameter is misleading
The url parameter is annotated @Nullable, but the only caller is onPageFinished, where Android's WebViewClient.onPageFinished(WebView, String) declares url as @NonNull.
Since null is never an expected input, consider changing to @NonNull to better document the contract:
private void recordLastLoadedDomain(@NonNull final String url) {
final ... recorder = mOnboardingTelemetryRecorder;
if (recorder == null || url.isEmpty()) {
return;
}
// ...
}This avoids misleading readers into thinking null is a valid input and removes the unnecessary null check.
Non-blocking — purely a correctness-of-contract suggestion.
Prvnkmr337
left a comment
There was a problem hiding this comment.
Low priority — Add verbose log when host extraction yields null/empty
In recordLastLoadedDomain, when Uri.parse(url).getHost() returns null or empty for a non-empty URL, it silently falls through. This is an unexpected case that could indicate a malformed URL — a verbose log would help with diagnostics:
final String host = Uri.parse(url).getHost();
if (host != null && !host.isEmpty()) {
recorder.setLastLoadedDomain(host);
} else {
Logger.verbose(TAG, "Onboarding telemetry: no host extracted from URL");
}Using Logger.verbose to keep it low-noise in production while still being available for debugging.
Non-blocking.
Prvnkmr337
left a comment
There was a problem hiding this comment.
Nit — Changelog entry missing PR number
The new changelog entry is missing the (#3121) suffix that all other entries follow:
-- [MINOR] Wire onboarding telemetry hooks into AzureActiveDirectoryWebViewClient for page-transition step capture (broker install, MDM enrollment, Company Portal launch, MFA linking) and last-loaded-domain tracking
++ [MINOR] Wire onboarding telemetry hooks into AzureActiveDirectoryWebViewClient for page-transition step capture (broker install, MDM enrollment, Company Portal launch, MFA linking) and last-loaded-domain tracking (#3121)- Replace fully-qualified telemetry class names with imports (regular import for OnboardingTelemetryRecorder, static imports for the STEP_* constants) so call sites read cleanly - recordLastLoadedDomain: change url parameter from @nullable to @nonnull to match WebViewClient.onPageFinished contract; drop the null check - recordLastLoadedDomain: log Logger.verbose when Uri.parse(url).getHost() returns null/empty for a non-empty URL — aids diagnostics for malformed URLs without being noisy in production - Changelog: add (#3121) PR reference suffix
- Replace fully-qualified telemetry class names with imports (regular import for OnboardingTelemetryRecorder, static imports for the STEP_* constants) so call sites read cleanly - recordLastLoadedDomain: change url parameter from @nullable to @nonnull to match WebViewClient.onPageFinished contract; drop the null check - recordLastLoadedDomain: log Logger.verbose when Uri.parse(url).getHost() returns null/empty for a non-empty URL — aids diagnostics for malformed URLs without being noisy in production - Changelog: add (#3121) PR reference suffix
0cde31a to
2a29e0b
Compare
…metry-webview-v2 # Conflicts: # changelog.txt
|
Rebased onto latest dev and resolved changelog conflicts. Please re-review when you get a chance. |
C3 in the onboarding telemetry feature series (follows merged #3088 / #3111 / #3117).
Adds optional onboarding-telemetry instrumentation to
AzureActiveDirectoryWebViewClientso the WebView page-transition flow can populate the onboarding blob.What this adds:
setOnboardingTelemetryRecorder(recorder)setter onAzureActiveDirectoryWebViewClient— host fragment/activity attaches the recorder once a seed JSON arrives.processInstallRequest/processPlayStoreURL/processIntentToInstallBrokerApp→STEP_BROKER_INSTALL_PROMPTEDprocessDeviceCaRequest→STEP_MDM_ENROLLMENT_STARTEDlaunchCompanyPortal/processWebCpRequest→STEP_COMPANY_PORTAL_LAUNCHEDprocessWebCpEnrollmentUrl→STEP_WEB_CP_ENROLLMENT_STARTEDopenGoogleEnrollmentUrl→STEP_GOOGLE_ENROLLMENT_STARTEDprocessAuthAppMFAUrl→STEP_AUTHENTICATOR_MFA_LINKING_STARTEDonPageFinishedcaptures the host of the final loaded URL intolast_loaded_domain.Safety: Recorder is
@Nullable— when not attached (every existing caller, today), this is a no-op.Tests:
AzureActiveDirectoryWebViewClientTestcovers all setter + emission paths.Fixes AB#3462876