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: general tracing support #16049

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from 15 commits
Commits
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
8 changes: 8 additions & 0 deletions include/envoy/common/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,11 @@ envoy_cc_library(
name = "scope_tracker_interface",
hdrs = ["scope_tracker.h"],
)

envoy_cc_library(
name = "lower_case_string",
hdrs = ["lower_case_string.h"],
deps = [
"//source/common/common:assert_lib",
],
)
83 changes: 83 additions & 0 deletions include/envoy/common/lower_case_string.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
#pragma once

#include <string>
#include <vector>

#include "common/common/assert.h"

#include "absl/strings/ascii.h"
#include "absl/strings/string_view.h"

namespace Envoy {

// Used by ASSERTs to validate internal consistency. E.g. valid HTTP header keys/values should
// never contain embedded NULLs.
static inline bool validHeaderString(absl::string_view s) {
// If you modify this list of illegal embedded characters you will probably
Copy link
Contributor

Choose a reason for hiding this comment

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

drive-by comment: This is HTTP-specific and I think should under an http/ directory rather than under common.

I think I see why you wanted to do this: you wanted to add similar support for tracing that is not dependent on http, but then I think you would want to make the validator be pluggable or something, maybe via a template?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks.😄 Yes, I want add some similar support for tracing. A simple function pointer may be a good choice. Templates or virtual functions will introduce some type conversion problems, which is completely unnecessary.
The validation logic of validHeaderString should be universal, that is, it does not contain newline characters and does not contain null. We can use it as the default validator.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not understanding why templates would create type conversion problems. But if you are going to use a function pointer you should use a new-style lambda.

Also you probably want to avoid having include/envoy/common/... reference include/envoy/http/... -- at least in my mind that should be a directed graph.

Copy link
Member Author

@wbpcode wbpcode Apr 30, 2021

Choose a reason for hiding this comment

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

@jmarantz

I'm not understanding why templates would create type conversion problems. But if you are going to use a function pointer you should use a new-style lambda.

Don't care, I should be wrong in my thinking.

Also you probably want to avoid having include/envoy/common/... reference include/envoy/http/... -- at least in my mind that should be a directed graph.

🤔 I was thinking too. Perhaps we can simply treat validHeaderString as a general validator. It does not have any special HTTP-specific logic, just ensure that the string is an ordinary single-line string that does not contain nulls.

This part will be a new independent PR. When it's ready, I will @ you, thanks 😄 .

Copy link
Contributor

Choose a reason for hiding this comment

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

It is based on http char-set, though, isn't it?

The use of a template, or for that matter, a function pointer, lambda, or pure virtual function call, is that you could put the generic impl into common/ and the http-specific variant in http/ and you have a clean separation.

Come to think of it, do you want to always lower-case names for non-http?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jmarantz Thank you very much for your patience and suggestions.

If I just want to get a general LowerCaseString implementation, your idea is undoubtedly correct. But this question is somewhat different.

Why I want a common LowerCaseString?

Because using RequestHeaderMapImpl to implement the setTraceContext(absl::string_view)/getTraceContext(absl::string_view) interface, additional overhead will be introduced. We need to convert absl::string_view to lower case and always use copy semantics. So I hope to design a general LowerCaseString and add two overloaded interfaces that use LowerCaseString to TraceContext. In this way, for existing Http Tracer users, the introduction of general tracing will not bring any additional overhead.

Why is it difficult to achieve clean separation?

If I use virtual functions or inheritance solutions, use the base class in the TraceContext interface, such as LowerCaseStringBase. Then when RequestHeaderMapImpl implements the TraceContext interface, it will involve the protocol conversion from LowerCaseStringBase to Http::LowerCaseString (I now have some new ideals to circumvent this problem).
If I use function pointers, then we must use validHeaderString as the default value, because now Http::LowerCaseString has been widely used, we need to reduce the impact on existing code. The problem of using templates is similar to that of function pointers. In other words, LowerCaseString will depend on validHeaderString.

// want to change header_map_fuzz_impl_test at the same time.
for (const char c : s) {
switch (c) {
case '\0':
FALLTHRU;
case '\r':
FALLTHRU;
case '\n':
return false;
default:
continue;
}
}
return true;
}

/**
* Wrapper for a lower case string used in header operations to generally avoid needless case
* insensitive compares.
*/
class LowerCaseString {
public:
LowerCaseString(LowerCaseString&& rhs) noexcept : string_(std::move(rhs.string_)) {
ASSERT(valid());
}
LowerCaseString& operator=(LowerCaseString&& rhs) noexcept {
string_ = std::move(rhs.string_);
ASSERT(valid());
return *this;
}

LowerCaseString(const LowerCaseString& rhs) : string_(rhs.string_) { ASSERT(valid()); }
LowerCaseString& operator=(const LowerCaseString& rhs) {
string_ = std::move(rhs.string_);
ASSERT(valid());
return *this;
}

explicit LowerCaseString(const absl::string_view new_string) : string_(new_string) {
ASSERT(valid());
lower();
}

const std::string& get() const { return string_; }
bool operator==(const LowerCaseString& rhs) const { return string_ == rhs.string_; }
bool operator!=(const LowerCaseString& rhs) const { return string_ != rhs.string_; }
bool operator<(const LowerCaseString& rhs) const { return string_.compare(rhs.string_) < 0; }

friend std::ostream& operator<<(std::ostream& os, const LowerCaseString& lower_case_string) {
return os << lower_case_string.string_;
}

private:
void lower() {
std::transform(string_.begin(), string_.end(), string_.begin(), absl::ascii_tolower);
}
bool valid() const { return validHeaderString(string_); }

std::string string_;
};

/**
* Convenient type for a vector of lower case string and string pair.
*/
using LowerCaseStrPairVector = std::vector<std::pair<const LowerCaseString, const std::string>>;

} // namespace Envoy
2 changes: 2 additions & 0 deletions include/envoy/http/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ envoy_cc_library(
],
deps = [
":header_formatter_interface",
"//include/envoy/common:lower_case_string",
"//include/envoy/tracing:trace_context_interface",
"//source/common/common:assert_lib",
"//source/common/common:hash_lib",
],
Expand Down
77 changes: 6 additions & 71 deletions include/envoy/http/header_map.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@
#include <string>
#include <vector>

