Support key lookup in span context extraction. #25
Conversation
| // | ||
| // Tracers may use this as an alternative to `ForeachKey` as a faster way to | ||
| // extract span context. | ||
| virtual expected<string_view> LookupKey(string_view /*key*/) const { |
There was a problem hiding this comment.
I'm super rusty in C++, but doesn't requiring the tracer implementation to run in try-catch introduces its own overhead? Exceptions should be used for exceptional situations, it's not an error or exception for the carrier to not support by-key lookup. I'd rather have something like optional<KeyedTextMapReader> getKeyedReader()
There was a problem hiding this comment.
This is an error code approach: it doesn't use exceptions.
In some of the projects that might use this (e.g. envoy, nginx), they index some of the keys but not all of them; so I tried to design it in a way that key-value lookup can be supported on a per-key basis, not just a simple on/off for everything.
There was a problem hiding this comment.
Wouldn't it be better to have this as an additional interface, which implementations can inherit? e.g.
class Indexable
{
public:
virtual optional<string_view> operator const = 0;
};
This way if you want to test if it is supported you can do a dynamic_cast. If you are some how certain that it will be supported you can just do a static_cast(I think) to avoid any RTTI overhead. You don't have to handle the not supported case in every call to LookupKey either, which will have its own overhead, even if it is just instruction cache use or branch prediction.
There was a problem hiding this comment.
@pantoss Lookup needs to be supported to be on a per-key level. Some applications don't index all of the header keys. For example, envoy provides fast lookup only for a list of hard-coded keys.
Some concerns have come up around the efficiency of extracting a span context. The current API forces tracer implementations to iterate and compare against all the key values in a carrier. If there’s no indexing on the keys, this is reasonable; but when there’s key indexing, it requires the tracer to do more comparisons than are necessary.
This PR adds optional support for looking up values by key. A carrier can either support lookup for all, some, or no keys; and it doesn’t break compatibility with the existing API. Tracers are still required to support extracting span context using the ForeachKey method, but can also take advantage of the lookup API for faster extraction if available.