Inconsistent metadata handling in `MultiplexLogHandler`
- 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
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