#include "envoy/common/lower_case_string.h"
#include "envoy/common/optref.h"
#include "envoy/common/pure.h"
#include "envoy/http/header_formatter.h"
#include "envoy/tracing/trace_context.h"

#include "common/common/assert.h"
#include "common/common/hash.h"
Expand All @@ -22,76 +24,8 @@
namespace Envoy {
namespace Http {

// Used by ASSERTs to validate internal consistency. E.g. valid HTTP header keys/values should
// never contain embedded NULLs.
static inline bool validHeaderString(absl::string_view s) {
// If you modify this list of illegal embedded characters you will probably
// want to change header_map_fuzz_impl_test at the same time.
for (const char c : s) {
switch (c) {
case '\0':
FALLTHRU;
case '\r':
FALLTHRU;
case '\n':
return false;
default:
continue;
}
}
return true;
}

/**
* Wrapper for a lower case string used in header operations to generally avoid needless case
* insensitive compares.
*/
class LowerCaseString {
public:
LowerCaseString(LowerCaseString&& rhs) noexcept : string_(std::move(rhs.string_)) {
ASSERT(valid());
}
LowerCaseString& operator=(LowerCaseString&& rhs) noexcept {
string_ = std::move(rhs.string_);
ASSERT(valid());
return *this;
}

LowerCaseString(const LowerCaseString& rhs) : string_(rhs.string_) { ASSERT(valid()); }
LowerCaseString& operator=(const LowerCaseString& rhs) {
string_ = std::move(rhs.string_);
ASSERT(valid());
return *this;
}

explicit LowerCaseString(const std::string& new_string) : string_(new_string) {
ASSERT(valid());
lower();
}

const std::string& get() const { return string_; }
bool operator==(const LowerCaseString& rhs) const { return string_ == rhs.string_; }
bool operator!=(const LowerCaseString& rhs) const { return string_ != rhs.string_; }
bool operator<(const LowerCaseString& rhs) const { return string_.compare(rhs.string_) < 0; }

friend std::ostream& operator<<(std::ostream& os, const LowerCaseString& lower_case_string) {
return os << lower_case_string.string_;
}

private:
void lower() {
std::transform(string_.begin(), string_.end(), string_.begin(), absl::ascii_tolower);
}
bool valid() const { return validHeaderString(string_); }

std::string string_;
};

/**
* Convenient type for a vector of lower case string and string pair.
*/
using LowerCaseStrPairVector =
std::vector<std::pair<const Http::LowerCaseString, const std::string>>;
using LowerCaseString = Envoy::LowerCaseString;
using LowerCaseStrPairVector = Envoy::LowerCaseStrPairVector;

/**
* Convenient type for an inline vector that will be used by HeaderString.
Expand Down Expand Up @@ -817,7 +751,8 @@ class RequestOrResponseHeaderMap : public HeaderMap {
// Request headers.
class RequestHeaderMap
: public RequestOrResponseHeaderMap,
public CustomInlineHeaderBase<CustomInlineHeaderRegistry::Type::RequestHeaders> {
public CustomInlineHeaderBase<CustomInlineHeaderRegistry::Type::RequestHeaders>,
public Tracing::TraceContext {
public:
INLINE_REQ_STRING_HEADERS(DEFINE_INLINE_STRING_HEADER)
INLINE_REQ_NUMERIC_HEADERS(DEFINE_INLINE_NUMERIC_HEADER)
Expand Down
12 changes: 6 additions & 6 deletions include/envoy/server/tracer_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,22 +41,22 @@ class TracerFactory : public Config::TypedFactory {
~TracerFactory() override = default;

/**
* Create a particular HttpTracer implementation. If the implementation is unable to produce an
* HttpTracer with the provided parameters, it should throw an EnvoyException in the case of
* general error or a Json::Exception if the json configuration is erroneous. The returned
* Create a particular trace driver implementation. If the implementation is unable to produce
* a trace driver with the provided parameters, it should throw an EnvoyException in the case
* of general error or a Json::Exception if the json configuration is erroneous. The returned
* pointer should always be valid.
*
* NOTE: Due to the corner case of OpenCensus, who can only support a single tracing
* configuration per entire process, the returned HttpTracer instance is not guaranteed
* configuration per entire process, the returned Driver instance is not guaranteed
* to be unique.
* That is why the return type has been changed to std::shared_ptr<> instead of a more
* idiomatic std::unique_ptr<>.
*
* @param config supplies the proto configuration for the HttpTracer
* @param context supplies the factory context
*/
virtual Tracing::HttpTracerSharedPtr createHttpTracer(const Protobuf::Message& config,
TracerFactoryContext& context) PURE;
virtual Tracing::DriverSharedPtr createTracerDriver(const Protobuf::Message& config,
TracerFactoryContext& context) PURE;

std::string category() const override { return "envoy.tracers"; }
};
Expand Down
18 changes: 16 additions & 2 deletions include/envoy/tracing/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,26 @@ licenses(["notice"]) # Apache 2

envoy_package()

envoy_cc_library(
name = "trace_context_interface",
hdrs = ["trace_context.h"],
)

envoy_cc_library(
name = "trace_driver_interface",
hdrs = ["trace_driver.h"],
deps = [
":trace_context_interface",
":trace_reason_interface",
"//include/envoy/stream_info:stream_info_interface",
],
)

envoy_cc_library(
name = "http_tracer_interface",
hdrs = ["http_tracer.h"],
deps = [
":trace_reason_interface",
"//include/envoy/access_log:access_log_interface",
":trace_driver_interface",
"//include/envoy/http:header_map_interface",
],
)
Expand Down
Loading