Skip to content

Add new best practices for structured logging and accepting loggers #370

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
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add new best practices for structured logging and accepting loggers #370

wants to merge 1 commit into from
+212 −14

Conversation

FranzBusch
Copy link
Member

No description provided.

@FranzBusch FranzBusch added the semver/none No version bump required. label Aug 18, 2025
glbrntt
glbrntt reviewed Aug 18, 2025
View reviewed changes
Sources/Logging/Docs.docc/BestPractices/003-AcceptingLoggers.md

#### Avoid: Libraries creating their own loggers

Libraries might create their own loggers; however, this leads to two problems.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth adding a caveat: libraries should default to creating a no-op logger if the user didn't pass one in, that makes it easier to ensure that the logger is always propagated through the library.

Copy link
Member Author

Choose a reason for hiding this comment

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

I intentionally didn't add this since I think this is a topic of contention right now. I personally think this is a bad pattern since I have seen many cases where the default parameter led to folks not passing in their logger and when debugging they didn't get log statements. I personally much rather prefer having a non-optional non-defaulted parameter since it requires users attention. However, I think there is design space here by leveraging structured concurrency more to avoid all these manual passings of loggers altogether.

@FranzBusch FranzBusch force-pushed the fb-bp-structured-logging-and-accepting-handlers branch from c2de357 to 8f34b9a Compare August 18, 2025 09:42
@FranzBusch FranzBusch requested a review from glbrntt August 18, 2025 09:45
heckj
heckj reviewed Aug 18, 2025
View reviewed changes
Copy link
Contributor

@heckj heckj left a comment

Choose a reason for hiding this comment

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

minor grammar/wording tweaks and punctuation suggestions

@FranzBusch FranzBusch force-pushed the fb-bp-structured-logging-and-accepting-handlers branch from 8f34b9a to c2b83ea Compare August 18, 2025 15:49
@FranzBusch FranzBusch requested a review from heckj August 18, 2025 16:10
@FranzBusch FranzBusch force-pushed the fb-bp-structured-logging-and-accepting-handlers branch from c2b83ea to fba1c8b Compare August 18, 2025 16:10
toffaletti
toffaletti reviewed Aug 18, 2025
View reviewed changes
Sources/Logging/Docs.docc/BestPractices/001-ChoosingLogLevels.md
- **Content**: High-level operation overview, connection events, major decisions
- **Usage**: May be enabled in some production deployments.
- **Performance**: Should not significantly undermine production performance.
- **Content**: High-level operation overview, connection events, major decisions.

##### Info Level
- **Usage**: Reserved for things that went wrong but can't be communicated
Copy link

Choose a reason for hiding this comment

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

I've not seen "info" level being used for things that went wrong. I would consider warn and error levels for things that went wrong. I use info for information like version, configuration, etc. that would help to diagnose "what software am I debugging" type questions.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is guidance in particular for libraries which should avoid logging at levels higher than info to avoid spamming log systems; however, they still sometimes need to inform users that something might have went wrong. In this case, we decided that info is the right level for that.

@DominikPalo
Copy link

DominikPalo commented Aug 19, 2025 •
edited
Loading

The “For libraries” paragraph in 001-ChoosingLogLevels.md is a bit confusing right now, since it contains a sentence "Each level serves different purposes:" in the middle of the paragraph. I think this sentence should be moved to the end of that paragraph, just before explaining different loggings levels.

I'm attaching a git patch fixing the issue:
swift-log-14-29-48.patch

@ktoso
Copy link
Member

ktoso commented Aug 21, 2025

@DominikPalo

The “For libraries” paragraph in 001-ChoosingLogLevels.md is a bit confusing right now, since it contains a sentence

Please make a separate PR about this, this isn't PR about that document; let's keep the discussions and fixes separate.

ktoso
ktoso approved these changes Aug 21, 2025
View reviewed changes
Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

LGTM, please make two PRs next time when adding unrelated documents though

Sources/Logging/Docs.docc/BestPractices/002-StructuredLogging.md

### Metadata key conventions

Use hierarchical dot-notation for related fields:
Copy link
Member

Choose a reason for hiding this comment

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

Yeah dots is a good recommendation I think... if some specific backend needs something else they can do what prometheus does in metrics for labels (nameAndLabelSanitizer) 👍

Sources/Logging/Docs.docc/BestPractices/003-AcceptingLoggers.md Outdated
When libraries accept loggers as method parameters, they enable automatic
propagation of contextual metadata attached to the logger instance. This is
especially important for distributed systems where correlation IDs and tracing
information must flow through the entire request processing pipeline.
Copy link
Member

Choose a reason for hiding this comment

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

The best practices listed are OK, however it somewhat pretty weird to call out tracing as motivation and not mention the way tracing actually integrates with logging -- which isn't this pattern (regardless of opinions on status quo).

I'd just drop "distributed tracing" from the motivation entirely if the examples are not going to address it at all, causes some confusion as it's not showing off how currently tracing actually integrates with loggers today.

Copy link
Member Author

Choose a reason for hiding this comment

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

I dropped tracing and just kept the correlation IDs part.

@DominikPalo
Copy link

@ktoso

Please make a separate PR about this, this isn't PR about that document; let's keep the discussions and fixes separate.

Ok, I’ll create a separate PR for that. Honestly, I already had a draft PR prepared for my change/suggestion, but later I found out that this PR already exists, which among other things also modifies the 001-ChoosingLogLevels.md document. So it seemed more reasonable to propose this small change directly here to avoid merge conflicts.

@FranzBusch
Copy link
Member Author

@DominikPalo I included your change here since I already have some changes in that file.

@FranzBusch FranzBusch force-pushed the fb-bp-structured-logging-and-accepting-handlers branch from fba1c8b to f406145 Compare August 21, 2025 09:43
czechboy0
czechboy0 reviewed Aug 25, 2025
View reviewed changes
Sources/Logging/Docs.docc/BestPractices/001-ChoosingLogLevels.md
@@ -39,29 +38,30 @@ its logging environment.

#### For libraries

Libraries should use **info level or lower** (info, debug, trace). Each level
serves different purposes:
Libraries should use **info level or lower** (info, debug, trace).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Libraries should use **info level or lower** (info, debug, trace).
Libraries should use **info level or less severe** (info, debug, trace).

Sources/Logging/Docs.docc/BestPractices/003-AcceptingLoggers.md
@@ -0,0 +1,100 @@
# 003: Accepting loggers in libraries

Accept loggers through method parameters to ensure proper metadata propagation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you consider splitting this one into a separate PR to allow us to discuss more, without blocking the other changes? I have more thoughts here.

@mhayes853 mhayes853 mentioned this pull request Aug 26, 2025
Closed
@heckj
Copy link
Contributor

heckj commented Sep 17, 2025

@swift-ci please test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@czechboy0 czechboy0 czechboy0 left review comments

@ktoso ktoso ktoso approved these changes

@glbrntt glbrntt Awaiting requested review from glbrntt

+2 more reviewers

@toffaletti toffaletti toffaletti left review comments

@heckj heckj heckj approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
semver/none No version bump required.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@FranzBusch @DominikPalo @ktoso @heckj @toffaletti @czechboy0 @glbrntt