Skip to content

log: support context-logging #611

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

Merged
merged 3 commits into from
May 23, 2025
Merged

log: support context-logging #611

merged 3 commits into from
May 23, 2025

Conversation

protolambda
Copy link
Collaborator

Description

Changes:

  • Support the context-logging interface of slog.Logger
    • Additionally includes TraceContext.
    • Intentionally does not include CritContext (crits are discouraged, and we would never want to filter from output based on context)
  • Add WriteCtx to complement the existing Write method
  • Allow it to set a default context on the logger, to use in case none is specified
  • Support slog.Attr in regular log calls
  • Fix test-logger to actually handle Write
  • Fix test-logger to handle Crit more nicely

Tests

  • Test context is preserved in logging, so that the handler can use it
  • Test that testlog Crit does work nicely
  • Test that slog.Attr can be used in regular log calls
  • Vmodule test still works (call-site detection)

Additional context

This helps us remove a big logging hack in our monorepo testing, so that we can do context-logging and test tracing properly.

We can also attach the devtest test-scope context to our loggers, so logs can be attributed to scopes with context.

And we can attach attributes like service-ID to package-level loggers, so we can make the log-handler easily filter down to only logs with relevant context.

Metadata

@protolambda protolambda requested a review from a team as a code owner May 22, 2025 20:27
@protolambda
Copy link
Collaborator Author

CI is broken due to upstream EELS release issue. Fixing in #612

Copy link
Member

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

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

Would be nice to open this PR upstream! Seems like a general improvement that's compatible with the existing API.

@protolambda protolambda merged commit e3f85bf into optimism May 23, 2025
11 checks passed
@protolambda protolambda deleted the context-logging branch May 23, 2025 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants