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.

Sign up for GitHub

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

Jump to bottom

switch to using logr for structured logging #1409

Merged
merged 16 commits into from
Mar 13, 2019
Merged

switch to using logr for structured logging #1409

merged 16 commits into from
Mar 13, 2019

Conversation

munnerz
Copy link
Member

@munnerz munnerz commented Feb 25, 2019 •
edited

What this PR does / why we need it:

This is a first pass at us switching to logr for structured logging.

I've chosen to plumb the actual logr instance through via the context.Context (golang context) variable, as it also allows us to handle cancellation etc all in one.

I've not converted the entire codebase yet - we need to continue going through:

  • plumbing context through
  • adding log := logf.FromContext(ctx)
  • replacing all calls to klog.* with log.*
  • replacing all calls to runtime.HandleError with log.Error(err, "...")

This will probably take a while to do properly, but can at least be split up into different chunks 😄

We should also work out what sort of log levels we want to support. I'd like to have a relatively quiet 'info' level, then one that includes debug information, and potentially another that logs all of the things (i.e. before and after), similar to 'trace'. I'm not sure if we're better to not include 'trace' for now, in favour of a very chatting debug level 😄

I'd also like to include some kind of 'sync uuid' field in the logger, so that users can correlate all the logs from a specific call to the Sync function when debugging.

Which issue this PR fixes: fixes #712

Special notes for your reviewer:

Release note:

Switch to structured logging using logr

@jetstack-bot jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. area/acme Indicates a PR directly modifies the ACME Issuer code area/api Indicates a PR directly modifies the 'pkg/apis' directory labels Feb 25, 2019
@jetstack-bot jetstack-bot added area/ca Indicates a PR directly modifies the CA Issuer code approved Indicates a PR has been approved by an approver from all required OWNERS files. area/monitoring Indicates a PR or issue relates to monitoring labels Feb 25, 2019
@munnerz munnerz added this to the Next milestone Feb 25, 2019
@jetstack-bot jetstack-bot added area/vault Indicates a PR directly modifies the Vault Issuer code size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 25, 2019
@munnerz
Copy link
Member Author

munnerz commented Feb 25, 2019

It may also be easier if I split this PR up into:

  • add logger scaffolding
  • for each package, a commit converting it to use the logger
  • vendor updates

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2019
@jetstack-bot jetstack-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 28, 2019
@jetstack-bot jetstack-bot added the area/acme/http01 Indicates a PR modifies ACME HTTP01 provider code label Feb 28, 2019
@munnerz munnerz force-pushed the klogr branch 2 times, most recently from d3f649f to a35d6e7 Compare March 1, 2019 00:16
@jetstack-bot jetstack-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/testing Issues relating to testing and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 1, 2019
@munnerz munnerz force-pushed the klogr branch 2 times, most recently from d33187f to ea5a728 Compare March 1, 2019 01:06
@jetstack-bot jetstack-bot added the dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. label Mar 1, 2019
@munnerz munnerz added this to the v0.8 milestone Mar 6, 2019
@munnerz munnerz added this to To do in v0.8 Mar 6, 2019
@munnerz munnerz moved this from To do to In progress in v0.8 Mar 6, 2019
kragniz
kragniz approved these changes Mar 12, 2019
View reviewed changes
Copy link
Contributor

@kragniz kragniz left a comment

Choose a reason for hiding this comment

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

This looks good to me, give me a ping when it's rebased for a lgtm

v0.8 automation moved this from In progress to Reviewer approved Mar 12, 2019
@munnerz
Update logs package for go-logr
5d73076 
Signed-off-by: James Munnelly <james@munnelly.eu>
@munnerz
Update acmeorders controller
708313a 
Signed-off-by: James Munnelly <james@munnelly.eu>
@munnerz
Update certificates controller
9271315 
Signed-off-by: James Munnelly <james@munnelly.eu>
@munnerz
Update issuers controller
d7c9d39 
Signed-off-by: James Munnelly <james@munnelly.eu>
@munnerz
Update metrics package
451b12f 
Signed-off-by: James Munnelly <james@munnelly.eu>
@munnerz
Update CA issuer
591f8b6 
Signed-off-by: James Munnelly <james@munnelly.eu>
@munnerz
Update ACME issuer
6f743db 
Signed-off-by: James Munnelly <james@munnelly.eu>
@munnerz
Use logr in pkg/util/kube
a135430 
Signed-off-by: James Munnelly <james@munnelly.eu>
@munnerz
Update cainjector
6b24e9c 
Signed-off-by: James Munnelly <james@munnelly.eu>
@munnerz
Update clusterissuers controller
c6c4983 
Signed-off-by: James Munnelly <james@munnelly.eu>
@munnerz
Update acmesolver to use logr
46faba0 
Signed-off-by: James Munnelly <james@munnelly.eu>
@munnerz
Define pkg/api.Scheme with all used types registered
f4c0de2 
Signed-off-by: James Munnelly <james@munnelly.eu>
@munnerz
Update acmechallenges controller
eaeefdf 
Signed-off-by: James Munnelly <james@munnelly.eu>
@munnerz
Use goflag instead of pflag for logs
420ebfe 
Signed-off-by: James Munnelly <james@munnelly.eu>
@munnerz
Run update-bazel
d282c5f 
Signed-off-by: James Munnelly <james@munnelly.eu>
@munnerz
Fix controller test pkg
aeaa341 
Signed-off-by: James Munnelly <james@munnelly.eu>
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kragniz, munnerz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2019
@munnerz
Copy link
Member Author

munnerz commented Mar 12, 2019

I've rebased - it appears to be flaking on DNS01 tests, but I can't see anything related to this PR.

/retest

@munnerz
Copy link
Member Author

munnerz commented Mar 13, 2019

/retest

@munnerz munnerz added the lgtm Indicates that a PR is ready to be merged. label Mar 13, 2019
@jetstack-bot jetstack-bot merged commit 05517af into cert-manager:master Mar 13, 2019
v0.8 automation moved this from Reviewer approved to Done Mar 13, 2019
@bradjones1 bradjones1 mentioned this pull request Feb 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kragniz kragniz kragniz approved these changes

@DanielMorsing DanielMorsing Awaiting requested review from DanielMorsing

@vdesjardins vdesjardins Awaiting requested review from vdesjardins

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/acme/http01 Indicates a PR modifies ACME HTTP01 provider code area/acme Indicates a PR directly modifies the ACME Issuer code area/api Indicates a PR directly modifies the 'pkg/apis' directory area/ca Indicates a PR directly modifies the CA Issuer code area/monitoring Indicates a PR or issue relates to monitoring area/testing Issues relating to testing area/vault Indicates a PR directly modifies the Vault Issuer code dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
No open projects
v0.8
  
Done
Milestone
v0.8
Development

Successfully merging this pull request may close these issues.

Feature Request: Structured Logging
3 participants
@munnerz @jetstack-bot @kragniz