Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Request] Expose CorrelationId from MsalException #4187

Closed
KurzyukovAndrey opened this issue Jun 26, 2023 · 4 comments · Fixed by #4649
Closed

[Feature Request] Expose CorrelationId from MsalException #4187

KurzyukovAndrey opened this issue Jun 26, 2023 · 4 comments · Fixed by #4649

Comments

@KurzyukovAndrey
Copy link

Problem
We need to meter CorrelationId in case of acquiring a token. However, we cannot retrieve CorrelationId from MsalException.

Use case
The simplified code of usage is shown below:

public async Task<AuthenticationResult> AcquireTokenForClientAsync(
	IEnumerable<string> scopes, 
	Action<AcquireTokenForClientParameterBuilder>? configuration, 
	CancellationToken cancellationToken)
{
	var acquireTokenBuilder = _confidentialClientApplication.AcquireTokenForClient(scopes);

	// WithCorrelationId might be called here.
	configuration?.Invoke(acquireTokenBuilder);

	try
	{
		var authenticationResult = await acquireTokenBuilder
			.ExecuteAsync(cancellationToken)
			.ConfigureAwait(false);

		// authenticationResult exposes correlationId.
	}
	catch (MsalException ex)
	{
		// MsalException does not expose correlationId. 
		// Cannot use correlationId for metering.
	}
}

Our consumers call WithCorrelationId using configuration parameter. Then, we acquire a token. In case of success, we can retrieve CorrelationId from AuthenticationResult.CorrelationId. But we cannot retrieve it from MsalException.

Proposal
Expose CorrelationId for all exceptions.

Additional context
Based on Exceptions in MSAL.NET, there are 4 exceptions. MsalServiceException and MsalUiRequiredException expose CorrelationId, but the rest do not.

@KurzyukovAndrey KurzyukovAndrey changed the title Not exposed CorrelationId from MsalException [Feature Request] Expose CorrelationId from MsalException Jun 26, 2023
@trwalke
Copy link
Member

trwalke commented Jun 28, 2023

Hi @KurzyukovAndrey, there shouldnt be any scenarios where we throw a MsalException it is a base class that the other exceptions derive from. You should rely on the correlation id in the MsalServiceException and MsalUiRequiredException since they can occur after a request is sent to the auth endpoint.
Also, typically the MsalClientException occurs in the client before a correlationd Id is sent to the endpoint so there would be no correlation id to look up in the telemetry anyway.

@KurzyukovAndrey
Copy link
Author

Hi @trwalke, thank you for your response!

@bgavrilMS bgavrilMS closed this as not planned Won't fix, can't repro, duplicate, stale Jul 5, 2023
@bgavrilMS bgavrilMS reopened this Jul 12, 2023
@bgavrilMS bgavrilMS added epic and removed epic labels Jul 12, 2023
@KurzyukovAndrey
Copy link
Author

Any updates about the progress/state?

@trwalke trwalke self-assigned this Feb 22, 2024
@trwalke trwalke linked a pull request Mar 6, 2024 that will close this issue
1 task
@trwalke trwalke closed this as completed Mar 6, 2024
@trwalke trwalke added this to the 4.60.0 milestone Mar 7, 2024
@neha-bhargava
Copy link
Contributor

Fixed in MSAL.Net 4.60.0 release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants