Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 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
1 change: 0 additions & 1 deletion bazel/external/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ cc_library(
name = "all_external",
srcs = [":empty.cc"],
deps = [
"@com_github_lightstep_lightstep_tracer_cpp//:lightstep",

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

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

36 changes: 25 additions & 11 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()
_com_github_opentracing_opentracing_cpp()
_com_github_lightstep_lightstep_tracer_cpp()
_com_github_nodejs_http_parser()
_com_github_tencent_rapidjson()
Expand Down Expand Up @@ -312,23 +313,36 @@ def _com_github_gcovr_gcovr():
actual = "@com_github_gcovr_gcovr//:gcovr",
)

def _com_github_opentracing_opentracing_cpp():
location = REPOSITORY_LOCATIONS[

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please use _repository_impl() here so that downstream builders of Envoy can override the dependency definition.

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

You'll want to adjust the repository_locations.bzl name to match.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed to use _repository_impl and matched names to keys in REPOSITORY_LOCATIONS.

"com_github_opentracing_opentracing_cpp"]
native.git_repository(
name = "io_opentracing_cpp",
remote = location["remote"],
commit = location["commit"],
)
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",
native.git_repository(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here.

name = "com_lightstep_tracer_cpp",
remote = location["remote"],
commit = location["commit"],
)
native.new_git_repository(
name = "lightstep_vendored_googleapis",
remote = location["googleapis"]["remote"],
commit = location["googleapis"]["commit"],
build_file = "@com_lightstep_tracer_cpp//:lightstep-tracer-common/third_party/googleapis/BUILD",
)
native.bind(
name = "lightstep",
actual = "@com_github_lightstep_lightstep_tracer_cpp//:lightstep",
actual = "@com_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",
),
com_github_opentracing_opentracing_cpp = dict(
commit = "713c15d40ae63185d2bec99bf3b03823967d7108",
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 = "c9d9215b5c3652c7eb5640903697d9a683e1df76",
remote = "https://github.com/lightstep/lightstep-tracer-cpp",
googleapis = dict(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's pretty confusing to have this extra level of nesting. How about giving googleapis its own location?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Separated out. Is it ok to build target in the same function as lightstep?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think so. The functions only exist for human readability, and it makes sense to have all the lightstep stuff in one place.

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 O(1) headers by key.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be clearer to refer to these as something like "predefined inline header (see ALL_INLINE_HEADERS below)". Using the complexity alone elides the semantic info here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed naming.

* @param key supplies the header key.
* @param entry is set to the header entry if it exists and if key is one of the O(1) 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 O(1) 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
17 changes: 17 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,23 @@ 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@rnburn rnburn Nov 22, 2017

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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) {
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