Skip to content
This repository has been archived by the owner on Jan 26, 2024. It is now read-only.

Support key lookup in span context extraction. #25

Merged
merged 2 commits into from
Nov 6, 2017
Merged

Support key lookup in span context extraction. #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
79f6ad7
Support key lookup in span context extraction.
rnburn Oct 13, 2017
2f5f8b7
Undo clang-formating change.
rnburn Oct 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions include/opentracing/propagation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,15 @@ const std::error_code invalid_carrier_error(2, propagation_error_category());
const std::error_code span_context_corrupted_error(
3, propagation_error_category());

// `key_not_found_error` occurs when TextMapReader::LookupKey fails to find
// an entry for the provided key.
const std::error_code key_not_found_error(4, propagation_error_category());

// `lookup_key_not_supported_error` occurs when TextMapReader::LookupKey is
// not supported for the provided key.
const std::error_code lookup_key_not_supported_error(
5, propagation_error_category());

// TextMapWriter is the Inject() carrier for the TextMap builtin format. With
// it, the caller can encode a SpanContext for propagation as entries in a map
// of unicode strings.
Expand All @@ -89,6 +98,18 @@ class TextMapReader {
public:
virtual ~TextMapReader() = default;

// LookupKey returns the value for the specified `key` if available. If no
// such key is present, it returns `key_not_found_error`.
//
// TextMapReaders are not required to implement this method. If not supported,
// the function returns `lookup_key_not_supported_error`.
//
// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Contributor Author

@rnburn rnburn Oct 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

return make_unexpected(lookup_key_not_supported_error);
}

// ForeachKey returns TextMap contents via repeated calls to the `f`
// function. If any call to `f` returns an error, ForeachKey terminates and
// returns that error.
Expand Down
16 changes: 13 additions & 3 deletions src/propagation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@ class PropagationErrorCategory : public std::error_category {
// Needed to fix bug in macOS build
// (https://travis-ci.org/isaachier/hunter/jobs/281868518).
// See https://stackoverflow.com/a/7411708/1930331 for justification.
PropagationErrorCategory()
{
}
PropagationErrorCategory() {}

const char* name() const noexcept override {
return "OpenTracingPropagationError";
Expand All @@ -27,6 +25,12 @@ class PropagationErrorCategory : public std::error_category {
if (code == span_context_corrupted_error.value()) {
return std::make_error_condition(std::errc::invalid_argument);
}
if (code == key_not_found_error.value()) {
return std::make_error_condition(std::errc::invalid_argument);
}
if (code == lookup_key_not_supported_error.value()) {
return std::make_error_condition(std::errc::not_supported);
}
return std::error_condition(code, *this);
}

Expand All @@ -40,6 +44,12 @@ class PropagationErrorCategory : public std::error_category {
if (code == span_context_corrupted_error.value()) {
return "opentracing: SpanContext data corrupted in Extract carrier";
}
if (code == key_not_found_error.value()) {
return "opentracing: SpanContext key not found";
}
if (code == lookup_key_not_supported_error.value()) {
return "opentracing: Lookup for the given key is not supported";
}
return "opentracing: unknown propagation error";
}
};
Expand Down