Skip to content

SLG-0003: Error parameter implementation#425

Merged
FranzBusch merged 15 commits into
apple:mainfrom
samuelmurray:SLG-0003-error-parameter-implementation
Apr 14, 2026
Merged

SLG-0003: Error parameter implementation#425
FranzBusch merged 15 commits into
apple:mainfrom
samuelmurray:SLG-0003-error-parameter-implementation

Conversation

@samuelmurray
Copy link
Copy Markdown
Contributor

@samuelmurray samuelmurray commented Mar 10, 2026

Add new methods on Logger that allows the caller to attach Errors to log events, which will be propagated to the LogHandler as-is.

Motivation:

Error information is typically of high interest in logs and log analysis. Currently, however, there is no standardized way of attaching Error information to logs, forcing users to either append it to the log message (logger.warning("Something went wrong: \(error)")), or attach it as metadata. By providing a separate parameter for this, and passing it as-is to the LogHandler, we both provide a standard, visible way to attach it, while also letting the LogHandler format it as it sees fit.
Closes #291

Modifications:

  • New methods on Logger that accept an (optional) Error.
  • An error property is added to LogEvent, which contains the Error from the new Logger methods.
  • Adjust LogHandler implementations to serialize error as metadata.

Result:

Users of Logger can use the new methods to attach Errors to their logs, which should help unify how Errors get logged in various code bases.
LogHandler implementations can use the new error property on LogEvent and serialize it in a way that's suitable for that implementation. In general, extracting the string representation and the (qualified) type name should be a good baseline.

This was referenced Mar 10, 2026
@samuelmurray samuelmurray force-pushed the SLG-0003-error-parameter-implementation branch 3 times, most recently from 1621efc to 4bf7b23 Compare April 8, 2026 16:16
Comment thread Sources/InMemoryLogging/InMemoryLogHandler.swift Outdated
@samuelmurray samuelmurray marked this pull request as ready for review April 8, 2026 16:19
@samuelmurray samuelmurray force-pushed the SLG-0003-error-parameter-implementation branch 2 times, most recently from 97bf262 to e7fd54c Compare April 8, 2026 16:22
Comment thread Tests/LoggingTests/TestLogger.swift Outdated
@samuelmurray
Copy link
Copy Markdown
Contributor Author

Seeing as the change to LogEvent was released in 1.11.0, I opted to not provide a default implementation where the error is serialized as metadata. As I mentioned in the review for that feature, I'm not sure how we could determine if a LogHandler supports this new attribute or not. So, the best we could do is to provide a fallback for those that still do not use LogEvent - but it would feel weird to provide a fallback for old versions (<=1.10.X) but not the most recent one (1.11.0).
As far as I understood the discourse in both these proposal reviews however, most people did not see this as a huge issue. But if you see some way to provide a fallback that works for 1.11.0, I'd be happy to add it.

@samuelmurray samuelmurray force-pushed the SLG-0003-error-parameter-implementation branch from e7fd54c to 79f5bc1 Compare April 8, 2026 17:02
@kukushechkin kukushechkin added the 🆕 semver/minor Adds new public API. label Apr 10, 2026
Copy link
Copy Markdown
Contributor

@kukushechkin kukushechkin left a comment

Choose a reason for hiding this comment

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

Thank you for the implementation, looks great overall! Let's iron out magic metadata entries vs explict error.

Comment thread Sources/InMemoryLogging/InMemoryLogHandler.swift Outdated
Comment thread Sources/Logging/Logging.swift Outdated
Comment thread Sources/Logging/Logging.swift
Comment thread Tests/LoggingTests/LoggingTest.swift Outdated
Comment thread Tests/LoggingTests/TestLogger.swift Outdated
Comment thread Tests/InMemoryLoggingTests/InMemoryLogHandlerTests.swift Outdated
Comment thread Tests/LoggingTests/TestLogger.swift Outdated
@kukushechkin
Copy link
Copy Markdown
Contributor

As for the failing CI checks:

  • The API breakage check failing on a package method is annoying, but ok.
  • Benchmarks producing slightly different values is probably due to the infra changes, let's observe on the subsequent runs.
  • Tests — in the proposal we have String(reflecting:) instead of String(describing:), is there a reason to use describing:?
  • And the formatting, but that one is self-explanatory.

@samuelmurray
Copy link
Copy Markdown
Contributor Author

As for the failing CI checks:

  • The API breakage check failing on a package method is annoying, but ok.
  • Benchmarks producing slightly different values is probably due to the infra changes, let's observe on the subsequent runs.
  • Tests — in the proposal we have String(reflecting:) instead of String(describing:), is there a reason to use describing:?
  • And the formatting, but that one is self-explanatory.

Right, the reflecting vs describing was a mistake on my part, changed the tests and InMemoryLogHandler to use reflecting and added a test verifies that that we compare fully qualified names in the equality check.

@samuelmurray
Copy link
Copy Markdown
Contributor Author

Unrelated to this PR:
Some files in this repo are getting quite long, especially Logging.swift. How about splitting it up some? For example, we could extract the included LogHandler implementations to separate files (similar to providers in swift-configuration), and maybe separate all the log methods to Logging+methods.swift.

Comment thread Sources/InMemoryLogging/InMemoryLogHandler.swift Outdated
@kukushechkin
Copy link
Copy Markdown
Contributor

I've pushed an update to the benchmark thresholds, this should make it pass.

@kukushechkin kukushechkin self-requested a review April 13, 2026 11:28
@kukushechkin kukushechkin requested a review from FranzBusch April 13, 2026 14:56
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should probably get rid of this TestLogger and replace it with the InMemoryLogHandler at some point in the future

@kukushechkin kukushechkin enabled auto-merge (squash) April 14, 2026 09:04
@FranzBusch FranzBusch disabled auto-merge April 14, 2026 13:03
@FranzBusch FranzBusch merged commit 5073617 into apple:main Apr 14, 2026
56 of 57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🆕 semver/minor Adds new public API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adding Error to log functions

3 participants