Skip to content

Conversation

@ndyakov
Copy link
Member

@ndyakov ndyakov commented Nov 25, 2025

Continue the work on #3560 opened by @ccoVeille

ccoVeille and others added 7 commits November 19, 2025 23:10
This commit introduces an optional logger parameter to various structs.
This enhancement allows users to provide custom logging implementations.
An issue will remain for direct calls to internal.Logger.Printf, as the
call depth cannot be adjusted there without changing the function signature.
@ndyakov ndyakov force-pushed the ndyakov/optional-logger branch from cb7a509 to 8bf1376 Compare November 25, 2025 16:20
@ndyakov ndyakov force-pushed the ndyakov/optional-logger branch from 4b03ae0 to f2406a6 Compare November 26, 2025 10:15
legacyLoggerWithLevel.Errorf(ctx, format, v...)
return
}
cl.logger.ErrorContext(ctx, format, v...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ErrorContext etc. will cause inaccurate source log attr.

Should use cl.logger.Handler().Handleinstead when wrapping, see slog godoc Example (Wrapping).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ccoVeille Maybe you would like to check this.

@wzy9607 thank you for pointing this out

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like there is an error here, yes

it should be

	cl.logger.ErrorContext(cl.printfToStructured(ctx, format, v...))

I will fix it.

I created the printToStructured to be able to handle the right format

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have fixed the issue in #3622

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe the fix was lost when you force pushed? Can we just add a test with slog that it will work as expected?

@ccoVeille
Copy link

I feel like this PR can be closed now, as #3622 replaces it

@ndyakov
Copy link
Member Author

ndyakov commented Nov 27, 2025

closed in favour of #3622

@ndyakov ndyakov closed this Nov 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants