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] Add more information to error logs #3278

Closed
pmaytak opened this issue Apr 14, 2022 · 5 comments
Closed

[Feature Request] Add more information to error logs #3278

pmaytak opened this issue Apr 14, 2022 · 5 comments

Comments

@pmaytak
Copy link
Contributor

pmaytak commented Apr 14, 2022

Problem statement
Currently users use either Error or Warning log level. When exception happens and is logged, the information can be a bit generic and not enough to debug (ex. some HTTP timeout exception happened). Sometimes the error is difficult to repro or the users can't change log level to Verbose, for some reason.

Possible solution
Make sure when we log errors (and perhaps warnings), we don't just log the exception but also any related request information that can help in debugging. Look at what we log in Info logs and maybe move that information to Error logs. Also verify all the important places in MSAL where we log errors (ex. where we send token requests) and see if more information is needed to be added there.

@pmaytak pmaytak added this to the 4.44.0 milestone Apr 14, 2022
@bgavrilMS bgavrilMS changed the title [Bug] Add more information to error logs [Feature Request] Add more information to error logs Apr 15, 2022
@bgavrilMS
Copy link
Member

I think we should do this for any HTTP errors, AAD errors and unexpected errors from MSAL. log.Error and log.Warning are used to also give user information about suboptimal config (e.g. using "https://lmo/common" is S2S flow will trigger a log error).

@bgavrilMS
Copy link
Member

@neha-bhargava - you can also pick this one.

@neha-bhargava neha-bhargava self-assigned this May 9, 2022
@neha-bhargava
Copy link
Contributor

neha-bhargava commented May 9, 2022

I think we should do this for any HTTP errors, AAD errors and unexpected errors from MSAL. log.Error and log.Warning are used to also give user information about suboptimal config (e.g. using "https://lmo/common" is S2S flow will trigger a log error).

@bgavrilMS Do you mean to log information related to which endpoint is used in case of HTTP errors? Or the flow used for AAD errors?

@bgavrilMS
Copy link
Member

@neha-bhargava - please reach to @pmaytak who can explain the context of this request in more detail. The point is that if some error occurs, we should dump at least the full details of the request to the Error log. We now only log at INFO level the details about each request (see the logic in RequestBase).

@pmaytak
Copy link
Contributor Author

pmaytak commented May 10, 2022

Updated work item description.

@pmaytak pmaytak closed this as completed Jun 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants