Change kafka to use Statement instead of Othernamespace #1306
Conversation
grcevski
requested review from
mariomac,
marctc and
rafaelroquetto
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1306 +/- ##
==========================================
+ Coverage 81.25% 81.37% +0.12%
==========================================
Files 142 142
Lines 14294 14295 +1
==========================================
+ Hits 11614 11633 +19
+ Misses 2116 2102 -14
+ Partials 564 560 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@esara I made a follow-up with the fix for Kafka and some refactoring so the names are the same across traces and metrics. It would be great to get your review. |
| func HostAsServer(span *Span) string { | ||
| if span.OtherNamespace != "" && span.OtherNamespace != span.ServiceID.Namespace { | ||
| if span.IsClientSpan() { | ||
| return SpanHost(span) + "." + span.OtherNamespace |
There was a problem hiding this comment.
one minor comment, if the span.Hostname == "" this would return span.Host+span.OtherNamespace - where the span.Host is an ip address
although you can assume if that the span.OtherNamespace k8s lookup worked, the span.Hostname should also be populated, in which case the span.Host would not be used
There was a problem hiding this comment.
Ah true, we probably don't want to do IP.namespace. I'll add a check for span.HostName existing. Thanks for catching this!
|
|
||
| func PeerAsClient(span *Span) string { | ||
| if span.OtherNamespace != "" && span.OtherNamespace != span.ServiceID.Namespace { | ||
| if !span.IsClientSpan() { |
We needed an extra field for the Kafka clientID in our span struct and we ended up using Othernamespace, which is wrong. That field is re-written by the nameresolver. We can use statement instead.
This is a follow-up of #1247. I refactored the code a bit so I can reuse the logic in the metrics and I added some unit tests.