-
Notifications
You must be signed in to change notification settings - Fork 8
Sharing some ideas from fork #34
Description
Thanks for creating this library! 🙏 👏
I'd like to share some ideas around this library.
These were all implemented in my fork https://github.com/pazams/yasctx/ .
Most of these will be breaking changes in relation to this repo, however, if there's any appetite to accept some of those here, I'd be happy to port them over here!
Propagation to parent context
This features currently exists, however it's locked in the http subpackage and middleware
slog-context/http/middleware.go
Line 34 in ffd9d2f
| r = r.WithContext(newCtx(ctx, newSyncOrderedMap())) |
It would be nice to use this feature in grpc or any type of program, not necessarily just in http servers.
Dual vs Single flow
The library supports two flows (attached logger vs attached attributes). This obviously caters to more users, however there's also a cost. Consider this line
slog-context/http/middleware.go
Line 58 in ffd9d2f
| return slogctx.With(ctx, args...) |
AddToGroup()
The Prepend() and Append() APIs are descriptive names in regards to their implementation, i.e. where do they affect the attribute slice. From a DevX point of view, of someone who wants to pick up this library without diving into source code, it isn't clear their difference relates to the nesting level - root vs group.
When considering Prepend(), it might be more useful to name it as AddToRoot(). Or just Add().
When considering Append(), it gets more complicated. It doesn't add to a specific group, rather it adds to a random group, whomever is the more deeply nested? It can be argued that adding an explicit group name is more useful? e.g. AddToGroup() with a group name param.
No-config
If the scope of the library is adding log attributes via a context, any other piece of logic/middleware that wants to hook up to it can just grab a context and add to it. If they can't grab a context, then arguably it can be done with another wrapper handler, instead of plugging it through this library. This approach then leads to NewHandler() taking just a next param without further configuration.
P.S sorry for the extra empty PRs on this repo.. GitHub prioritizes the upstream (parent) repository during PR creation from forks, so I had a few misclicks when working on my fork.