Enable process level tags to be associated with the tracer #143
Conversation
Codecov Report
@@ Coverage Diff @@
## master #143 +/- ##
============================================
+ Coverage 36.18% 36.24% +0.06%
Complexity 598 598
============================================
Files 94 94
Lines 6343 6349 +6
Branches 1052 1052
============================================
+ Hits 2295 2301 +6
Misses 3866 3866
Partials 182 182
Continue to review full report at Codecov.
|
| @@ -89,7 +90,6 @@ private Tracer( | |||
|
|
|||
| this.version = loadVersion(); | |||
|
|
|||
| Map<String, Object> tags = new HashMap<String, Object>(); | |||
There was a problem hiding this comment.
I think it is better to create a new map and add all tags from the parameter tags, rather than taking the ownership of the map created outside of the constructor.
There was a problem hiding this comment.
actually, never mind, the map is owned by the builder, so it's fine
There was a problem hiding this comment.
@pavolloffay Note that this will have to change because process-level tags are not sent as regular tags on every Span, but only in the Process object in the reported batch.
|
@yurishkuro thanks, I will handle it. How are we going to handle this in zipkin? Add to each/first in the process span? |
|
Yes, in Zipkin we used to add them to first-in-process span only, to minimize the span size. It was a bit confusing to people. though. |
| @@ -370,8 +371,23 @@ Builder withMetrics(Metrics metrics) { | |||
| return this; | |||
| } | |||
|
|
|||
| Builder withTag(String key, String value) { | |||
There was a problem hiding this comment.
This should be public. I will change it in #142 in order to test it when corverting to thrift model.
Inline with the new jaeger model, where tags can be associated with the process, this change enables some process level tags to be specified when building the
Tracer.