[Proposal] SLG-0004: metadata value attributes (revision 2)

Server Development
1

Hi all,

The revised proposal SLG-0004: Metadata values privacy attribute for swift-log is now up and In Review. The first revision was discussed here.

The review period will run until May 12th — please feel free to post your feedback as a reply to this thread.

Thanks!

3 Likes
2

I'm -0.5 on this proposal. While I understand that this solution solves the problem, I'm generally not a fan of implicit value-smuggling that is otherwise impossible to discover by adopters. Having to check at runtime for concrete magical values is more of the domain of dynamic languages that don't have the benefit of strong typing. Here I don't think we need to resort to hiding the attributes this way.

The fact that LogHandler/LogEvent didn't have to change is what rubs me the wrong way. This is a new feature, and we expect log handlers to adapt to it. So something about that protocol should change, I think.

IMO the previous explicit API was easier to discover and reason about, even if it was more work to implement and had a bigger API impact - I think that clarity was a feature, not a bug.

Just my 2c :slightly_smiling_face:

1 Like
3

+1, thanks for all the work on this.

I think putting this in the .stringConvertible(...) case is acceptable here because the value actually is string convertible. It's a string and extra information that supporting log handlers can extract: exactly what we need here.

Sure, it would be even better if we had a .attributed(...) case but the fact is that we don't and the enum is non-extensible (that was all the language supported at the time). The .stringConvertible(...) case was added specifically to allow retro-fitting functionality that we hadn't thought of in 2018/2019. In my mind, this fits perfectly: Log handlers that support certain attributes can do so, others will be blind to the attributes but continue to log the strings correctly.

This new proposal is so much better than the old ones:

  • +1 on separation of policy (what attributes, how to handle) & mechanism (the possibility to add attributes)
  • +1 on not bifurcating the logging API into log events with and log events without attributes. Bifurcated APIs are really not fun, especially when it comes to protocol conformances
1 Like
4

+1 from me

This is a new feature, and we expect log handlers to adapt to it.

@Honza_Dvorsky I think with the proposal's current shape we actually don't necessarily expect log handlers to adapt. If I'm understanding correctly, attributes are defined outside of swift-log so if this proposal were to be merged in its current state, handlers wouldn't have anything to adapt to. Sure they could check if the metadata is attributed but without the MetadataAttributeKey-conforming type, I'm struggling to think of what handlers would do with that information. I guess they could dump it as part of the log? That seems like a Handler-specific decision though.

I guess TL;DR I see this implementation as giving handlers the opportunity to do more rather than requiring that they do more which feels right to me.

1 Like
5

I'm not sure I understand the scope of the proposal.

In "Proposed solution":

Two common attributes — sensitivity and value type hints — will be defined in separate packages so that the core Logging module remains free of domain-specific attributes while providing shared vocabulary across libraries.

In "Example attributes":

The following examples illustrate how ecosystem packages could define attributes using the MetadataAttributeKey protocol. These are not part of this proposal.

My take from reading the proposal is that no attributes are added at this point - but then I think the wording in the proposed solution is misleading. Personally, though, I would add them in the same proposal. Without any "common" attributes, I think the addition will only slight adoption, since it would require both the logging code and the LogHandler to depend on the same 3rd party package, or for the LogHandler itself to define attributes. Also, if common attributes such as public/private and type-hints are not added from the start or early on, we risk people adopting 3rd party packages for this, which could fragment the user base if they were added later to swift-log.

Apart from that, I like the change to put the attributes on the leaf nodes rather than at top level keys, as was the case in the previous iteration. Though I'm not sure about the following:

Recursive attribute inspection. The .attributes property only inspects top-level values. Metadata values nested inside .dictionary or .array cases carry their own attributes on leaf values, but handler-level iteration (for example, redaction) operates on top-level entries only. A future proposal could introduce recursive attribute traversal utilities for handlers that need deep inspection.

Why is this? Can't the LogHandler already do this by recursing on any .dictionary and .array elements? Pseudo code:

for (key, _) in eventMetadata {
    var value = eventMetadata[key]!
    switch value {
        case .string(let val):
            let attrs = val.attributes
            attrs[ProcessingStage.self] = .enriched
            eventMetadata[key]?.attributes = attrs
        case .stringConvertible(let val):
            let attrs = val.attributes
            attrs[ProcessingStage.self] = .enriched
            eventMetadata[key]?.attributes = attrs
        case .dictionary(let val): // recurse
        case .array(let val): // recurse
    }
}
1 Like
6

@samuelmurray

My take from reading the proposal is that no attributes are added at this point

Yes, this is correct. I agree that paragraph implies we define those separate packages at the same time, which is not true, the point was we do not add attributes to swift-log at this stage. I'll rephrase it to be clear.

I would add them in the same proposal

The main reason we do not add them at this point is deciding what is common and what is not is subjective, we open the door for all kinds of attributes to be added to the API package. The fragmentation risk is real, though. Having a list of common attribute packages in swift-log docs is one strategy to improve discoverability and mitigate fragmentation. Another is discouraging LogHandlers defining their own attributes. A very real scenario we would like to avoid is a Darwin-specific OSLogHandler defining the privacy attribute and a cross-platform library adopting this attribute, making Linux-targeted apps using that library to require Darwin-specific OSLogHandler.

Why is this? Can't the LogHandler already do this by recursing on any .dictionary and .array elements?

You are right, I will remove this paragraph from the future directions, it is not helpful.

1 Like
7

Thanks for the clarification!

8

I read through the latest proposal here again and all the alternatives you considered. I think this is a good approach that is incremental on the current APIs and still would allow us to change how we pass attributes in the future. So overall I am a +1 on this proposal.

The only thing that stood out to me was this:

public protocol MetadataAttributeKey: Sendable, RawRepresentable where RawValue == Int64 {}

It feels odd calling this type a "Key". Furthermore, I think the type should be nested inside MetadataValueAttributes. What if we do this instead?

public struct MetadataValueAttributes: Sendable, Equatable, ExpressibleByArrayLiteral {
    public protocol Attribute: Sendable, RawRepresentable where RawValue == Int64 {}

    // ...
}
1 Like
9

@FranzBusch I think this is a left over from the point of experiments where attributes were a dict. I like the proposed nested protocols. pushed the change.

@samuelmurray and also updated the phrasing we discussed.

2 Likes
10

Hi all,

The review period for [SLG-0004]: metadata value attributes is now over. As there were no major objections raised, I'm marking the proposal as Ready for Implementation. :tada:

Thank you!

5 Likes