opentracing / opentracing-java

OpenTracing API for Java. 🛑 This library is DEPRECATED! https://github.com/opentracing/specification/issues/163
http://opentracing.io
Apache License 2.0
1.68k stars 345 forks source link

TextMap and get(String key) #169

Open Scottmitch opened 6 years ago

Scottmitch commented 6 years ago

The Tracer APIs demonstrate using TextMap as the carrier when extracting tracing information from HTTP headers [1]. However the TextMap API lacks a get(String) type API, and instead provides an iterator. This makes the cost of extracting values from a TextMap proportional to the number of headers and requires iterator creation for each extraction. Can someone explain the rational for the lack of a get(..) API on TextMap, and is there an alternative approach/carrier to use which does provide this type of API?

[1] https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/Tracer.java#L78

yurishkuro commented 6 years ago

If I remember correctly, there were two reasons:

  1. some transports may not give by-key access to the headers (because of streaming, what not)
  2. in case of HTTP, there's usually no way of telling how the headers are modified, some frameworks might present them as lowercase, others in uppercase, others in First-Cap-Before-Dash.
Scottmitch commented 6 years ago

some transports may not give by-key access to the headers (because of streaming, what not)

In case of streaming is the thought that the iterator will block until more headers are present? In my experience it is not common to stream the meta data portion of requests (e.g. headers) because this may dictate how the body is processed.

in case of HTTP, there's usually no way of telling how the headers are modified, some frameworks might present them as lowercase, others in uppercase, others in First-Cap-Before-Dash.

I see. That would be the responsibility for TextMap implementations if they must preserve HTTP semantics. For example Netty's HttpHeaders provides a Map API with get(..) that accounts for case insensitive compares. The method in TextMap could even be default:

@Nullable
default String get(CharSequence name) {
  // use iterator to find the value, this method can be overridden if comparison or lookup can be improved.
}
yurishkuro commented 6 years ago

I just remembered another reason. Some tracers encode baggage in http headers as {some-prefix}-{baggage-key}: {baggage-value | urlencode}. The benefits of this format are (a) it's easily readable on the wire / tcpdump, (b) it imposes less limitations on the length of baggage as opposed to encoding everything into one header. However, when deserializing this back from HTTP headers there is no way for the tracer to know which keys to as for from the Headers map, it can only do it by iterating and checking the prefix.

In retrospect, the same could've been achieved via different encoding, e.g.

{some-prefix}-0: {number of baggage items}; {baggage-key-0}={baggage-value-0}
{some-prefix}-n: {baggage-key-n}={baggage-value-n}

That encoding would've also addressed the problem of the keys changing case in some http frameworks.

yurishkuro commented 6 years ago

Given that changing the text map to provide direct get/set methods would be backwards incompatible change, perhaps the alternative is to allow the carriers support an additional interface with direct get method, so that tracers that are able to resolve their context via get could check for that interface and use it, otherwise fallback to iterating.

Scottmitch commented 6 years ago

Given that changing the text map to provide direct get/set methods would be backwards incompatible change

Is this true? As suggested in https://github.com/opentracing/opentracing-java/issues/169#issuecomment-324694434 it could be done with a default method.

bhs commented 6 years ago

@scottmitch we are restricted to Java 1.6 features due to compatibility issues with Android (and other legacy stuff); I believe that prevents us from pursuing the approach above.

Scottmitch commented 6 years ago

@bhs - thanks for clarifying. 1.6 compatibility makes sense.

yurishkuro commented 6 years ago

@Scottmitch are you trying to integrate Netty with OpenTracing? Are you able to share the code?

Scottmitch commented 6 years ago

@yurishkuro - please reach out to me offline to discuss in more detail

yurishkuro commented 6 years ago

@Scottmitch ping me at ys@uber.com ?

carlosalberto commented 6 years ago

Going to mark this issue as "Enhancement" as it looks we need Java 1.7.x and above. Is this the case? @yurishkuro

yurishkuro commented 6 years ago

Only if we did this via default method. An optional interface would work with 1.6.

The other thing is - only one person has ever asked for this. I think rule of three should apply.