open-telemetry / opentelemetry-swift

OpenTelemetry API for Swift
https://opentelemetry.io/docs/instrumentation/swift/
Apache License 2.0
207 stars 124 forks source link

Update attribute processor for stable metrics #524

Closed yangjian closed 5 months ago

yangjian commented 5 months ago

Previously AttribtueProcessor is a class and not open. It can't be customized externally. Now it's changed to procotol and its static functions are moved into SimpleAttributeProcessor (now it's public).

Also duplicated NoopAttributesProcessor is removed and more tests are added.

linux-foundation-easycla[bot] commented 5 months ago

CLA Signed

The committers listed above are authorized under a signed CLA.

nachoBonafonte commented 5 months ago

I would like to understand the use case for this change, whyy the need to inherit AttributeProcessor instead of extending it

yangjian commented 5 months ago

There're common labels for all metrics in our iOS app. Attribute processor can be used to merge them into final labels. But one of these labels can be changed by user or server. Customized attribute processor can be more flexible to support it. Something like this:

class GlobalSharedLabels: AttributeProcessor {
    let attributes: ManagedAtomic<AtomicContainer<[String: AttributeValue]>>

    public updateLabels(...) {
        attributes.store(...)
    }

    public func process(incoming: [String: AttributeValue]) -> [String: AttributeValue] {
        incoming.merging(attributes.load(...), uniquingKeysWith: { $1 })
    }
}

Alternatively, it can return a SimpleAttributeProcessor, but it isn't exported before.

codecov[bot] commented 5 months ago

Codecov Report

Attention: Patch coverage is 69.23077% with 8 lines in your changes are missing coverage. Please review.

:exclamation: No coverage uploaded for pull request base (main@4d1502b). Click here to learn what that means.

Files Patch % Lines
...TelemetrySdk/Metrics/Stable/View/ViewBuilder.swift 0.00% 5 Missing :warning:
...rySdk/Metrics/Stable/View/AttributeProcessor.swift 85.71% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #524 +/- ## ======================================= Coverage ? 67.45% ======================================= Files ? 335 Lines ? 14585 Branches ? 0 ======================================= Hits ? 9839 Misses ? 4746 Partials ? 0 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.