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

RFD 0154 Logging Guidelines #34173

Merged
merged 1 commit into from
Nov 8, 2023
Merged

RFD 0154 Logging Guidelines #34173

merged 1 commit into from
Nov 8, 2023

Conversation

rosstimothy
Copy link
Contributor

@mdwn
Copy link
Contributor

mdwn commented Nov 2, 2023

Massive +1 to this.

@strideynet strideynet self-requested a review November 3, 2023 10:22
strideynet
strideynet reviewed Nov 3, 2023
View reviewed changes
rfd/0154-logging-guidelines.md Show resolved Hide resolved
rfd/0154-logging-guidelines.md Show resolved Hide resolved
rfd/0154-logging-guidelines.md Show resolved Hide resolved
@rosstimothy rosstimothy marked this pull request as ready for review November 3, 2023 13:36
@rosstimothy rosstimothy requested a review from zmb3 November 3, 2023 13:36
@github-actions github-actions bot added rfd Request for Discussion size/md labels Nov 3, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 3, 2023

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@rosstimothy rosstimothy added the no-changelog Indicates that a PR does not require a changelog entry label Nov 3, 2023
codingllama
codingllama approved these changes Nov 7, 2023
View reviewed changes
Copy link
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

This is great, ship it!

rfd/0154-logging-guidelines.md Show resolved Hide resolved
rfd/0154-logging-guidelines.md Show resolved Hide resolved
rfd/0154-logging-guidelines.md Outdated Show resolved Hide resolved
rfd/0154-logging-guidelines.md Outdated Show resolved Hide resolved
rfd/0154-logging-guidelines.md Outdated Show resolved Hide resolved
rfd/0154-logging-guidelines.md Show resolved Hide resolved
@rosstimothy
Copy link
Contributor Author

For the curious, #34072 contains an implementation of the slog handlers which match the output of our existing logrus formatters. It contains benchmarks comparing the custom slog handlers to our current logrus formatters and the standard slog handlers.

zmb3
zmb3 approved these changes Nov 8, 2023
View reviewed changes
rfd/0154-logging-guidelines.md Show resolved Hide resolved
@rosstimothy rosstimothy added this pull request to the merge queue Nov 8, 2023
Merged via the queue into master with commit c6193f4 Nov 8, 2023
32 checks passed
@rosstimothy rosstimothy deleted the rfd/0154-logging-guidelines.md branch November 8, 2023 22:47
@rosstimothy rosstimothy mentioned this pull request Nov 30, 2023
Closed
@ravicious
Copy link
Member

The message should be a fragment, and not a full sentence. Terminating the message with punctuation should be avoided.

Does it mean it should be "lorem ipsum dolor sit amet" or "Lorem ipsum dolor sit amet"? Most if not all log messages I wrote used to follow "Lorem ipsum dolor sit amet.".

@ravicious ravicious mentioned this pull request Jun 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codingllama codingllama codingllama approved these changes

@zmb3 zmb3 zmb3 approved these changes

@strideynet strideynet strideynet approved these changes

Assignees
No one assigned
Labels
no-changelog Indicates that a PR does not require a changelog entry rfd Request for Discussion size/md
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rosstimothy @mdwn @ravicious @zmb3 @codingllama @strideynet