diff --git a/changelog.txt b/changelog.txt index 7904fbed39..dd8c9df5fa 100644 --- a/changelog.txt +++ b/changelog.txt @@ -1,5 +1,6 @@ vNext ---------- +- [MINOR] Add callback for OneAuth for measuring Broker Discovery Client Perf (#2796) - [MINOR] Add new span name for DELEGATION_CERT_INSTALL's telemetry (#2790) - [MINOR] Refactor getAccountByLocalAccountId (#2781) - [MINOR] Add OTel Benchmarker (#2786) diff --git a/common/src/main/java/com/microsoft/identity/common/internal/activebrokerdiscovery/BrokerDiscoveryClient.kt b/common/src/main/java/com/microsoft/identity/common/internal/activebrokerdiscovery/BrokerDiscoveryClient.kt index 728c30f4fd..a5f70cfdec 100644 --- a/common/src/main/java/com/microsoft/identity/common/internal/activebrokerdiscovery/BrokerDiscoveryClient.kt +++ b/common/src/main/java/com/microsoft/identity/common/internal/activebrokerdiscovery/BrokerDiscoveryClient.kt @@ -275,19 +275,39 @@ class BrokerDiscoveryClient(private val brokerCandidates: Set, override fun getActiveBroker(shouldSkipCache: Boolean): BrokerData? { return runBlocking { - return@runBlocking getActiveBrokerAsync(shouldSkipCache) + return@runBlocking getActiveBrokerAsync(shouldSkipCache, null) } } - private suspend fun getActiveBrokerAsync(shouldSkipCache:Boolean): BrokerData?{ + override fun getActiveBroker( + shouldSkipCache: Boolean, + telemetryCallback: IBrokerDiscoveryClientTelemetryCallback + ): BrokerData? { + return runBlocking { + return@runBlocking getActiveBrokerAsync(shouldSkipCache, telemetryCallback) + } + } + + private suspend fun getActiveBrokerAsync(shouldSkipCache:Boolean, + telemetryCallback: IBrokerDiscoveryClientTelemetryCallback?): BrokerData?{ val methodTag = "$TAG:getActiveBrokerAsync" + + val timeStartAcquiringLock = System.nanoTime() classLevelLock.withLock { + telemetryCallback?.onLockAcquired(System.nanoTime() - timeStartAcquiringLock) if (!shouldSkipCache) { if (cache.shouldUseAccountManager()) { + telemetryCallback?.onUseAccountManager() return getActiveBrokerFromAccountManager() } + val timeStartReadingFromCache = System.nanoTime() cache.getCachedActiveBroker()?.let { - if (!isPackageInstalled(it)) { + telemetryCallback?.onReadFromCache(System.nanoTime() - timeStartReadingFromCache) + + val timeStartIsPackageInstalled = System.nanoTime() + val isPackageInstalled = isPackageInstalled(it) + telemetryCallback?.onFinishCheckingIfPackageIsInstalled(System.nanoTime() - timeStartIsPackageInstalled) + if (!isPackageInstalled) { Logger.info( methodTag, "There is a cached broker: $it, but the app is no longer installed." @@ -296,7 +316,10 @@ class BrokerDiscoveryClient(private val brokerCandidates: Set, return@let } - if (!isValidBroker(it)) { + val timeStartIsValidBroker = System.nanoTime() + val isValidBroker = isValidBroker(it) + telemetryCallback?.onFinishCheckingIfValidBroker(System.nanoTime() - timeStartIsValidBroker) + if (!isValidBroker) { Logger.info( methodTag, "Clearing cache as the installed app does not have a matching signature hash." @@ -305,7 +328,11 @@ class BrokerDiscoveryClient(private val brokerCandidates: Set, return@let } - if(!ipcStrategy.isSupportedByTargetedBroker(it.packageName)){ + val timeStartIsSupportedByTargetedBroker = System.nanoTime() + val isSupportedByTargetedBroker = + ipcStrategy.isSupportedByTargetedBroker(it.packageName) + telemetryCallback?.onFinishCheckingIfSupportedByTargetedBroker(System.nanoTime() - timeStartIsSupportedByTargetedBroker) + if(!isSupportedByTargetedBroker){ Logger.info( methodTag, "Clearing cache as the installed app does not provide any IPC mechanism to communicate to. (e.g. the broker code isn't shipped with this apk)" @@ -319,12 +346,14 @@ class BrokerDiscoveryClient(private val brokerCandidates: Set, } } + val timeStartQueryFromBroker = System.nanoTime() val brokerData = queryFromBroker( brokerCandidates = brokerCandidates, ipcStrategy = ipcStrategy, isPackageInstalled = isPackageInstalled, isValidBroker = isValidBroker ) + telemetryCallback?.onFinishQueryingResultFromBroker(System.nanoTime() - timeStartQueryFromBroker) if (brokerData != null) { cache.setCachedActiveBroker(brokerData) @@ -343,6 +372,7 @@ class BrokerDiscoveryClient(private val brokerCandidates: Set, ) ) + telemetryCallback?.onUseAccountManager() val accountManagerResult = getActiveBrokerFromAccountManager() Logger.info( methodTag, "Tried getting active broker from account manager, " + diff --git a/common/src/main/java/com/microsoft/identity/common/internal/activebrokerdiscovery/IBrokerDiscoveryClient.kt b/common/src/main/java/com/microsoft/identity/common/internal/activebrokerdiscovery/IBrokerDiscoveryClient.kt index 8c8c5d4aff..f3ebb99494 100644 --- a/common/src/main/java/com/microsoft/identity/common/internal/activebrokerdiscovery/IBrokerDiscoveryClient.kt +++ b/common/src/main/java/com/microsoft/identity/common/internal/activebrokerdiscovery/IBrokerDiscoveryClient.kt @@ -37,6 +37,20 @@ interface IBrokerDiscoveryClient { * **/ fun getActiveBroker(shouldSkipCache: Boolean = false) : BrokerData? + + /** + * Performs a discovery to figure out which broker app the SDK (MSAL/OneAuth) + * has to send its request to. + * + * @param shouldSkipCache If true, this will skip cached value (if any) + * and always query the broker for the result. + * @param telemetryCallback callback with telemetry data. + * @return BrokerData package name and signature hash of the targeted app. + * **/ + fun getActiveBroker(shouldSkipCache: Boolean = false, + telemetryCallback: IBrokerDiscoveryClientTelemetryCallback) : BrokerData? + + /** * Force a broker app to figure out which broker app the SDK (MSAL/OneAuth) * has to send its request to. diff --git a/common/src/main/java/com/microsoft/identity/common/internal/activebrokerdiscovery/IBrokerDiscoveryClientTelemetryCallback.kt b/common/src/main/java/com/microsoft/identity/common/internal/activebrokerdiscovery/IBrokerDiscoveryClientTelemetryCallback.kt new file mode 100644 index 0000000000..389958b80e --- /dev/null +++ b/common/src/main/java/com/microsoft/identity/common/internal/activebrokerdiscovery/IBrokerDiscoveryClientTelemetryCallback.kt @@ -0,0 +1,80 @@ +// Copyright (c) Microsoft Corporation. +// All rights reserved. +// +// This code is licensed under the MIT License. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files(the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and / or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions : +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. +package com.microsoft.identity.common.internal.activebrokerdiscovery + +interface IBrokerDiscoveryClientTelemetryCallback { + /** + * Time spent acquiring the lock/mutex for broker discovery. + * + * @param timeSpentInNanoSeconds Time spent acquiring the lock/mutex (in nanoseconds). + **/ + fun onLockAcquired(timeSpentInNanoSeconds: Long) + + /** + * If triggered, the broker discovery client is using Account Manager + * (e.g. one of the installed broker is too old and doesn't support this new mechanism). + **/ + fun onUseAccountManager() + + /** + * Triggered when BrokerDiscoveryClient finishes reading from its cache. + * + * @param timeSpentInNanoSeconds Time spent reading the cache (in nanoseconds). + **/ + fun onReadFromCache(timeSpentInNanoSeconds: Long) + + /** + * Triggered when BrokerDiscoveryClient finishes validating if the cached broker package is installed. + * + * @param timeSpentInNanoSeconds Time spent to validate if the package is installed (in nanoseconds). + **/ + fun onFinishCheckingIfPackageIsInstalled(timeSpentInNanoSeconds: Long) + + /** + * Triggered when BrokerDiscoveryClient finishes validating if the cached broker package supports the given IPC strategy. + * + * @param timeSpentInNanoSeconds Time spent to validate if the package supports the IPC strategy (in nanoseconds). + **/ + fun onFinishCheckingIfSupportedByTargetedBroker(timeSpentInNanoSeconds: Long) + + /** + * Triggered when BrokerDiscoveryClient finishes validating if the cached broker package is a valid broker. + * + * @param timeSpentInNanoSeconds Time spent to validate if the package is a valid broker (in nanoseconds). + **/ + fun onFinishCheckingIfValidBroker(timeSpentInNanoSeconds: Long) + + /** + * Triggered when BrokerDiscoveryClient finishes querying active broker from one of the broker apps. + * + * There are 2 possible cases when this is triggered. + * 1. Both Broker and this MSAL/OneAuth app are freshly installed. + * This will trigger the broker discovery in broker, and will take longer time. + * + * 2. Broker has already done broker discovery before (e.g. by other MSAL/OneAuth app). Only this MSAL/OneAuth app is freshly installed. + * When the request reaches the broker, the broker would just return the cached value, which will be faster. + * + * @param timeSpentInNanoSeconds Time spent to query the active broker (in nanoseconds). + **/ + fun onFinishQueryingResultFromBroker(timeSpentInNanoSeconds: Long) +} diff --git a/common/src/main/java/com/microsoft/identity/common/internal/activebrokerdiscovery/LegacyBrokerDiscoveryClient.kt b/common/src/main/java/com/microsoft/identity/common/internal/activebrokerdiscovery/LegacyBrokerDiscoveryClient.kt index dfe8ef1e43..1f821f407d 100644 --- a/common/src/main/java/com/microsoft/identity/common/internal/activebrokerdiscovery/LegacyBrokerDiscoveryClient.kt +++ b/common/src/main/java/com/microsoft/identity/common/internal/activebrokerdiscovery/LegacyBrokerDiscoveryClient.kt @@ -35,6 +35,15 @@ class LegacyBrokerDiscoveryClient(val context: Context): IBrokerDiscoveryClient .getActiveBrokerFromAccountManager() } + override fun getActiveBroker( + shouldSkipCache: Boolean, + telemetryCallback: IBrokerDiscoveryClientTelemetryCallback + ): BrokerData? { + telemetryCallback.onUseAccountManager() + return AccountManagerBrokerDiscoveryUtil(context) + .getActiveBrokerFromAccountManager() + } + override fun forceBrokerRediscovery(brokerCandidate: BrokerData): BrokerData { throw UnsupportedOperationException("LegacyBrokerDiscoveryClient does not support forceBrokerRediscovery.") }