Add correlationId to log messages #3601
Conversation
github-actions
bot
added
msal-browser
|
Having the correlation id provided on the individual log messages (instead of being set on the logger itself) may be a better approach, to better support multiple concurrent requests. |
I was discussing this with @jo-arroyo! Since every request is a separate instance, will there be a case where parallel requests point to the same logger instance and mess up the correlation-id referenced? If so, we need to track correlation-id separately per request and add it to the logs as an extra parameter. Better yet, have a request logger instance derived from the main class. My guess is this depends on the design of parallel requests. We can discuss this @jo-arroyo @jasonnutter |
Not every request is guaranteed to have its own logger instance. |
Codecov Report
*This pull request uses carry forward flags. Click here to find out more.
|
…nfiguration, remove excess logger passing in browser, remove passing request to common
…to specify package on logMessage
…ationId from request where no client yet
…nt logger instance
…microsoft-authentication-library-for-js into add-correlation-id-logs
jo-arroyo
requested review from
pkanher617,
sameerag,
samuelkubai and
tnorling
as code owners
|
Did you consider generating a new logger instance per request (instead of using a single class property) and passing that down? It sounds like we can't avoid passing something down to functions if we want logging, I wonder if passing the logger instance would be better than passing the correlationId and then also needing to provide name and version on each message. Thoughts? |
In the future (i.e. v3), I would like to have a model where when a request is made, the first thing that happens is the library creates a client instance (e.g. SilentFlowClient, AuthCodeClient), and then everything downstream happens in that client, including log messages (as today each flow client includes its own logger). For example, today for a silent request, some things happen in the |
I'm actually working on a prototype for something very similar to this as we speak...FWIW I don't think it necessarily needs to wait until v3, but we'll see
Agreed, however, everything does happen downstream from the API that was called. We could instantiate a new logger instance each time an API is called and pass this down to whatever functions need to use the logger (which we're already doing in many cases today). I'm not personally a fan of passing the correlationId down everywhere because once you're outside PCA you'll need to provide this in addition to the logger. Is there something I'm overlooking that would not make this feasible today? |
| const authorizationCodeClient = new AuthorizationCodeClient( | ||
| authClientConfig | ||
| ); | ||
| this.logger.verbose("Auth code client created", validRequest.correlationId); |
There was a problem hiding this comment.
As there are not many logs in msal-node that use the flow client logger, I've decided to use the global logger for now with the updated correlation ID instead of instantiating a new nodeRequestLogger instance for each request. This can, of course, be changed in future PRs as the Logger changes.
…gger to use logger.clone
Co-authored-by: Jason Nutter <janutter@microsoft.com>
|
🎉 We recommend upgrading to the latest version of Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
This PR adds the correlation ID to log messages to make it easier to track log messages associated with requests. The correlation ID is either passed in or created per request, and will be added to log messages related to the request and passed down to flow client to use on the client's logger instance. Addresses #3022.
Msal-common
Loggerto allow setting correlation ID on the logger instance from theloggerOptionsLogger.logMessage()to set correlation ID if present. Precedence goes to the correlation ID passed in the log message, then to the correlation ID set on the Logger instance.Loggercorrelation ID changesMsal-browser
browserRequestLogger, with the correlation ID set when instantiated. This logger instance is used for all subsequent logs related to the request, and is passed into called functions and interaction handlers where applicable.browserRequestLoggeris not available, the correlation ID, if available, may still be passed for log messages.Loggeris redesigned.getClientConfiguration, so correlation ID can be set on flow client logger instancebrowserRequestLoggerMsal-node
buildOauthClientConfiguration, so correlation ID can be set on flow client logger instancetraceto reduce noisiness ofverboselevel log messagesScreenshot examples of new logs:
Note: We realize that we are knowingly taking on tech debt by passing in the correlation ID manually into log messages. In the future, we would like to refactor usage of the Logger so this will no longer be necessary.