Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(logging): Swap loggers from zap to log/slog #319

Merged
merged 30 commits into from
Jul 18, 2024

Conversation

zalgonoise
Copy link
Contributor

@zalgonoise zalgonoise commented Nov 1, 2023

This PR replaces go-uber/zap with Go standard library's log/slog. The internal/log package within the template folder now includes:

  • a New(level string) *slog.Logger function to replace the former NewAtLevel function. It still works pretty much the same way but does not return an error through safe defaults.
  • a NewSpanContextHandler(handler slog.Handler, withSpanID bool) slog.Handler function, that decorates a slog.Handler to add context-provided trace and span IDs as attributes (optional)
  • a InterceptorLogger(l *slog.Logger) logging.Logger function which could be used for gRPC services as a logging interceptor.

When adding the span context handler and interceptor logger logic, the tests started failing with an error:

--- FAIL: TestGT_InitNewProject (0.05s)
    --- FAIL: TestGT_InitNewProject/generates_folder_in_target_dir_and_initializes_it_with_go.mod_and_.git (0.01s)
        new_test.go:505: 
                Error Trace:    /Users/pinto_ramos_/mygo/github.com/SchwarzIT/go-template/pkg/gotemplate/new_test.go:505
                Error:          Received unexpected error:
                                template: :25: function "Name" not defined
                Test:           TestGT_InitNewProject/generates_folder_in_target_dir_and_initializes_it_with_go.mod_and_.git

After some debugging this error was coming from the text template parser, as it read through the test files, found two open braces and thought it should be handling a template. To address this issue, I've changed pkg/gotemplate not to parse the content with the text template if it is a *_test.go file. This approach can be discussed better in case we'd prefer to have an exclusion list for specific paths, instead.

Closes #311

@zalgonoise zalgonoise changed the title Feature/311/swap loggers to slog [311] Swap loggers from zap to log/slog Nov 1, 2023
@MarvinJWendt
Copy link
Member

MarvinJWendt commented Nov 2, 2023

Hi @zalgonoise, I see that you wrote all test cases manually for the logger. Go provides slogtest, which tests every function of a slog handler. We could use this :)

It checks that all keys and values are printed, that the groups work, and stuff like that.

@MarvinJWendt MarvinJWendt changed the title [311] Swap loggers from zap to log/slog feat(logging): Swap loggers from zap to log/slog Nov 2, 2023
@MarvinJWendt
Copy link
Member

MarvinJWendt commented Nov 2, 2023

I also edited the title to fit the conventional commits convention, as we usually use it:

image

We can discuss getting rid of it, and use GitHub labels instead. This would also make the releases prettier with GitHubs changelog generator.

PS: The issue number / reference is automatically added when we squash the PR.

@zalgonoise
Copy link
Contributor Author

zalgonoise commented Nov 6, 2023

Hi @zalgonoise, I see that you wrote all test cases manually for the logger. Go provides slogtest, which tests every function of a slog handler. We could use this :)

It checks that all keys and values are printed, that the groups work, and stuff like that.

Hi @MarvinJWendt :D I also looked into slogtest, at least when adding the span context middleware, but the reason why I scratched it for some simpler, more straight-forward tests, was because this package seems to be more prepared to test the actual slog.Handler implementations in the context of writing logs to an output.

So, when using the interface as middleware, it felt a bit like I was just testing the standard library's JSON slog.Handler just to look for an attribute! But give me some time to read through the package a bit more, if I can find a good way to use it without checking the other attributes :)

Copy link
Contributor

@johannes-riecken johannes-riecken left a comment

Choose a reason for hiding this comment

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

I just found this one minor simplification. Nice job!

_template/internal/log/context_test.go Outdated Show resolved Hide resolved
_template/internal/log/context_test.go Outdated Show resolved Hide resolved
@zalgonoise
Copy link
Contributor Author

I just found this one minor simplification. Nice job!

