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

streamline logging #844

Merged
merged 2 commits into from
Oct 5, 2022
Merged

streamline logging #844

merged 2 commits into from
Oct 5, 2022

Conversation

vroldanbet
Copy link
Contributor

@vroldanbet vroldanbet commented Sep 26, 2022

Part of #753
Built on top of jzelinskie/cobrautil#19

What

Adjustments to homogenize and streamline how logging is used in SpiceDB.

How

Not all logging messages have been revisited. That will be tackled in a follow up. This PR sets the foundation.

  • migrates to cobrautil v2
  • makes it possible to stop relying on zerolog global logger, and thus be able to have our own default logger (disabled)
  • configures logger to be non-blocking
  • configures same logger for OTel library
  • logging is now disabled by default in our tests (no more need to do zerolog.SetGlobalLevel(zerolog.Disabled) in each test)
  • zerolog.Ctx() is used in the codebase frequently, but because we don't set the default logger for contet-based logging. We are now setting zerolog.DefaultContextLogger whenever our own global logger is set.
  • add missing Close on http downloader server and log error

The changeset is relatively simple, it's just that lots of imports had to be adjusted. Files to look into are:

Example startup at INFO level

image

Example startup at DEBUG level

image

@github-actions github-actions bot added area/api v0 Affects the v0 API area/api v1 Affects the v1 API area/CLI Affects the command line area/dashboard Affects the dashboard area/datastore Affects the storage system area/dependencies Affects dependencies area/dispatch Affects dispatching of requests area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Sep 26, 2022
@vroldanbet vroldanbet force-pushed the logging-improvements branch 4 times, most recently from 4c1c3ee to a27f236 Compare September 27, 2022 10:35
@vroldanbet vroldanbet force-pushed the logging-improvements branch 4 times, most recently from 8e5bec8 to 7750ef2 Compare October 4, 2022 08:19
@vroldanbet vroldanbet self-assigned this Oct 4, 2022
@vroldanbet vroldanbet force-pushed the logging-improvements branch from 7750ef2 to 44bd3f9 Compare October 4, 2022 08:22
@vroldanbet vroldanbet changed the title revamps logging streamline logging Oct 4, 2022
@vroldanbet vroldanbet force-pushed the logging-improvements branch from 44bd3f9 to 2900f1a Compare October 4, 2022 09:05
@vroldanbet vroldanbet marked this pull request as ready for review October 4, 2022 09:16
@vroldanbet vroldanbet requested review from a team and jakedt October 4, 2022 09:16
@vroldanbet vroldanbet enabled auto-merge October 4, 2022 09:32
Copy link
Member

@jzelinskie jzelinskie left a comment

Choose a reason for hiding this comment

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

Only minor nitpicks

internal/logging/logger.go Outdated Show resolved Hide resolved
internal/logging/logger.go Outdated Show resolved Hide resolved
pkg/cmd/root.go Outdated Show resolved Hide resolved
@vroldanbet vroldanbet force-pushed the logging-improvements branch from 2900f1a to 2f01c07 Compare October 5, 2022 18:09
- migrates to cobrautil v2
- configures logger to be non-blocking
- configures same logger for OTel library
- logging is now disabled by default in our tests
- adds missing call to Close() in download server
@vroldanbet vroldanbet force-pushed the logging-improvements branch from 2f01c07 to c5c8b26 Compare October 5, 2022 18:14
- remove warning in favour of key-value
- add missing message when serving http plain-text
- align message wording
- change "prefix" with "service" in log messages
- log error stopping HTTP server as error, not warn
- incorrect error message "failed while serving http",
  it was unclear if it was http or https
@vroldanbet vroldanbet force-pushed the logging-improvements branch from c5c8b26 to c354661 Compare October 5, 2022 18:19
@vroldanbet vroldanbet requested a review from jzelinskie October 5, 2022 18:19
@vroldanbet vroldanbet merged commit 4247105 into main Oct 5, 2022
@vroldanbet vroldanbet deleted the logging-improvements branch October 5, 2022 18:25
@github-actions github-actions bot locked and limited conversation to collaborators Oct 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/api v0 Affects the v0 API area/api v1 Affects the v1 API area/CLI Affects the command line area/dashboard Affects the dashboard area/datastore Affects the storage system area/dependencies Affects dependencies area/dispatch Affects dispatching of requests area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants