Add new best practices for structured logging and accepting loggers

[FranzBusch]

No description provided.

[glbrntt]

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.

[FranzBusch]

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.

[heckj]

minor grammar/wording tweaks and punctuation suggestions

[DominikPalo]

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]

@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]

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

[ktoso]

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) 👍

[kukushechkin]

Are those considered equivalent?

logger.debug(
    "Database operation completed",
    metadata: [
        "db.operation": "SELECT",
        "db.table": "users",
        "db.duration": "\(duration)",
        "db.rows": "\(rowCount)"
    ]
)

and

logger.debug(
    "Database operation completed",
    metadata: [
        "db": [
            "operation": "SELECT",
            "table": "users",
            "duration": "\(duration)",
            "rows": "\(rowCount)"
        ]
    ]
)

[ktoso]

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.

[FranzBusch]

I dropped tracing and just kept the correlation IDs part.

[DominikPalo]

@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]

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

[czechboy0]

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.

[heckj]

@swift-ci please test

[ktoso]

still lgtm :-)