Skip to content

Sharing some ideas from fork #34

@pazams

Description

@pazams

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

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

return slogctx.With(ctx, args...)
- it's a nice fallback but in doing so chooses one of the flows (attached logger). The existence of two flows might force other features to guess and pick just one. In the example above one flow gets to have fault tolerance with respect to the http middleware, while another doesn't.

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions