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

Drop klog and migrate to logging package #1178

Merged
merged 3 commits into from
Apr 28, 2022
Merged

Drop klog and migrate to logging package #1178

merged 3 commits into from
Apr 28, 2022

Conversation

dramich
Copy link
Contributor

@dramich dramich commented Apr 26, 2022

  • Go mod update
  • Update logging package and flag parsing
  • Swap klog to log package

What does this PR change?

Removed klog from all of our logging - it is still used in packages we depend on (k8s-client)
Moved all klog calls to our logging package which now uses zerolog under the hood
Defaulted to using a pretty style printing to keep the output similar to existing logging. This can be changed to json with a flag/envvar which should technically be faster. We don't fully take advantage of the structured part yet but it would still be better than nothing if a user wants to use it.

New logging format:

2022-04-27T10:00:11.28658-07:00 ??? Log level set to info
2022-04-27T10:00:11.286854-07:00 INF Starting cost-model (git commit "1.91.0-rc.0")
2022-04-27T10:00:11.286939-07:00 FTL No address for prometheus set in $PROMETHEUS_SERVER_ENDPOINT. Aborting.

Old logging format:

I0427 09:35:51.473189   31474 router.go:1351] Starting cost-model (git commit "1.91.0-rc.0")
F0427 09:35:51.473367   31474 router.go:1370] No address for prometheus set in $PROMETHEUS_SERVER_ENDPOINT. Aborting.

Note that the file and line number has been dropped. If this is deemed required it can be added back or have a flag to disable/enable it

Does this PR relate to any other PRs?

Not yet

How will this PR impact users?

The logging output will look different but similar. See above about JSON stuff.

Does this PR address any GitHub or Zendesk issues?

#1170

How was this PR tested?

Swapped out logging and run kubecost

Does this PR require changes to documentation?

Have you labeled this PR and its corresponding Issue as "next release" if it should be part of the next Kubecost release? If not, why not?

Copy link
Contributor

@nikovacevic nikovacevic left a comment

Choose a reason for hiding this comment

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

Looks great. I'll want to do another pass, but as a quick skim, just caught one apparent typo.

pkg/services/clusterservice.go Outdated Show resolved Hide resolved
@Adam-Stack-PM Adam-Stack-PM added next release This PR/issue is expected to be merged/addressed in the next release v1.93 labels Apr 27, 2022
Copy link
Contributor

@michaelmdresser michaelmdresser left a comment

Choose a reason for hiding this comment

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

Looks great, nice touches adjusting a couple of the overly-verbose log levels!

Comment on lines +87 to +89
log.Info().Msgf(fmt.Sprintf("[Profiler] %s", format), a...)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Profilef seems like a good candidate for a somewhat structured log, maybe:

// "type" is such an overloaded word, I'm not sure
// what the best practice is for structured log keys
log.Info().Str("type", "profile").Msgf(...) 

What do you think?

I totally understand if you want to leave this kind of update for another time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, how and what to add is a good question. We could potentially turn the profile logger into a sub logger i.e. https://github.com/rs/zerolog#sub-loggers-let-you-chain-loggers-with-additional-context
I don't have enough context yet to say where to go with the profile logger, perhaps it should end up behind a flag so it's not always printing logs since profiling is usually resource intensive and not always needed.

@mbolt35
Copy link
Contributor

mbolt35 commented Apr 27, 2022

This is beautiful - Viper looks helpful, maybe we can leverage it more in some other areas where we have config chaos.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release This PR/issue is expected to be merged/addressed in the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants