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

make context.Keys available as LogFormatterParams #1779

Merged
merged 4 commits into from Mar 7, 2019
Merged

make context.Keys available as LogFormatterParams #1779

merged 4 commits into from Mar 7, 2019

Conversation

MusicAdam
Copy link
Contributor

This makes the context available in LogFormatterParams and therefore allows access to context variables as part of the log output formatting.

@codecov
Copy link

codecov bot commented Feb 23, 2019

Codecov Report

Merging #1779 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1779      +/-   ##
==========================================
+ Coverage   98.62%   98.62%   +<.01%     
==========================================
  Files          41       41              
  Lines        2112     2113       +1     
==========================================
+ Hits         2083     2084       +1     
  Misses         18       18              
  Partials       11       11
Impacted Files Coverage Δ
logger.go 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 057f63b...79a0760. Read the comment docs.

@thinkerou
Copy link
Member

@MusicAdam why need it? can you post one usage scenario? thanks!

@thinkerou
Copy link
Member

closing, thanks!

@thinkerou thinkerou closed this Mar 1, 2019
@MusicAdam
Copy link
Contributor Author

@thinkerou Its useful for logging context variables, such as a request-id. It makes the logger more robust.

@appleboy appleboy reopened this Mar 6, 2019
@sairoutine
Copy link
Contributor

Instead of LogFormatterParams containing the context, it is better to add the its parameters or value to LogFormatterParams's field directly.

The context in Logger is useful. However, unnecessary methods of context can be used in the Logger. (For example c.Abort(), c.JSON(), c.Next() etc...)

Calling these functions may cause the unexpected bugs.

@MusicAdam MusicAdam changed the title make context available as LogFormatterParams make context.Keys available as LogFormatterParams Mar 6, 2019
@MusicAdam
Copy link
Contributor Author

Instead of LogFormatterParams containing the context, it is better to add the its parameters or value to LogFormatterParams's field directly.

The context in Logger is useful. However, unnecessary methods of context can be used in the Logger. (For example c.Abort(), c.JSON(), c.Next() etc...)

Calling these functions may cause the unexpected bugs.

This is a good point. I have updated so that context.Keys are passed instead. Since the Keys field is exported, I think it is safe to pass it directly instead of something more complicated like a copy or a separate accessor struct to allow Key retrieval.

@sairoutine
Copy link
Contributor

Great!

@appleboy appleboy added this to the 1.4 milestone Mar 7, 2019
@appleboy appleboy requested a review from thinkerou March 7, 2019 01:43
Copy link
Member

@thinkerou thinkerou left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@thinkerou thinkerou merged commit 3dc2478 into gin-gonic:master Mar 7, 2019
wybcp added a commit to wybcp/gin that referenced this pull request Mar 7, 2019
* master:
  make context.Keys available as LogFormatterParams (gin-gonic#1779)
  spell check  (gin-gonic#1796)
  chore: use internal/json (gin-gonic#1791)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants