-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Don't modify process attributes in jaeger translator #884
Don't modify process attributes in jaeger translator #884
Conversation
Codecov Report
@@ Coverage Diff @@
## master #884 +/- ##
==========================================
+ Coverage 85.97% 86.00% +0.03%
==========================================
Files 162 162
Lines 12948 12949 +1
==========================================
+ Hits 11132 11137 +5
+ Misses 1395 1393 -2
+ Partials 421 419 -2
Continue to review full report at Codecov.
|
Good catch. The current behavior is a bug and does not match our data ownership guidelines: https://github.com/open-telemetry/opentelemetry-collector/tree/master/exporter#data-ownership @dmitryax FYI. |
@@ -131,6 +130,10 @@ func attributesToJaegerProtoTags(attrs pdata.AttributeMap) []model.KeyValue { | |||
|
|||
tags := make([]model.KeyValue, 0, attrs.Cap()) | |||
attrs.ForEach(func(key string, attr pdata.AttributeValue) { | |||
if key == conventions.AttributeServiceName { | |||
return | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for fixing it!
The only concern I have is that now we scatter handling of conventions.AttributeServiceName across many functions, it doesn't seem right especially having attributesToJaegerProtoTags
function being called in other places. Probably we could add another argument to attributesToJaegerProtoTags
like skipKeys map[string]bool
that can be used to filter particular keys out. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
attributesToJaegerProtoTags
is used in several places, so it makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not like this. Map lookup is going to have cost. Translation functions need to be fast, and this one loop is the most frequently called one. I would like to see performance profile before accepting this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think map lookups should be pretty fast. map initialization is the most expensive, but here it's handled perfectly, only two initializations. Otherwise we can just duplicate the function and create resourceAttributesToJaegerProtoTags
for now in order to not block the fix. @tigrannajaryan what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want a quick fix let's pass a single bool
parameter that controls AttributeServiceName
specifically.
If we want the map I can be easily convinced by a CPU profile that shows it is not significant. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance impact is unclear.
This bug was fixed in #907 |
Description: currently
InternalTracesToJaegerProto
deletesservice.name
attribute during conversion to jaeger proto. It causes problems if you want to export trace data via several exporters.