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

Simple sync log. #327

Merged
merged 6 commits into from
Jul 20, 2016
Merged

Simple sync log. #327

merged 6 commits into from
Jul 20, 2016

Conversation

ChrisHines
Copy link
Member

Implements #246. Simpler alternative to #273.

This PR adds two simple concurrency safety helpers to package log and also rewrites the package docs to help guide a new user gently into the log package. It also adds or rewrites several package level executable examples.

Please review. @peterbourgon

@ChrisHines
Copy link
Member Author

The CI failures were caused by the kit/tracing/opentracing package.

// Using a SyncLogger has the benefit that it guarantees each log event is
// handled atomically within the wrapped logger, but it typically serializes
// both the formatting and output logic. Use a SyncLogger if the formatting
// logger may perform multiple writes per log event.
Copy link
Member

Choose a reason for hiding this comment

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

Amazing doc comment. Thank you.

@peterbourgon
Copy link
Member

If you rebase on master, those build failures should go away. Assuming that's true, LGTM! If not, let me know and I'll take care of it.

- Add sections for basic usage, log contexts, dynamic context values,
  and concurrent safety.
- Add executable tests that illustrate the new topics.
- Update README.md to use log.NewSyncWriter in examples.
@ChrisHines ChrisHines merged commit 3880c6f into master Jul 20, 2016
@ChrisHines ChrisHines deleted the simple-sync-log branch July 20, 2016 15:12
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.

2 participants