apple / swift-distributed-tracing

Instrumentation library for Swift server applications
https://swiftpackageindex.com/apple/swift-distributed-tracing/main/documentation/tracing
Apache License 2.0
224 stars 34 forks source link

SpanAttributes subscript should use concrete type and not an existential #82

Closed fabianfett closed 1 year ago

fabianfett commented 1 year ago

Currently SpanAttributes uses an existential for its subscript API.

public struct SpanAttributes {
  public subscript(_ name: String) -> SpanAttributeConvertible? { get set }
}

Swift Logging uses the concrete type Logger.Metadata.Value instead:

public struct Logger {
    subscript(metadataKey metadataKey: String) -> Logger.MetadataValue? { get set }

    public enum MetadataValue {
        case string(String)
        case stringConvertible(CustomStringConvertible & Sendable)
        case dictionary(Metadata)
        case array([Metadata.Value])
    }
}

I think that we should change the SpanAttributes subscript API to return a SpanAttribute. This way this would work like Logging and we remove an existential from the API boundary

ktoso commented 1 year ago

Thanks for catching this -- here I think indeed we'll be able to do that 👀 Let's give it a shot.

ktoso commented 1 year ago

I'm not sure about this, it means we'd lose the ability to:

        span.attributes["http.status"] = response.status

which is very nice 🤔 I could not find a way around it.

ktoso commented 1 year ago

We can add additional API though, I'll do that