Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tracing: Support OpenTracing API #2017

Merged
merged 27 commits into from
Dec 16, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
773211e
Support OpenTracing API.
rnburn Nov 8, 2017
e57a0fd
Resolve merge conflicts.
rnburn Nov 9, 2017
4af99ef
Const correctness fixes.
rnburn Nov 9, 2017
93ab508
Use PURE instead of "= 0".
rnburn Nov 9, 2017
ad4062a
Log Inject/Extract failures at debug level.
rnburn Nov 9, 2017
739cd54
Avoid copying when deserializing LightStep response.
rnburn Nov 10, 2017
6270b3f
Change propagation options to be const.
rnburn Nov 10, 2017
4e03f60
Merge with build modifications.
rnburn Nov 13, 2017
ab3db85
Fix lightstep build.
rnburn Nov 14, 2017
cee939f
Replace nullptr branching with RELEASE_ASSERT.
rnburn Nov 14, 2017
c775ded
s/header_map_callback/headerMapCallback/
rnburn Nov 14, 2017
f1db61f
Use stats to track span context injection/extraction failures.
rnburn Nov 20, 2017
94750bd
Add TODO note for Grpc::AsyncClient.
rnburn Nov 22, 2017
1e255d4
s/O(1) header/predefined inline header/
rnburn Nov 22, 2017
725d002
Add note for const_cast.
rnburn Nov 22, 2017
961e195
Document OpenTracingDriver.
rnburn Nov 22, 2017
ecf6df1
Control span context propagation with enum instead of bools.
rnburn Nov 23, 2017
0803ff2
Avoid copy associated with std::istringstream.
rnburn Nov 27, 2017
5d62c51
Ensure span-context headers aren't duplicated.
rnburn Dec 3, 2017
3ff6aaf
Remove default case from switch statment.
rnburn Dec 6, 2017
cc6a7e2
s/MemoryStreamBuffer/ConstMemoryStreamBuffer/
rnburn Dec 6, 2017
739bf4a
s/IMemoryStream/InputConstMemoryStream/
rnburn Dec 6, 2017
cf7896b
Fix ordering of member functions.
rnburn Dec 6, 2017
ce00ac6
Document reason for choosing const_cast.
rnburn Dec 6, 2017
861e87c
Don't move from std::shared_ptr.
rnburn Dec 6, 2017
562746e
Set propagation mode via constructor.
rnburn Dec 7, 2017
a9411ad
Make propagation_mode_ const.
rnburn Dec 11, 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
3 changes: 2 additions & 1 deletion bazel/external/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ cc_library(
name = "all_external",
srcs = [":empty.cc"],
deps = [
"@com_github_lightstep_lightstep_tracer_cpp//:lightstep",
Copy link
Contributor

Choose a reason for hiding this comment

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

This target is used to pre-build the dependencies when generating the Docker image. We still want to pre-build Lightstep and all its deps since they're pretty big.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

"@io_opentracing_cpp//:opentracing",
"@com_github_lightstep_lightstep_tracer_cpp//:lightstep_tracer",
"@com_google_googletest//:gtest",
],
)
Expand Down
50 changes: 0 additions & 50 deletions bazel/external/lightstep.BUILD

This file was deleted.

23 changes: 0 additions & 23 deletions bazel/external/lightstep.genrule_cmd

This file was deleted.

26 changes: 13 additions & 13 deletions bazel/repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ def envoy_dependencies(path = "@envoy_deps//", skip_targets = []):
_com_github_fmtlib_fmt()
_com_github_gabime_spdlog()
_com_github_gcovr_gcovr()
_io_opentracing_cpp()
_com_github_lightstep_lightstep_tracer_cpp()
_com_github_nodejs_http_parser()
_com_github_tencent_rapidjson()
Expand Down Expand Up @@ -312,23 +313,22 @@ def _com_github_gcovr_gcovr():
actual = "@com_github_gcovr_gcovr//:gcovr",
)

def _io_opentracing_cpp():
_repository_impl("io_opentracing_cpp")
native.bind(
name = "opentracing",
actual = "@io_opentracing_cpp//:opentracing",
)

