[SLG-0004]: Revised Metadata value attributes + sensitivity attribute implementation#439
Conversation
f965af9 to
7a1a084
Compare
weissi
left a comment
There was a problem hiding this comment.
The general idea and the implementation suggestion of using a fixed-sized inline slot is good.
But overall -1 because of these concerns:
- Major concern: Complete bifurcation of SwiftLog's core APIs. There's a new
LogMessagecalledLogEvent. There's new, duplicated metadata. This is essentially shipping a whole new version disguised as a feature addition. - Major concern: Bundles with sensitivity, privileges sensitivity & introduces unduly policy decisions regarding sensitivity into the core mechanism package. This one's easy to fix, remove anything relating to sensitivity from this proposal and create a new package
swift-log-sensitivity-attributesthat has no special privileges - Minor: Reveals too much about how the storage is implemented without reason
|
|
||
| This design separates the transport mechanism (attributed metadata) from specific attribute semantics (privacy labels), allowing future extensions without breaking existing attributed metadata code. | ||
| The attribute mechanism is extensible: handler packages and middleware can define their own attribute types using the | ||
| same `MetadataAttributeKey` protocol. A `LogHandler` middleware can read the attributes it cares about, act on them, |
There was a problem hiding this comment.
SwiftLog doesn't have a middleware logger concept. You could implement one but I think that's besides the point.
I reckon this is meant to be an example but I think it's very confusing. Anything relating to middlewares is squarely a "middle end" concern and not a concern of the the deliberately as-policy-free-as-possible SwiftLog. I would advise to remove this, there are no middlewares, let's not invent them in an unrelated proposal.
There was a problem hiding this comment.
Agree, middleware is a bad term here. Rephrased.
| public init(_ value: MetadataValue, privacy: PrivacyLevel) | ||
| } | ||
| public protocol MetadataAttributeKey: Sendable, | ||
| RawRepresentable where RawValue == Int {} |
There was a problem hiding this comment.
This is one of the very few instances where I would think that a fixed-width integer type (e.g. Int64) would maybe make sense? What if somebody adds case special = 9223372036854775807 (64 1 bits), then this would only work in 64 bit environments.
This is a fairly minor concern but I do think this is meant to potentially be used with bit masks, so this could be a concern I think.
There was a problem hiding this comment.
Let's use Int64.
| /// | ||
| /// ## Creating Attributed Metadata | ||
| /// The conforming type must be an enum with `Int` raw values. Each attribute occupies just | ||
| /// `(ObjectIdentifier, Int)` in the inline slot, avoiding heap allocation for the |
There was a problem hiding this comment.
This unnecessarily leaks the guts. I don't think we should reveal the implementation here. The int-bit is fine but the ObjectIndentifier should be opaque IMHO
| /// The privacy attributes associated with this metadata value. | ||
| public var attributes: MetadataValueAttributes | ||
| Each conforming type serves as both the key and the value for an attribute. The metatype (`Key.Type`) identifies | ||
| the attribute at runtime via `ObjectIdentifier` — no coordination between packages is needed. |
There was a problem hiding this comment.
no need to leak how we're storing the types I believe. The only thing the user knows is that it's identified by Key.Type -- and crucially not how we're storing it.
| ### Future directions | ||
|
|
||
| 2. **Convenience methods (`.private()` and `.public()`):** Add extension methods to `String` and `MetadataValue` for creating attributed metadata: | ||
| - **Metrics extraction middleware.** A `LogHandler` middleware that reads a metric attribute and dual-writes to |
There was a problem hiding this comment.
Middleware doesn't exist in SwiftLog, I think this should be removed
| forwards the attributed metadata to the next handler. | ||
| - **Cardinality hints.** An attribute indicating whether a field is safe to index as a label/dimension in observability | ||
| backends. Only the call site knows a field's cardinality ahead of time. | ||
| - **Typed metadata values.** Adding typed variants to `MetadataValue` (`.int64`, `.double`, `.bool`) would reduce the |
There was a problem hiding this comment.
This isn't a future direction for SwiftLog, it's an idea for a new package that works with SwiftLog, maybe swift-log-type-annotations
| backends. Only the call site knows a field's cardinality ahead of time. | ||
| - **Typed metadata values.** Adding typed variants to `MetadataValue` (`.int64`, `.double`, `.bool`) would reduce the | ||
| need for attributes that compensate for stringly-typed metadata. | ||
| - **More stored slots.** If real-world usage shows growth in the number of attributes per value, it would make sense |
There was a problem hiding this comment.
I would see this as an adjustment. Let's just not document how many are guaranteed to be allocation free. You can leave a comment saying that and how we will monitor what seems like a good amount of inline storage.
| ]) | ||
| ``` | ||
| Pack attribute values into an inline `UInt64` using declared bit offsets. Smaller struct and O(1) access, but requires | ||
| authors to coordinate bit layout and risks collisions between independent packages. |
There was a problem hiding this comment.
FWIW, the collisions could be avoided if SwiftLog had API to provide "your bit" to you. But yes, it's more difficult to design and might not carry its weight.
I'm happy with the "N (1 to start with) attributes are free because we store them inline" proposal
| } | ||
| ``` | ||
|
|
||
| #### Adopting attributed metadata in LogHandlers |
There was a problem hiding this comment.
I think the adoption should not be this difficult. What we're proposing in disguise in this proposal is to essentially create a whole second API for SwiftLog. I do not think that this is a good direction.
7a1a084 to
44cf766
Compare
44cf766 to
c080098
Compare
c080098 to
b689899
Compare
Important
This is a revision of SLG-0004 previously discussed here. Note this is a draft, including more than just the proposal implementation.
Add an extensible per-value attribute mechanism for metadata, with sensitivity as the first attribute.
Motivation:
Metadata values in
swift-logare opaque strings by the time theLogHandlerreceives them. The call site oftenknows things about a value that the handler cannot infer — for example, whether the value should be redacted in
different environments.
Modifications:
AttributedMetadataValue-based mechanism toLogging.Sensitivityattribute to the newLoggingSensitivitytarget.OSLogHandleras an example of a concrete log handler taking advantage of theSensitivityattribute.Result:
swift-logprovides an optional extensible mechanism for attaching attributes to metadata values.swift-logprovides a commonSensitivityattribute and examples how to use it.