@johannes-riecken awesome input! Thanks for noticing, I don't know how I let that slide! :D

@zalgonoise
Copy link
Contributor Author

Looking into this (and other) PR's CI jobs, it seems that the semgrep one is failing:

example job

Prepare workflow directory
Prepare all required actions
Getting action download info
Error: Unable to resolve action `returntocorp/semgrep-action@v1`, unable to find version `v1`

Looking into semgrep's action on Github, it seems that they've marked it as deprecated. They point to the following guide for Github

The problem that I am having is their requirement to login in order to have the Sarif output that is used for github/codeql-action/upload-sarif. I tried running the semgrep container locally with:

docker run -it -v "${PWD}:/src" returntocorp/semgrep:latest semgrep ci --config 'p/auto'

The command above runs fine (and we can include it in the Makefile as well). However if I add the --sarif flag, it asks to login first. I guess that this is where the Github token comes in, in their guide.

docker run -it -v "${PWD}:/src" returntocorp/semgrep:latest semgrep ci --sarif          

run `semgrep login` before using `semgrep ci` or set `--config`

Does anyone have a better idea on how we should approach this change, for semgrep? :)

@zalgonoise
Copy link
Contributor Author

@MarvinJWendt another update regarding slogtest :D

Unfortunately, we are unable to use it for this particular wrapper / decorator. The problem with slogtest is that it has zero tests using context!!

While that makes it a great suggestion for the experimental repo, we need that context for those tests. A regular (*slog.Logger).Info call uses a context.Background() when it hits the Handle call in the slog.Handler :)

@zalgonoise
Copy link
Contributor Author

zalgonoise commented Dec 27, 2023

Waiting for #328, to run CI workflows again

@zalgonoise
Copy link
Contributor Author

I noticed that I left an unresolved comment, it's already as an accepted change :D @johannes-riecken would you like to take another look? :)

@zalgonoise zalgonoise force-pushed the feature/311/swap-loggers-to-slog branch from 9a56234 to 3957031 Compare February 23, 2024 13:07
@zalgonoise zalgonoise requested a review from a team as a code owner February 23, 2024 13:07
@zalgonoise
Copy link
Contributor Author

resolved conflicts on go.mod / go.sum :)

while we're at it, updated all dependencies (within Go 1.21). We're still able to do the jump to 1.22 at any time :D probably in a dedicated issue / PR

@MarvinJWendt
Copy link
Member

image

GOMAXPROCS seems to be using logfmt. Can we make it use our logger? 🤔

@zalgonoise zalgonoise force-pushed the feature/311/swap-loggers-to-slog branch from a4f5b17 to bff1dd7 Compare July 18, 2024 13:44
zalgonoise and others added 18 commits July 18, 2024 15:45
Co-authored-by: Johannes Riecken <johannes.riecken@gmail.com>
…g/slog; added trace context handler; added gRPC interceptor logger
…g/slog; added trace context handler; added gRPC interceptor logger
…g/slog; added trace context handler; added gRPC interceptor logger
…g/slog; added trace context handler; added gRPC interceptor logger
…g/slog; added trace context handler; added gRPC interceptor logger
…g/slog; added trace context handler; added gRPC interceptor logger
@zalgonoise zalgonoise force-pushed the feature/311/swap-loggers-to-slog branch from bff1dd7 to 3872c0b Compare July 18, 2024 13:45
…g/slog; added trace context handler; added gRPC interceptor logger
…g/slog; added trace context handler; added gRPC interceptor logger
…g/slog; added trace context handler; added gRPC interceptor logger
@zalgonoise zalgonoise merged commit 6c36459 into main Jul 18, 2024
3 checks passed
@zalgonoise zalgonoise deleted the feature/311/swap-loggers-to-slog branch July 18, 2024 14:23
@MarvinJWendt MarvinJWendt added the feature A new feature label Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to slog
3 participants