def _com_github_lightstep_lightstep_tracer_cpp():
location = REPOSITORY_LOCATIONS[
"com_github_lightstep_lightstep_tracer_cpp"]
genrule_repository(
name = "com_github_lightstep_lightstep_tracer_cpp",
urls = location["urls"],
sha256 = location["sha256"],
strip_prefix = location["strip_prefix"],
patches = [
"@envoy//bazel/external:lightstep-missing-header.patch",
],
genrule_cmd_file = "@envoy//bazel/external:lightstep.genrule_cmd",
build_file = "@envoy//bazel/external:lightstep.BUILD",
_repository_impl("com_github_lightstep_lightstep_tracer_cpp")
_repository_impl(
name = "lightstep_vendored_googleapis",
build_file = "@com_github_lightstep_lightstep_tracer_cpp//:lightstep-tracer-common/third_party/googleapis/BUILD",
)
native.bind(
name = "lightstep",
actual = "@com_github_lightstep_lightstep_tracer_cpp//:lightstep",
actual = "@com_github_lightstep_lightstep_tracer_cpp//:lightstep_tracer",
)

def _com_github_tencent_rapidjson():
Expand Down
13 changes: 10 additions & 3 deletions bazel/repository_locations.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,17 @@ REPOSITORY_LOCATIONS = dict(
commit = "c0d77201039c7b119b18bc7fb991564c602dd75d",
remote = "https://github.com/gcovr/gcovr",
),
io_opentracing_cpp = dict(
commit = "550c686e0e174c845a034f432a6c31a808f5f994",
remote = "https://github.com/opentracing/opentracing-cpp",
),
com_github_lightstep_lightstep_tracer_cpp = dict(
sha256 = "f7477e67eca65f904c0b90a6bfec46d58cccfc998a8e75bc3259b6e93157ff84",
strip_prefix = "lightstep-tracer-cpp-0.36",
urls = ["https://github.com/lightstep/lightstep-tracer-cpp/releases/download/v0_36/lightstep-tracer-cpp-0.36.tar.gz"],
commit = "deb5284395075028c3e5b4eab1416fe1e597bdb7",
remote = "https://github.com/lightstep/lightstep-tracer-cpp",
),
lightstep_vendored_googleapis = dict(
commit = "d6f78d948c53f3b400bb46996eb3084359914f9b",
remote = "https://github.com/google/googleapis",
),
com_github_nodejs_http_parser = dict(
commit = "feae95a3a69f111bc1897b9048d9acbc290992f9", # v2.7.1
Expand Down
3 changes: 0 additions & 3 deletions ci/build_container/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,3 @@ $(THIRDPARTY_DEPS)/%.dep: $(RECIPES)/%.sh

# Special support for targets that need protobuf, and hence take a dependency on protobuf.dep.
PROTOBUF_BUILD ?= $(THIRDPARTY_BUILD)/$(if $(BUILD_DISTINCT),protobuf.dep,)

$(THIRDPARTY_DEPS)/lightstep.dep: $(RECIPES)/lightstep.sh $(THIRDPARTY_DEPS)/protobuf.dep
@+$(call build-recipe,PROTOBUF_BUILD=$(PROTOBUF_BUILD))
14 changes: 13 additions & 1 deletion include/envoy/http/header_map.h
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ class HeaderMap {
/**
* Get a header by key.
* @param key supplies the header key.
* @return the header entry if it exsits otherwise nullptr.
* @return the header entry if it exists otherwise nullptr.
*/
virtual const HeaderEntry* get(const LowerCaseString& key) const PURE;

Expand Down Expand Up @@ -384,6 +384,18 @@ class HeaderMap {
*/
virtual void iterateReverse(ConstIterateCb cb, void* context) const PURE;

enum class Lookup { Found, NotFound, NotSupported };

/**
* Lookup one of the predefined inline headers (see ALL_INLINE_HEADERS below) by key.
* @param key supplies the header key.
* @param entry is set to the header entry if it exists and if key is one of the predefined inline
* headers; otherwise, nullptr.
* @return Lookup::Found if lookup was successful, Lookup::NotFound if the header entry doesn't
* exist, or Lookup::NotSupported if key is not one of the predefined inline headers.
*/
virtual Lookup lookup(const LowerCaseString& key, const HeaderEntry** entry) const PURE;

/**
* Remove all instances of a header by key.
* @param key supplies the header key to remove.
Expand Down
1 change: 1 addition & 0 deletions source/common/common/logger.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ namespace Logger {
FUNCTION(router) \
FUNCTION(runtime) \
FUNCTION(testing) \
FUNCTION(tracing) \
FUNCTION(upstream)

enum class Id {
Expand Down
11 changes: 11 additions & 0 deletions source/common/common/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,17 @@ std::string DateFormatter::now() {
ProdSystemTimeSource ProdSystemTimeSource::instance_;
ProdMonotonicTimeSource ProdMonotonicTimeSource::instance_;

ConstMemoryStreamBuffer::ConstMemoryStreamBuffer(const char* data, size_t size) {
// std::streambuf won't modify `data`, but the interface still requires a char* for convenience,
// so we need to const_cast.
char* ptr = const_cast<char*>(data);

this->setg(ptr, ptr, ptr + size);
}

InputConstMemoryStream::InputConstMemoryStream(const char* data, size_t size)
: ConstMemoryStreamBuffer{data, size}, std::istream{static_cast<std::streambuf*>(this)} {}

bool DateUtil::timePointValid(SystemTime time_point) {
return std::chrono::duration_cast<std::chrono::milliseconds>(time_point.time_since_epoch())
.count() != 0;
Expand Down
20 changes: 20 additions & 0 deletions source/common/common/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,26 @@ class ProdMonotonicTimeSource : public MonotonicTimeSource {
static ProdMonotonicTimeSource instance_;
};

/**
* Class used for creating non-copying std::istream's. See InputConstMemoryStream below.
*/
class ConstMemoryStreamBuffer : public std::streambuf {
public:
ConstMemoryStreamBuffer(const char* data, size_t size);
};

/**
* std::istream class similar to std::istringstream, except that it provides a view into a region of
* constant memory. It can be more efficient than std::istringstream because it doesn't copy the
* provided string.
*
* See https://stackoverflow.com/a/13059195/4447365.
*/
class InputConstMemoryStream : public virtual ConstMemoryStreamBuffer, public std::istream {
public:
InputConstMemoryStream(const char* data, size_t size);
};

/**
* Utility class for date/time helpers.
*/
Expand Down
23 changes: 23 additions & 0 deletions source/common/http/header_map_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,29 @@ void HeaderMapImpl::iterateReverse(ConstIterateCb cb, void* context) const {
}
}

HeaderMap::Lookup HeaderMapImpl::lookup(const LowerCaseString& key,
const HeaderEntry** entry) const {
StaticLookupEntry::EntryCb cb = ConstSingleton<StaticLookupTable>::get().find(key.get().c_str());
Copy link
Member

Choose a reason for hiding this comment

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

Could we potentially have a findConst() or something which returns a ConstEntryCb so that we don't need the const_cast below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Via a macro, each of the O(1) headers adds a lambda funtion that takes HTTPHeaderMapImpl& as an argument. The only way I see around const_cast is to have them also add entries for a const HTTPHeaderMapImpl& lambda.

IMO const_cast is the better option.

Copy link
Member

Choose a reason for hiding this comment

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

I could go either way. I think it's slightly cleaner to have the init code add a const callback and then have a fineConst(), but I'm ok with this. I would just add a comment on the tradeoff.

Copy link
Contributor Author

@rnburn rnburn Nov 22, 2017

Choose a reason for hiding this comment

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

Added comment for const_cast.

Copy link
Member

Choose a reason for hiding this comment

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

nit: For the next person that comes along and asks "why const_cast" can you add a brief additional comment on the tradeoffs between this and ^.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added explanation for choosing const_cast.

if (cb) {
// The accessor callbacks for predefined inline headers take a HeaderMapImpl& as an argument;
// even though we don't make any modifications, we need to cast_cast in order to use the
// accessor.
//
// Making this work without const_cast would require managing an additional const accessor
// callback for each predefined inline header and add to the complexity of the code.
StaticLookupResponse ref_lookup_response = cb(const_cast<HeaderMapImpl&>(*this));
*entry = *ref_lookup_response.entry_;
if (*entry) {
return Lookup::Found;
} else {
return Lookup::NotFound;
}
} else {
*entry = nullptr;
return Lookup::NotSupported;
}
}

void HeaderMapImpl::remove(const LowerCaseString& key) {
StaticLookupEntry::EntryCb cb = ConstSingleton<StaticLookupTable>::get().find(key.get().c_str());
if (cb) {
Expand Down
1 change: 1 addition & 0 deletions source/common/http/header_map_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ class HeaderMapImpl : public HeaderMap {
const HeaderEntry* get(const LowerCaseString& key) const override;
void iterate(ConstIterateCb cb, void* context) const override;
void iterateReverse(ConstIterateCb cb, void* context) const override;
Lookup lookup(const LowerCaseString& key, const HeaderEntry** entry) const override;
void remove(const LowerCaseString& key) override;
size_t size() const override { return headers_.size(); }

Expand Down
4 changes: 4 additions & 0 deletions source/common/tracing/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,21 @@ envoy_cc_library(
name = "http_tracer_lib",
srcs = [
"http_tracer_impl.cc",
"opentracing_driver_impl.cc",
],
hdrs = [
"http_tracer_impl.h",
"opentracing_driver_impl.h",
],
external_deps = ["opentracing"],
deps = [
"//include/envoy/local_info:local_info_interface",
"//include/envoy/runtime:runtime_interface",
"//include/envoy/thread_local:thread_local_interface",
"//include/envoy/tracing:http_tracer_interface",
"//include/envoy/upstream:cluster_manager_interface",
"//source/common/access_log:access_log_formatter_lib",
"//source/common/buffer:zero_copy_input_stream_lib",
"//source/common/common:base64_lib",
"//source/common/common:macros",
"//source/common/common:utility_lib",
Expand Down
Loading