Retry for invalid_grant errors - Silent Iframe

[jo-arroyo]

First of 3 PRs addressing need for retry for backup auth service.

[tnorling]

I wonder if we should log a flag to telemetry to explicitly indicate a retry was performed? Or is it preferable to determine this by building queries with the existing data? I expect this will have big perf implications if this is done at scale and it might be nice to have an easy way to track this. Just a suggestion but I'll leave that up to you and @konstantin-msft

[peterzenz]

I wonder if we should log a flag to telemetry to explicitly indicate a retry was performed? Or is it preferable to determine this by building queries with the existing data? I expect this will have big perf implications if this is done at scale and it might be nice to have an easy way to track this. Just a suggestion but I'll leave that up to you and @konstantin-msft

All of these retries need an explicit indicator in telemetry that they are happening. It would likely be helpful to log the error returned for both the original call (invalid grant today, possibly others in the future?) as well as the retry result.

[konstantin-msft]

I wonder if we should log a flag to telemetry to explicitly indicate a retry was performed? Or is it preferable to determine this by building queries with the existing data? I expect this will have big perf implications if this is done at scale and it might be nice to have an easy way to track this. Just a suggestion but I'll leave that up to you and @konstantin-msft

All of these retries need an explicit indicator in telemetry that they are happening. It would likely be helpful to log the error returned for both the original call (invalid grant today, possibly others in the future?) as well as the retry result.

It makes sense to add a new telemetry data point that indicates a retry was performed. Error codes, in turn, are captured in the "context" field for each instrumented function. However, there are two scenarios here:

• Both the original call and retry fail -> the context is always set. The retry error is placed in the errorCode field, while the first error can be found in the "context" field only.
• Original call fails and retry succeeds -> the context is redacted for 90% of the calls. Hence, we won't be able to capture most of the original errors so counting their numbers by type won't be possible. So, it makes sense to add yet another telemetry data point that captures the original error code.

[jo-arroyo]

I wonder if we should log a flag to telemetry to explicitly indicate a retry was performed? Or is it preferable to determine this by building queries with the existing data? I expect this will have big perf implications if this is done at scale and it might be nice to have an easy way to track this. Just a suggestion but I'll leave that up to you and @konstantin-msft

All of these retries need an explicit indicator in telemetry that they are happening. It would likely be helpful to log the error returned for both the original call (invalid grant today, possibly others in the future?) as well as the retry result.

It makes sense to add a new telemetry data point that indicates a retry was performed. Error codes, in turn, are captured in the "context" field for each instrumented function. However, there are two scenarios here:

• Both the original call and retry fail -> the context is always set. The retry error is placed in the errorCode field, while the first error can be found in the "context" field only.
• Original call fails and retry succeeds -> the context is redacted for 90% of the calls. Hence, we won't be able to capture most of the original errors so counting their numbers by type won't be possible. So, it makes sense to add yet another telemetry data point that captures the original error code.

Thanks for the input, all! I added a new data point. Let me know if you prefer a different name.

[konstantin-msft]

Optional. You can simplify nested if conditions by joining them together into if (!authClient || !(e instanceof ServerError) || e.errorCode !== BrowserConstants.INVALID_GRANT_ERROR) { then throw e } so you won't need else.