open-telemetry / opentelemetry-swift

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

Changing access control level for few methods in SpanAdapter #480

Closed vj4iosdev closed 10 months ago

vj4iosdev commented 10 months ago

What's changed in this PR?

Why is this change required?

codecov[bot] commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Files Coverage Δ
...penTelemetryProtocolCommon/trace/SpanAdapter.swift 99.00% <100.00%> (ø)
...etryProtocolGrpc/trace/OtlpTraceJsonExporter.swift 88.57% <100.00%> (ø)

... and 2 files with indirect coverage changes

:loudspeaker: Thoughts on this report? Let us know!.

vj4iosdev commented 10 months ago

@nachoBonafonte @bryce-b @vvydier Please take a look whenever you get some time.

nachoBonafonte commented 10 months ago

It looks good to me, I completely agree with the getExportedSpans() being public, but I don't understand why you need the SpanAdapter utility functions to be public instead of modifying the SpanData and calling the default toProtoSpan()

vj4iosdev commented 10 months ago

It looks good to me, I completely agree with the getExportedSpans() being public, but I don't understand why you need the SpanAdapter utility functions to be public instead of modifying the SpanData and calling the default toProtoSpan()

Ok. So here is our use case. When we receive spanData from web FCI to mobile FCI, we are required not to generate the spanID, traceID, parentSpanID and span attributes but override these as received from the web. We need to override toProtoSpan(spanData: SpanData) method in SpanAdapter in order to override the spanID, traceID, parentSpanID and span attributes but the SpanAdapter being struct we cannot mark the method toProtoSpan(spanData: SpanData) as open to enable it for the override.

So here is our code:

static func toProtoSpan(spanData: SpanData) -> Opentelemetry_Proto_Trace_V1_Span {

        if OverrideProtoSpan.containsOverridenAttributes(spanData: spanData) {
            // we override spanData here
            return OverrideProtoSpan.toProtoSpan(spanData: spanData)
        }

        // then the rest of the code as is..
}

And in the OverrideProtoSpan.toProtoSpan(spanData: spanData) we use the utility functions of SpanAdapter to override the traceID, spanID, parentSpanID and span attributes received from the web FCI. Hope this clarifies.

nachoBonafonte commented 10 months ago

Thanks for the clarification it makes total sense now. If you think that making span adapter a class with the open method works better for you, fellow free to change it.

vj4iosdev commented 10 months ago

Thanks for the clarification it makes total sense now. If you think that making span adapter a class with the open method works better for you, fellow free to change it.

Thanks for the approval @nachoBonafonte . Static declarations are implicitly 'final' so may have to make changes in many places if I do that. So I will have this merged as of now and open a new one later. Please merge this PR whenever you happen to see this.