apple / apple/swift-log

Inconsistent metadata handling in `MultiplexLogHandler`

Open
#498 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Swift
Stars
4k
Forks
344
Avg merge
1d 11h
Merged PRs (30d)
5

Description

The current `MultiplexLogHandler` implementation has some surprising behaviour when it comes to metadata.

### Priority
When constructing a `MultiplexLogHandler`, one can pass multiple handlers, `[first, ..., last]`, which may or may not have metadata attached to them. When accessing metadata of the `MultiplexLogHandler`, this is delegated to the underlying handlers. However, the priority is inconsistent.

1. When accessing via `subscript(metadataKey:)`, the value is taken from the _first_ handler for which the key is present.
2. When accessing via `metadata`, the value is taken from the _last_ handler for which the key is present.

The docs state that the _first_ handler should always have highest priority. This was noted in https://github.com/apple/swift-log/pull/488.

### Metadata Provider

When accessing the `metadata` property of a `MultiplexLogHandler`, a metadata object is constructed by merging the metadata from the underlying handlers. However, not only are their metadata used, but also their metadata providers.

This can lead to surprising behaviour such as

```swift
multiplexHandler.metadata = [:]
assert(multiplexHandler.metadata.isEmpty, "Would expect this to be true, but will not be if `metadataProvider` has content")
```

### Discussion

The current implementation is surprising (or arguably just "wrong"), but changing it might be considered breaking. What alternatives do we have?

1. **Do nothing**
Update documentation to reflect current behaviour, clearly point out the surprising behaviour. This is in essence what https://github.com/apple/swift-log/pull/488 suggests.

2. **Change current behaviour**
Update the `MultiplexLogHandler` implementation to 1) have consistent ordering, and 2) not use metadata provider in metadata property access. This might be considered breaking, even though one could argue that the current behaviour is a bug.

3. **Replace current handler with new**
Create a replacement for `MultiplexLogHandler`, and deprecate the current one. Probably safer than 2., as it is opt-in to update to the new behaviour. This would require a new name, and update all documentation to point to the new one.

Contributor guide

Open the contributing guide

Research direction

Start by reading the MultiplexLogHandler implementation, especially metadata, subscript(metadataKey:), and metadataProvider handling, along with the discussion in pull request 488. Compare the documented first-handler priority with the current behavior and verify the metadata-provider example. Done requires an agreed direction among the three alternatives before implementation can be scoped.

Written by the indexing model from the issue text.

Assessment

Tech stack
swift
Domain
api
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
28/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.