log icon indicating copy to clipboard operation
log copied to clipboard

feat: support slog attributes

Open op opened this issue 9 months ago • 3 comments

This adds support for slog attributes and fixes #119.

op avatar May 07 '24 15:05 op

Codecov Report

Attention: Patch coverage is 85.71429% with 6 lines in your changes missing coverage. Please review.

Project coverage is 82.63%. Comparing base (2338a13) to head (6594782). Report is 23 commits behind head on main.

Files Patch % Lines
json.go 85.36% 6 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #127      +/-   ##
==========================================
+ Coverage   80.92%   82.63%   +1.71%     
==========================================
  Files          11       11              
  Lines         739      714      -25     
==========================================
- Hits          598      590       -8     
+ Misses        126      108      -18     
- Partials       15       16       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 07 '24 15:05 codecov[bot]

This adds support for slog attributes and fixes #119.

TODO

Current PR assume we're on Go 1.21. Open question:

  1. Upgrade to require Go 1.21. This would allow merging of logger.go and logger_121.go and simplify existing code base.
  2. Split all slog stuff out into json_go121.go and further complicate code base.

@aymanbagabas what do you think?

I would say keep supporting Go 1.19 and split code. We can use type aliases to simplify things. For example, slog.Attr can be defined as type slogAttr = slog.Attr for both Go 1.19 and Go 1.21. The same goes for the other imported slog types. This way we don't need to import slog in json.go.

aymanbagabas avatar May 07 '24 15:05 aymanbagabas

@aymanbagabas very cool idea! that worked pretty nice.

op avatar May 07 '24 15:05 op

@aymanbagabas hi, did you see that all comments has been addressed? :)

op avatar Jun 02 '24 20:06 op

Hey @op! Thanks for addressing the comments. I tried running the example from #119, but I think the implementation still doesn't support arbitrary maps, which is the whole point of the issue.

aymanbagabas avatar Jun 04 '24 15:06 aymanbagabas

Hey @op! Thanks for addressing the comments. I tried running the example from #119, but I think the implementation still doesn't support arbitrary maps, which is the whole point of the issue.

No worries. Good catch @aymanbagabas! Didn't realise the slog Handle function worked differently. It was previously calling attr.Value.String(). I dropped the .String() call to allow the unmodified slog.Value flow through all formatters. For the JSON formatter, I've added a check to make sure it's output is identical in "slog mode" too now.

Fixing the text handler seem like a future improvement to me. Sorry, I don't have time to look at that.

op avatar Jun 04 '24 19:06 op

@aymanbagabas ping! :penguin:

op avatar Jun 18 '24 20:06 op

@aymanbagabas seems like govulncheck doesn't work with 1.19. Looks like https://github.com/golang/vuln/commit/745db65f35062023c5994fceb1931ef839777007 introduces slices.

op avatar Jun 18 '24 20:06 op

@aymanbagabas seems like govulncheck doesn't work with 1.19. Looks like golang/vuln@745db65 introduces slices.

Hmm, seems like golang/vuln needs to update their go.mod

Anyway, this looks good, thanks again @op!

aymanbagabas avatar Jun 18 '24 20:06 aymanbagabas