Skip to content

yet another proposal for server logging using slog#835

Open
istyf wants to merge 11 commits intogopcua:mainfrom
istyf:fix/server-logging-using-slog
Open

yet another proposal for server logging using slog#835
istyf wants to merge 11 commits intogopcua:mainfrom
istyf:fix/server-logging-using-slog

Conversation

@istyf
Copy link
Copy Markdown

@istyf istyf commented Nov 28, 2025

This is a proposal for allowing the gopcua server to log using slog "without knowing" that log/slog is the actual logging backend. The application using the gopcua server, initialises the logger with a ualog.New(...) call that returns a context.Context that is then passed to the server.New function.

This PR is loosely based on PR #800 (and discussions around it) but keeps the log handler management outside of the library. Its changes should not change any behaviours except for a potential crash at https://github.com/gopcua/opcua/blob/main/server/node.go#L271:L274 where a reference type id was dereferenced even if it was nil.

I have not spent a ton of time on documentation and examples before getting any feedback, but there is/are some in the ualog package's docs.go and ualog_test.go.

@istyf
Copy link
Copy Markdown
Author

istyf commented Nov 28, 2025

This PR focuses on server logging because that is where my personal need currently is, but also to reduce the size of the PR. At 37 changed files it is already quite big. If it gains acceptance I am of course happy to make the corresponding changes in the client code as well.

In the comment trail on PR 800 the ability to add source location to the output is discussed. This PR does not allow that because the source location is lost when we wrap slog. It is doable if someone finds it to be a deal breaker, but I personally do not find any value in logging source location over just greping/searching for the log message.

@istyf
Copy link
Copy Markdown
Author

istyf commented Nov 28, 2025

One more thing. To keep the size of the PR down I have only added ctx arguments to the functions that are actually doing any logging right now. If this PR is accepted I think it would be a good idea to add _ context.Context at least to interfaces and public methods to not require a potentially breaking change in future versions just because a function decides that it wants to log something. That could be done in a separate cleanup PR though.

@istyf
Copy link
Copy Markdown
Author

istyf commented Feb 26, 2026

@JeremyTheocharis @kung-foo I would love to get some feedback on this to see if it is worth investing more time in.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant