Implement W3C tracestate#779
Conversation
| * Field to capture tracestate header | ||
| * Internal representation of the tracestate | ||
| */ | ||
| private LinkedHashMap<String, String> internalList = new LinkedHashMap<String, String>(32); |
There was a problem hiding this comment.
A good practice to follow is use Interface.
Map<String, String> internalList = new LinkedHashMap<>(32). You also need not specify the types in diamond operator twice. By the way, LinkedHashMap is used to maintain insertion order. Do you need to maintain that order here?
There was a problem hiding this comment.
Is 32 the max limit according to spec?
There was a problem hiding this comment.
Yes, https://github.com/w3c/trace-context/blob/master/trace_context/HTTP_HEADER_FORMAT.md#header-value
There can be a maximum of 32 list-members in a list.
| for (String item : values) { | ||
| Matcher m = MEMBER_FORMAT_RE.matcher(item); | ||
| if (!m.find()) { | ||
| throw new IllegalArgumentException(String.format("invalid string %s in tracestate", item)); |
There was a problem hiding this comment.
In general, within Application Insights the practice that was followed was to not throw only, but do try - catch and Log. So that we don't break users application. Do we want to throw here (for generic library purpose and use try catch in implementation)?
There was a problem hiding this comment.
From class implementation perspective, it makes sense to throw if invalid argument is passed in.
For integration, we need to swallow the exception and ignore the broken header according to the W3C spec.
| /** | ||
| * Ctor that creates a tracestate object from a parent one | ||
| */ | ||
| public Tracestate(Tracestate parent, String key, String value) { |
There was a problem hiding this comment.
For application insights case we need another helper method also - which can basically swap the value of the azure key. Should this helper reside here? If not we would need to have almost similar method in the AI implementation.
There was a problem hiding this comment.
Can we use new Tracestate(tracestate, "key", "new value")?
dhaval24
left a comment
There was a problem hiding this comment.
In general this looks fine, though I am bit worried how will regex evaluation impact performance. In general if the tracestate is long enough than every eval operation would be O(N) where N is the size of tracestate. All this happens in request hot path. Though it should be fine for now.
@dhaval24