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

Migrate to slog #3376

Merged
merged 25 commits into from
Oct 7, 2024
Merged

Migrate to slog #3376

merged 25 commits into from
Oct 7, 2024

Conversation

bufdev
Copy link
Member

@bufdev bufdev commented Oct 6, 2024 •
edited
Loading

This uses the same zapcore.Core, so logging output is exactly the same for now - we rely on the log format for printing warnings in a certain way, and the log format as-is is nicer for CLI output anyways. We can look to change that out in the future.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 6, 2024 •
edited
Loading

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedOct 7, 2024, 3:30 PM

@bufdev bufdev changed the title WIP: Migrate from zap to slog Migrate from zap to slog Oct 6, 2024
@bufdev bufdev changed the title Migrate from zap to slog Migrate from zap to slog (do not merge yet) Oct 6, 2024
@bufdev
Copy link
Member Author

bufdev commented Oct 6, 2024

Big refactor just pushed up:

  • Renamed slogbuild to slogapp and zapbuild to zapapp, as this really matches the other package names with regards to their purpose.
  • Added appext.LogFormat and appext.LogLevel, including parsing for them.
  • Made slogapp and zapapp talk in terms of appext.LogFormat and appext.LogLevel, as opposed to doing their own parsing.
  • Added appext.BuilderWithLoggerProvider to allow custom loggers to be provided.
  • Made a default LoggerProvider not rely on anything but the stdlib, so appext does not depend on slogapp/zapapp.
  • Called appext.BuilderWithLoggerProvider(slogapp.NewLogger) in relevant places in the CLI.

Core should use appext.BuilderWithLoggerProvider to create a new *slog.Logger based on the input io.Writer, appext.LogLevel, appext.LogFormat.

@bufdev bufdev changed the title Migrate from zap to slog (do not merge yet) Migrate to slog Oct 7, 2024
bufdev
bufdev commented Oct 7, 2024
View reviewed changes
private/pkg/slogapp/slogapp.go Outdated

// LoggerProvider is an appext.LoggerProvider for use with appext.BuilderWithLoggerProvider.
func LoggerProvider(container appext.NameContainer, logLevel appext.LogLevel, logFormat appext.LogFormat) (*slog.Logger, error) {
core, err := zapapp.NewCore(container.Stderr(), logLevel, logFormat)
Copy link
Member Author

Choose a reason for hiding this comment

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

Minor cleanup: this should call NewLogger

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@bufdev bufdev merged commit e4340cc into main Oct 7, 2024
10 checks passed
@bufdev bufdev deleted the slog branch October 7, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfridman mfridman mfridman approved these changes

@emcfarlane emcfarlane emcfarlane approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bufdev @emcfarlane @mfridman