OtlpExporter peer.service resolution #1392
Conversation
…peer.service tag in OtlpExporter.
…few things around to resolve types ending up in multiple assemblies.
Codecov Report
@@ Coverage Diff @@
## master #1392 +/- ##
==========================================
- Coverage 81.30% 81.28% -0.03%
==========================================
Files 226 227 +1
Lines 6088 6097 +9
==========================================
+ Hits 4950 4956 +6
- Misses 1138 1141 +3
|
| @@ -16,6 +16,7 @@ | |||
|
|
|||
| using System.Runtime.CompilerServices; | |||
|
|
|||
| [assembly: InternalsVisibleTo("OpenTelemetry" + AssemblyInfo.PublicKey)] | |||
There was a problem hiding this comment.
which particular class/api is triggering this change? can we keep using shared file approach instead?
There was a problem hiding this comment.
The PeerServiceResolver I refactored into SDK needs SemanticConventions, which is in API. So I included that file in SDK. Problem is, OpenTelemetry.Tests won't build with that. Because OpenTelemetry.Tests can see the internals of API & SDK, it was getting an error like "SemanticConventions type exists in both API & SDK" 🤯 I couldn't figure out a more elegant way to fix it. Open to ideas!
There was a problem hiding this comment.
FYI - I've seen this workaround. Not sure if I would call it "elegant" though 🧙♀️
There was a problem hiding this comment.
Okay got it. I am okay to keep it like this.
There was a problem hiding this comment.
@reyang I think I actually removed that when I made this change 🤣 But I could put that back and just make sure API & SDK SemanticConventions each get a different namespace, that would fix it. Not sure which approach is better. Why I went with this one: This is letting SDK see everything in API. SDK is kind of a superset of API so it really should see everything? 🤷
PS: One other change that is easy to miss on the diff: In the projects I touched, I move all the includes to /Includes/ folder. It was a little inconsistent before, some in root, some in /Internal/, some in /Implementation/.
There was a problem hiding this comment.
@CodeBlanch sorry I didn't walk through this PR.
I would vote for making the API internals visible to the SDK since the namespace hack is hard to maintain/debug/test.
There was a problem hiding this comment.
One thing we might need to consider is the API/SDK version compat.
E.g. do we expect version 1.1 of the SDK to work with 1.0 API?
Relates to #1294
Changes
Refactored the
peer.serviceresolution logic from Jaeger & Zipkin into a shared location so OtlpExporter could also use it. The goal is to achieve feature parity amongst the supported exporters.CHANGELOG.mdupdated for non-trivial changes