Support jaeger-baggage header for ad-hoc baggage
#525
Conversation
ghost
assigned 
yurishkuro
force-pushed
the
support-jaeger-baggage-header2
branch
from
8debfb6 to
5c0ec61
Compare
| return context.withBaggage(baggage); | ||
| } | ||
|
|
||
| private Map<String, String> parseBaggageHeader(String header, Map<String, String> baggage) { | ||
| String[] parts = header.split("\\ *,\\ *"); |
There was a problem hiding this comment.
Don't we have a similar logic for the JAEGER_TAGS?
There was a problem hiding this comment.
It's similar, but that one has additional variable resolution logic for values.
| if (baggage == null) { | ||
| baggage = new HashMap<String, String>(); | ||
| } | ||
| baggage.put(kv[0], kv[1]); |
There was a problem hiding this comment.
Can we place some constraints here, like limiting the number of entries and their size? I can see this being used for DDoS without such constraints, as an attacker needs to send the headers once and they are propagated throughout the whole infra, potentially multiple times.
There was a problem hiding this comment.
What would you suggest? A limit on the total length of this header?
Alternatively we can just integrate it with baggage restrictions manager.
There was a problem hiding this comment.
How about making this configurable? JAEGER_MAX_NUM_BAGGAGE_ITEMS, JAEGER_MAX_SIZE_BAGGAGE_ITEMS, with a default of 10 baggage items and 256 bytes per key and value (512 bytes per pair)?
There was a problem hiding this comment.
Env vars feel out of place for this. The baggage size policy should be controlled centrally in organization, and we already have a mechanism for that via baggage restrictions manager.
There was a problem hiding this comment.
Do we have it documented somewhere? I never used it myself, wouldn't know how to even start using that.
There was a problem hiding this comment.
I am going to punt on restrictions, because this change is not adding any new security risks, as the bad actor can just as well send the normal tracing/baggage headers in the request.
yurishkuro
force-pushed
the
support-jaeger-baggage-header2
branch
from
7b98f2f to
779acd8
Compare
Signed-off-by: Yuri Shkuro <ys@uber.com>
Codecov Report
@@ Coverage Diff @@
## master #525 +/- ##
============================================
+ Coverage 88.36% 88.78% +0.41%
- Complexity 505 516 +11
============================================
Files 66 66
Lines 1883 1890 +7
Branches 239 244 +5
============================================
+ Hits 1664 1678 +14
+ Misses 143 139 -4
+ Partials 76 73 -3
Continue to review full report at Codecov.
|
yurishkuro
force-pushed
the
support-jaeger-baggage-header2
branch
from
d6600e9 to
1f79d9f
Compare
|
|
||
| private Map<String, String> parseBaggageHeader(String header, Map<String, String> baggage) { | ||
| String[] parts = header.split("\\ *,\\ *"); | ||
| for (int i = 0; i < parts.length; i++) { |
There was a problem hiding this comment.
minor nit: can be simplified to a range for loop because we don't use the outer index
for (String part : header.split(" *, *")) {
Also, are the regex escapes required here?
There was a problem hiding this comment.
I means \s, will change.
| } | ||
|
|
||
| private Map<String, String> parseBaggageHeader(String header, Map<String, String> baggage) { | ||
| String[] parts = header.split("\\ *,\\ *"); |
There was a problem hiding this comment.
Programmatically set baggage can have , or = in the baggage value while it seems adhoc baggage cannot. While it is obvious, we should call out this limitation in documentation.
There was a problem hiding this comment.
That seems kind of obvious if you're asked to provide baggage in the k1=v1, k2=v2 format.
| @@ -53,7 +53,7 @@ protected JaegerSpanContext( | |||
| String debugId, | |||
| JaegerObjectFactory objectFactory) { | |||
| if (baggage == null) { | |||
| throw new NullPointerException(); | |||
| baggage = Collections.<String, String>emptyMap(); | |||
Per jaegertracing/jaeger#1010