Praposal for logging support for msal-go #535

4gust · 2024-11-19T14:15:45Z

Praposal for logging support for msal-go

bgavrilMS

We discussed that we don't need the callback option for 1.18 for now.

bgavrilMS · 2024-12-09T11:28:45Z

apps/design/logPraposal.md

+
+// Logger struct for Go versions <= 1.20.
+type Logger struct {
+	LogCallback CallbackFunc


We discussed that for golang 1.18 it's ok to not have any logging.

….18, and just focusing on the logger working on 1.21 and above * adds tests * refactors logger documentation * adds sample logger app

bgavrilMS · 2024-12-19T14:16:44Z

apps/logger/logger.go

-	if logger, ok := input.(*slog.Logger); ok {
-		return &Logger{logging: logger}, nil
+// New creates a new logger instance
+func New(slogLogger *slog.Logger) (*Logger, error) {


I am not sure about this. The moment we put slog in the public API surface, Azure SDK will not be able to use MSAL SDK.

You need to hide this API in MSAL 1.18 and lower and only expose it in MSAL 1.2x

Internally, MSAL 1.18 should still continue to work and the internal logging statement will do nothing.

See with https://github.com/AzureAD/microsoft-authentication-library-for-go/blob/main/apps/design/release.md for testing with azure sdk

bgavrilMS · 2024-12-19T14:31:19Z

apps/logger/logger.go

+)
+
+// Logger struct for Go 1.21+ with full `slog` logging support.
+type Logger struct {


Do we need this object?

…ion for 1.21+ * Update tests

chlowell · 2025-01-07T01:00:06Z

apps/confidential/confidential_121_plus.go

+)
+
+//  WithLogger allows for a custom logger to be set.
+func WithLogger(l interface{}) Option {


interface{} effectively disables compiler type checking here, making this function difficult to use and turning a class of build errors into runtime errors--not great. This option only works if l is a *slog.Logger; should that be the parameter's type?

chlowell · 2025-01-07T01:02:44Z

apps/internal/logger/logger_121_plus.go

+
+	// Log the entry with the message and fields
+	a.logging.Log(
+		context.Background(),


this must be the Context of the encompassing operation i.e., the caller must provide it

chlowell · 2025-01-07T01:04:21Z

apps/tests/devapps/logger/logger_sample.go

Who's the audience for this?

Was made for testing purposes but can remove

chlowell · 2025-01-07T01:06:40Z

apps/internal/logger/logger_121_plus.go

+		return &logger{logging: loggerInterface}, nil
+	}
+
+	return nil, fmt.Errorf("invalid input for Go 1.21+; expected *slog.Logger")


then shouldn't the parameter be of type *slog.Logger?

chlowell · 2025-01-07T01:10:05Z

apps/design/logging.md

+## Key Pointers
+
+1. **For Go <= 1.20**:
+   - User should pass *nil* to `logger.New(nil)`


Users can't call logger.New because it's in an internal package. Is this doc or the implementation incorrect?

This doc is the old proposal that was updated for the newer implementation, but the new implementation has since changed so will have to update this document once we are happy with the implementation

chlowell · 2025-01-07T01:40:53Z

apps/confidential/confidential_121_plus.go

+		logInstance, err := logger.New(l)
+		if err != nil {
+			fmt.Println("Error creating logger with slog:", err)
+			return


This won't do. The caller has every reason to believe the call succeeded and won't find out something went wrong until they notice the lack of logging and/or this line in stdout. Returning the error is the right thing to do but impossible here. So, this needs a redesign. Off the top of my head, I suppose the alternatives are:

make it impossible for this function to receive an error i.e., make it impossible for logger.New to return an error

separate logger construction from this function i.e., have users call logger.New themselves, check its error, pass the logger to WithLogger

Option 2 is not great for developer experience though. The app developer would be exposed to an unnecessary abstraction.

Option 1 looks great. If not possible, would Option 3 be feasible - return the error in the ConfidentialClientApplication / ManagedIdentityApplication builder ?

I have updated the code for something like Option 1, so a default logger gets created in the case there is type assertion failures or nil provided

I don't see any updates in the PR

I just pushed now, was changing some test stuff

The question I'm really circling around here is, do you imagine ever supporting logging via something other than slog? Answering this points the way forward. If you want to support only slog, you can simply have WithLogger(logger slog.Logger), you don't need an abstraction layer like LoggerInterface, and you get my option 1 for free because this WithLogger can't produce an error. If you want to maintain the possibility of easy non-slog logging, then you need to expose LoggerInterface, have WithLogger take one of those, and decide what to do when someone calls WithLogger(nil)

I think the main issue or maybe just a misunderstanding on how best to handle it, is for go versions < 1.21
The LoggerInterface i think makes it simple to handle having something that supports 1.21 and also not causing an issue with < 1.21.
If we remove the loggerInterface would it not cause the code to become a bit more messy and having to use build tags directly inside the confidential.go class, and having clientOptions defined in both places?

…g of error * Updates tests

sonarqubecloud · 2025-01-08T16:24:57Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

chlowell · 2025-01-08T16:55:14Z

apps/confidential/confidential.go

@@ -345,6 +349,7 @@ func New(authority, clientID string, cred Credential, options ...Option) (Client
 	}
 	base.AuthParams.IsConfidentialClient = true

+	opts.logger.Log(context.Background(), logger.Info, "Created confidential client", logger.Field("clientID", clientID))


I wouldn't log this because it has no new information for the application: the caller knows that it called this function with this argument, and the error return communicates success/failure. If you still want to log this, you must set a timeout on the Context, and I suggest changing the level to debug

chlowell · 2025-01-08T16:55:53Z

apps/confidential/confidential.go

 	opts := clientOptions{
 		authority: authority,
 		// if the caller specified a token provider, it will handle all details of authentication, using Client only as a token cache
 		disableInstanceDiscovery: cred.tokenProvider != nil,
 		httpClient:               shared.DefaultClient,
 		azureRegion:              autoEnabledRegion,
+		logger:                   nil,


zero value of a pointer is nil

chlowell · 2025-01-08T16:58:12Z

apps/internal/logger/logger.go

+// If nil is provided a default logger instance is created.
+func New(slogLogger *slog.Logger) *Logger {
+	if slogLogger == nil {
+		defaultLogger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{}))


Why not slog.Default()?

chlowell · 2025-01-08T17:01:44Z

apps/internal/logger/logger.go

+	Error = slog.LevelError
+)
+
+type Logger struct {


You don't need to wrap slog types, you can simply export them with the same names. For example:

Suggested change

type Logger struct {

type Logger = slog.Logger

Then if you make this package slog, all your calling code can use the slog API directly on all versions of Go MSAL supports

chlowell · 2025-01-08T17:10:40Z

apps/internal/logger/logger_test.go

+
+	// Check the output
+	output := buf.String()
+	expectedMessages := []string{


Shouldn't this test the structure as well i.e., the attributes added to each message?

Added MD file

a58420b

4gust requested review from bgavrilMS and rayluo as code owners November 19, 2024 14:15

Updated callback support in 1.21 as well

57f484f

bgavrilMS approved these changes Nov 22, 2024

View reviewed changes

4gust changed the title ~~Added MD file~~ Praposal for logging support for msal-go Nov 25, 2024

Updated logging with files

deb5d6e

bgavrilMS reviewed Dec 9, 2024

View reviewed changes

* Pushes changes needed for logging, including removing support for 1…

e855af1

….18, and just focusing on the logger working on 1.21 and above * adds tests * refactors logger documentation * adds sample logger app

bgavrilMS reviewed Dec 19, 2024

View reviewed changes

AndyOHart added 4 commits December 23, 2024 15:27

* Changes how logger works so that we can create a logger implementat…

0ba0540

…ion for 1.21+ * Update tests

* remove unneeded log

77d735e

* moves context.Background to internal implementation

e6aa078

* Makes changes so WithLogger is only accessible for 1.21 and above

dffa979

chlowell requested changes Jan 7, 2025

View reviewed changes

AndyOHart added 4 commits January 7, 2025 16:00

* Updates logic to create default logger instance and remove returnin…

aa59b0f

…g of error * Updates tests

* Updates to remove LoggerInterface

da84180

* Adds missing functions to help compile go1.18

435bbb6

* Updates logger to remove LoggerInterface

1a2ccbf

chlowell reviewed Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Praposal for logging support for msal-go #535

Praposal for logging support for msal-go #535

4gust commented Nov 19, 2024

bgavrilMS left a comment

bgavrilMS Dec 9, 2024

bgavrilMS Dec 19, 2024

bgavrilMS Dec 19, 2024

bgavrilMS Dec 19, 2024

bgavrilMS Dec 19, 2024

chlowell Jan 7, 2025

chlowell Jan 7, 2025

chlowell Jan 7, 2025

AndyOHart Jan 7, 2025

chlowell Jan 7, 2025

chlowell Jan 7, 2025

AndyOHart Jan 7, 2025

chlowell Jan 7, 2025

bgavrilMS Jan 7, 2025

AndyOHart Jan 7, 2025

bgavrilMS Jan 7, 2025

AndyOHart Jan 7, 2025

chlowell Jan 7, 2025

AndyOHart Jan 7, 2025

sonarqubecloud bot commented Jan 8, 2025

chlowell Jan 8, 2025

chlowell Jan 8, 2025

chlowell Jan 8, 2025

chlowell Jan 8, 2025

chlowell Jan 8, 2025

Praposal for logging support for msal-go #535

Are you sure you want to change the base?

Praposal for logging support for msal-go #535

Conversation

4gust commented Nov 19, 2024

bgavrilMS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 8, 2025

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment