Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 52d6bcb8 authored by Arpan's avatar Arpan
Browse files

Fix Order of Logging 

Our metrics record duplicate and default values. This can be solved by
moving the order of the emits further down. The method also needs to be
locked so that it is not called multiple times - or if so, is not facing
any race conditions that may continue to cause unexpected outcomes.

Bug: 362995084
Bug: 362994633
Test: North stars in metrics counts - repro for this locally has been
attempted, though only collected metrics have this issue
Flag: android.credentials.flags.fix_metric_duplication_emits

Change-Id: Iae0d31d3c5b48897fe72b69bb26b09c3d35a756b
parent 29706569

services/credentials/java/com/android/server/credentials/RequestSession.java

+61 −11
Original line number Diff line number Diff line
@@ -258,12 +258,33 @@ abstract class RequestSession<T, U, V> implements CredentialManagerUi.Credential 
        if (propagateCancellation) {

            mProviders.values().forEach(ProviderSession::cancelProviderRemoteSession);

        }

        mRequestSessionMetric.logApiCalledAtFinish(apiStatus);

        mRequestSessionStatus = RequestSessionStatus.COMPLETE;

        if (Flags.fixMetricDuplicationEmits()) {

            logTrackOneCandidatesAndPrepareFinalPhaseLogs(apiStatus);

        }

        mRequestSessionMetric.logApiCalledAtFinish(apiStatus);

        mProviders.clear();

        clearRequestSessionLocked();

    }



    /**

     * Ensures all logging done in final phase methods only occur within the 'finishSession'.

     */

    private void logTrackOneCandidatesAndPrepareFinalPhaseLogs(int apiStatus) {

        mRequestSessionMetric.logCandidateAggregateMetrics(mProviders);

        if (isRespondingWithError(apiStatus)) {

            mRequestSessionMetric.collectFinalPhaseProviderMetricStatus(

                    /*hasException=*/ true, ProviderStatusForMetrics.FINAL_FAILURE);

        } else if (isRespondingWithUserCanceledError(apiStatus)) {

            mRequestSessionMetric.collectFinalPhaseProviderMetricStatus(

                    /*hasException=*/false, ProviderStatusForMetrics.FINAL_FAILURE

            );

        } else if (isRespondingWithSuccess(apiStatus)) {

            mRequestSessionMetric.collectFinalPhaseProviderMetricStatus(/*hasException=*/ false,

                    ProviderStatusForMetrics.FINAL_SUCCESS);

        }

    }



    void cancelExistingPendingIntent() {

        if (mPendingIntent != null) {

            try {
@@ -343,9 +364,11 @@ abstract class RequestSession<T, U, V> implements CredentialManagerUi.Credential 
     * @param response the response associated with the API call that just completed

     */

    protected void respondToClientWithResponseAndFinish(V response) {

        if (!Flags.fixMetricDuplicationEmits()) {

            mRequestSessionMetric.logCandidateAggregateMetrics(mProviders);

        mRequestSessionMetric.collectFinalPhaseProviderMetricStatus(/*has_exception=*/ false,

            mRequestSessionMetric.collectFinalPhaseProviderMetricStatus(/*hasException=*/ false,

                    ProviderStatusForMetrics.FINAL_SUCCESS);

        }

        if (mRequestSessionStatus == RequestSessionStatus.COMPLETE) {

            Slog.w(TAG, "Request has already been completed. This is strange.");

            return;
@@ -360,8 +383,10 @@ abstract class RequestSession<T, U, V> implements CredentialManagerUi.Credential 
            finishSession(/*propagateCancellation=*/false,

                    ApiStatus.SUCCESS.getMetricCode());

        } catch (RemoteException e) {

            if (!Flags.fixMetricDuplicationEmits()) {

                mRequestSessionMetric.collectFinalPhaseProviderMetricStatus(

                    /*has_exception=*/ true, ProviderStatusForMetrics.FINAL_FAILURE);

                        /*hasException=*/ true, ProviderStatusForMetrics.FINAL_FAILURE);

            }

            Slog.e(TAG, "Issue while responding to client with a response : " + e);

            finishSession(/*propagateCancellation=*/false, ApiStatus.FAILURE.getMetricCode());

        }
@@ -374,9 +399,11 @@ abstract class RequestSession<T, U, V> implements CredentialManagerUi.Credential 
     * @param errorMsg  the error message given back in the flow

     */

    protected void respondToClientWithErrorAndFinish(String errorType, String errorMsg) {

        if (!Flags.fixMetricDuplicationEmits()) {

            mRequestSessionMetric.logCandidateAggregateMetrics(mProviders);

            mRequestSessionMetric.collectFinalPhaseProviderMetricStatus(

                /*has_exception=*/ true, ProviderStatusForMetrics.FINAL_FAILURE);

                    /*hasException=*/ true, ProviderStatusForMetrics.FINAL_FAILURE);

        }

        if (mRequestSessionStatus == RequestSessionStatus.COMPLETE) {

            Slog.w(TAG, "Request has already been completed. This is strange.");

            return;
@@ -385,7 +412,6 @@ abstract class RequestSession<T, U, V> implements CredentialManagerUi.Credential 
            finishSession(/*propagateCancellation=*/true, ApiStatus.CLIENT_CANCELED.getMetricCode());

            return;

        }



        try {

            invokeClientCallbackError(errorType, errorMsg);

        } catch (RemoteException e) {
@@ -393,7 +419,9 @@ abstract class RequestSession<T, U, V> implements CredentialManagerUi.Credential 
        }

        boolean isUserCanceled = errorType.contains(MetricUtilities.USER_CANCELED_SUBSTRING);

        if (isUserCanceled) {

            mRequestSessionMetric.setHasExceptionFinalPhase(/* has_exception */ false);

            if (!Flags.fixMetricDuplicationEmits()) {

                mRequestSessionMetric.setHasExceptionFinalPhase(/* hasException */ false);

            }

            finishSession(/*propagateCancellation=*/false,

                    ApiStatus.USER_CANCELED.getMetricCode());

        } else {
@@ -421,4 +449,26 @@ abstract class RequestSession<T, U, V> implements CredentialManagerUi.Credential 
            finishSession(isUiWaitingForData(), ApiStatus.CLIENT_CANCELED.getMetricCode());

        }

    }



    /**

     * This captures the final state of the apiStatus as presented in 'finishSession'.

     */

    private boolean isRespondingWithError(int apiStatus) {

        return apiStatus == ApiStatus.FAILURE.getMetricCode()

                || apiStatus == ApiStatus.CLIENT_CANCELED.getMetricCode();

    }



    /**

     * A unique failure case, where we do not set the exception bit to be true.

     */

    private boolean isRespondingWithUserCanceledError(int apiStatus) {

        return apiStatus == ApiStatus.USER_CANCELED.getMetricCode();

    }



    /**

     * This captures the final state of the apiStatus as presented in 'finishSession'.

     */

    private boolean isRespondingWithSuccess(int apiStatus) {

        return apiStatus == ApiStatus.SUCCESS.getMetricCode();

    }

}