Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing access control level for few methods in SpanAdapter #480

Merged

Conversation

vj4iosdev
Copy link
Contributor

What's changed in this PR?

  • Changing the access control level for the below methods from internal to public in SpanAdapter
  1. toProtoSpanKind()
  2. toProtoSpanEvent()
  3. toProtoSpanLink()
  4. toStatusProto()
  • One more minor change done in OtlpTraceJsonExporter i,e changing the access control for the method getExportedSpans() to public from internal. I had contributed to this earlier but we were using this file locally. Now we are upgrading to use this directly from this repo.

Why is this change required?

  • We are leveraging OpenTelemetry extensively for both mobile and web in our app (nested as well).
    In some of the flows, the WebView generates the Spans and hands over the SpanData to mobile. In this case, the native side, instead of creating new Spans/ids, needs to override the SpanData with the received SpanData (containing already generated span ids.). So we need the above-mentioned methods to be public to do so. Hence opening the PR with the changes required.

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

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

📢 Thoughts on this report? Let us know!.

@vj4iosdev
Copy link
Contributor Author

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

@nachoBonafonte
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

vj4iosdev commented Oct 27, 2023

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.

@nachoBonafonte nachoBonafonte merged commit 7bef81a into open-telemetry:main Oct 27, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants