[core-rest-pipeline] Move logging policy in default pipeline

[xirzec]

Currently, logging occurs after the retryPhase (and nominally after the tracing and redirect policies), but authentication (added by core-client) comes earlier.

However, now that we've added the sign phase after retry, an auth policy that uses this new phase in order to re-sign retried requests won't get logged. I feel like we should move the loggingPolicy into afterPhase: "Sign" to account for this.

I'm less sure about moving the default auth policy into the sign phase, given that most normal bearer tokens don't need to be re-stamped each time we retry, especially since the auth policy handles token expiration errors.

@jeremymeng @deyaaeldeen @joheredi thoughts?

[azure-sdk]

Label prediction was below confidence level 0.6 for Model:ServiceLabels: 'Azure.Core:0.25389853,azure-spring:0.20232049,Storage:0.11080181'

[deyaaeldeen]

I feel like we should move the loggingPolicy into afterPhase: "Sign" to account for this.

That makes sense to me!

I'm less sure about moving the default auth policy into the sign phase, given that most normal bearer tokens don't need to be re-stamped each time we retry, especially since the auth policy handles token expiration errors.

I think it makes sense not to move it because otherwise we will be wasting time asking for new tokens after each retry unless the previous token expired for some reason.

[joheredi]

+1 on both points. We can move the logging policy and keep the default auth where it is now, shouldn't affect the auth path since 4xx errors are not retried anyway.

[jeremymeng]

Makes sense to me! Just to help my understanding, logging policy will not be applied to our default auth policy?

[xirzec]

Makes sense to me! Just to help my understanding, logging policy will not be applied to our default auth policy?

Sort of, the way this works is we aren't going to list any auth headers/parameters as loggable:

https://github.com/Azure/azure-sdk-for-js/blob/dccf667a3a39f35a40c041e3c657ec079e09e466/sdk/core/core-rest-pipeline/src/policies/logPolicy.ts#L25

The current list is here: https://github.com/Azure/azure-sdk-for-js/blob/dccf667a3a39f35a40c041e3c657ec079e09e466/sdk/core/core-rest-pipeline/src/util/sanitizer.ts#L28