tracing: general tracing support by wbpcode · Pull Request #16049 · envoyproxy/envoy

wbpcode · 2021-04-18T05:53:58Z

Signed-off-by: wbpcode comems@msn.com

Commit Message: tracing: general tracing support
Additional Description:

Current tracing system only has great support to HTTP protocol. Other protocol such as Dubbo Proxy or Thrift Proxy cannot reuse tracers that designed for HTTP protocol.
This PR try to separate tracer implementation and HTTP protocol to make the current tracers protocol-independent.

There are mainly two protocol depentent design in current tracing system.

First, Http::RequestHeaderMap for tracing context. Tracing Driver will try to obtain tracing context from downstream Http::RequestHeaderMap and create tracing span. Tracing span will inject new context to Http::RequestHeaderMap to transfer it to upstream.
Second, Tracing driver factory will create HTTP tracing wrapper HttpTracer rather then raw Driver.

So, this PR design a new abstract class for tracing context to replace Http::RequestHeaderMap (While other protocols can implement an Http::RequestHeaderMap to directly reuse the existing Tracer, this requires a significant amount of work.):

class TracingContext {
public:
  virtual ~TracingContext() = default;

  /**
   * Get tracing context value by key.
   *
   * @param key The context key of string view type. The context key should be a string view of a
   * const lowercase string.
   * @return The optional context value of string_view type.
   */
  virtual absl::optional<absl::string_view>
  getTracingContext(const absl::string_view key) const PURE;

  /**
   * Set new tracing context key/value pair.
   *
   * @param key The context key of string view type. The context key should be a string view of a
   * const lowercase string with a longer lifetime than the current TracingContext object.
   * @return The optional context value of string_view type.
   */
  virtual void setTracingContext(const absl::string_view key, const absl::string_view value) PURE;
};

And then, we update tracing driver factory to make it no deps to HttpTracer:

class TracerFactory : public Config::TypedFactory {
public:
  ~TracerFactory() override = default;

  virtual Tracing::DriverSharedPtr createTracerDriver(const Protobuf::Message& config,
                                                      TracerFactoryContext& context) PURE;

All other code modifications are just to adapt to the above changes.

A more detailed doc:
https://docs.google.com/document/d/1gxk5vfHl-Gt3wiNZ-f46Dwmk2NFiEzFJQugmZmpSXk8/edit?usp=sharing

Risk Level: mid
Testing: [WIP]
Docs Changes: N/A
Release Notes: [WIP]

repokitteh-read-only · 2021-04-18T05:54:07Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #16049 was opened by wbpcode.

see: more, trace.

wbpcode · 2021-04-18T06:09:33Z

cc @mattklein123 I think this simple design can make tracers become protocol-independent. But I don’t know if the community has any opinions? 🤔

Signed-off-by: wbpcode <comems@msn.com>

wbpcode · 2021-04-21T07:30:43Z

It's ready for a first review. @mattklein123
This PR changes the RequestHeaderMap in each tracer to TracingContext, so it seems that there are a lot of file updates. But there is almost no logical change.

If necessary, I can split this PR into multiple PRs, which can reduce the complexity of review. 😄

…al-tracing

lizan · 2021-04-21T23:07:12Z

Anyone from @envoyproxy/first-pass-reviewers want to take this? At a quick glance it is in right direction.

rojkov

Basically this looks very good to me. Just couple of things to clarify.

include/envoy/server/tracer_config.h

rojkov · 2021-04-22T07:05:51Z

include/envoy/tracing/trace_driver.h

+
+  /**
+   * The way how to apply the custom tag to the span,
+   * generally obtain the tag value from the context and attached it to the span.


Should it be "generally obtains the tag value from the context and attaches it to the span"?

rojkov · 2021-04-22T09:21:55Z

source/common/tracing/http_tracer_impl.cc


 absl::string_view RequestHeaderCustomTag::value(const CustomTagContext& ctx) const {
-  if (!ctx.request_headers) {
+  if (!ctx.tracing_context) {


Better make it more explicit

if (ctx.tracing_context != nullptr) {

rojkov · 2021-04-22T09:23:34Z

source/common/tracing/http_tracer_impl.cc

 const envoy::config::core::v3::Metadata*
 MetadataCustomTag::metadata(const CustomTagContext& ctx) const {
-  const StreamInfo::StreamInfo& info = ctx.stream_info;
+  if (!ctx.stream_info) {


rojkov · 2021-04-22T09:31:54Z

source/common/tracing/http_tracer_impl.h

+
 private:
-  DriverPtr driver_;
+  DriverSharedPtr driver_;


Whom the driver is shared with? I wonder if collective ownership is needed here.

Yep. The factory will keep a shared_ptr of OpenCensus driver to ensure its uniqueness. This is a long-standing problem and I hope it can be resolved in the future.

/** * Create a particular tracer driver implementation. If the implementation is unable to produce * an tracer 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 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::DriverSharedPtr createTracerDriver(const Protobuf::Message& config, TracerFactoryContext& context) PURE;

I see. Thanks!

wbpcode · 2021-04-23T01:44:48Z

@rojkov Thank you very much for your kind review comments. I will update this PR this weekend. 😃

rojkov · 2021-04-23T08:43:50Z

api/envoy/config/trace/v3/tracer.proto

+message TracerProvider {
+  // The name of the trace driver to instantiate. The name must match a supported trace driver.
+  // See the :ref:`extensions listed in typed_config below <extension_category_envoy.tracers>` for the
+  // default list of the trace driver.


In other places I see "tracing driver", "tracer driver" in addition to "trace driver" used interchangeably. Can we stick to a single term?

rojkov · 2021-04-23T08:50:16Z

include/envoy/tracing/trace_context.h

+ * protocol requests and provides Tracer Driver with common methods for obtaining and setting the
+ * Tracing context.
+ */
+class TracingContext {


Better rename the file to tracing_context.h to reflect its content.

rojkov · 2021-04-23T09:18:51Z

source/common/http/header_map_impl.h

  HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key,
                                     HeaderString&& value);
-  HeaderMap::NonConstGetResult getExisting(const LowerCaseString& key);
+  HeaderMap::NonConstGetResult getExisting(const absl::string_view key);


Here we change the function's contract by adding a new implicit requirement for a caller that key must be lower case. Can we guarantee that or we don't care about casing now?

getExisting(const absl::string_view key) is a protected method that be used by get(const LowerCaseString& key) and getTracingContext(const absl::string_view key).

The former is the interface of HeaderMap and can always keep the case constraint. The latter is the interface of TracingContext, which is designed to not care about the case of the key.
When developers use TracingContext based on RequestHeaderMapImpl, they should ensure that the key meets the requirements of RequestHeaderMap (key must be lowercase).

@rojkov In the design of TracingContext, I hope it is completely protocol-independent, so a simple string_view is used as the key type.
But in actual use, the combination of TracingContext and HeaderMap is not perfect.
In getTracingContext, I must assume that the content of key is a lowercase string. In setTracingContext, I must create a LowCaseString copy of the key.
I am thinking about whether I can directly move LowCaseString to common, and then TracingContext directly uses LowCaseString as the key type.
But I am still struggling with it. Maybe the community can give some opinions. 🤔

I don't have much experience with non-HTTP tracers and don't know if it's practical to convert to LowerCaseString always :( Will defer to the code owners to decide.

rojkov · 2021-04-23T09:30:17Z

source/common/tracing/http_tracer_impl.h

+
 private:
-  DriverPtr driver_;
+  DriverSharedPtr driver_;


I see. Thanks!

Signed-off-by: wbpcode <comems@msn.com>

…al-tracing

…xt for case-insensitive key Signed-off-by: wbpcode <comems@msn.com>

Signed-off-by: wbpcode <comems@msn.com>

jmarantz · 2021-04-29T12:33:50Z

include/envoy/common/lower_case_string.h

+// 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


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?

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.

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.

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

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?

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

Signed-off-by: wbpcode <comems@msn.com>

lizan · 2021-04-29T18:02:50Z

source/common/http/header_map_impl.h

+  absl::optional<absl::string_view> getTraceContext(const LowerCaseString& key) const override {
+    auto result = get(key);
+    if (!result.empty()) {
+      return result[0]->value().getStringView();
+    }
+    return absl::nullopt;
+  }
+
+  void setTraceContext(const LowerCaseString& key, absl::string_view value) override {
+    setReferenceKey(key, value);
+  }


We might want to just add an implicit conversion from LowerCaseString to string_view, so you don't need those overloads? Though we might want setTraceContext by reference or copy versions.

@lizan These overloads are designed to ensure that general tracing does not impose additional performance overhead on existing http tracer users. For example, the overhead of verifying that string_view is a legitimate lower case string.
Without this overload, there is only getTraceContext(absl::string_view key), so when RequestHeaderMapImpl implements this interface, we always have to make sure that the content of the key is a lower case string.

I agree with the sentiment that this PR needs to be split up. So if necessary, this can be continued in a new PR.

lizan

In general, this PR involves many refactoring that can be split out to other PR so it would be easier to review.

wbpcode · 2021-04-30T02:57:00Z

In general, this PR involves many refactoring that can be split out to other PR so it would be easier to review.

@lizan @mattklein123

This PR could be split out to four different PRs:

Remove the trace driver's dependency on HttpTracerImpl; remove trace drivers' dependency on HttpTracerImpl #16244
move Http::LowerCaseString to common for common use; (move LowerCaseString to common for common use #16539 May be we don't need this.)
add TraceContext interface and implement it by RequestHeaderMapImpl; general tracing context interface #16793
replace RequestHeaderMap in tracers by TraceContext.

The first PR and fourth PR will involve a large number of file updates, but no logical changes.

This PR is part of #16049 to support general tracing. Please check #16049 get more details. Commit Message: remove trace drivers' dependency on HttpTracerImpl Additional Description: Now all tracers (zipkin, skywalking, etc.) will depend on HttpTracerImpl, making it difficult for Tracers to be reused by other protocols (Dubbo, Thrift, etc.). The purpose of this PR is to change this dependency. Risk Level: Low (No new logic). Testing: Add. Docs Changes: N/A Release Notes: N/A Signed-off-by: wbpcode <comems@msn.com>

github-actions · 2021-05-30T16:15:09Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

wbpcode · 2021-06-01T01:36:26Z

/no-stale

Signed-off-by: wbpcode <comems@msn.com> Sub-PR of #16049. Check #16049 get more background information. This PR designed a new generic abstraction `TraceContext` to replace `Http::RequestHeaderMap` to provide tracing context to the tracer driver. `Http::RequestHeaderMapImpl` already inherits from TraceContext and implements the relevant interfaces. Next, we just need simply replace `Http::RequestHeaderMap` with `TraceContext` in all tracer drivers implementations. After that, the main body of the entire general tracing system is completed. I'm not sure, whether the replacement step requires a separate PR. Because it does not involve any new logic, but there will be a lot of file changes. Risk Level: Low. Testing: Added. Docs Changes: N/A Release Notes: N/A Signed-off-by: wbpcode <comems@msn.com>

Signed-off-by: wbpcode <comems@msn.com> Sub-PR of envoyproxy#16049. Check envoyproxy#16049 get more background information. This PR designed a new generic abstraction `TraceContext` to replace `Http::RequestHeaderMap` to provide tracing context to the tracer driver. `Http::RequestHeaderMapImpl` already inherits from TraceContext and implements the relevant interfaces. Next, we just need simply replace `Http::RequestHeaderMap` with `TraceContext` in all tracer drivers implementations. After that, the main body of the entire general tracing system is completed. I'm not sure, whether the replacement step requires a separate PR. Because it does not involve any new logic, but there will be a lot of file changes. Risk Level: Low. Testing: Added. Docs Changes: N/A Release Notes: N/A Signed-off-by: wbpcode <comems@msn.com> Signed-off-by: chris.xin <xinchuantao@qq.com>

Sub-PR of #16049. Check #16049 get more background information. This PR is last sub-PR of #16049. It simply replaces `Http::RequestHeaderMap` with general `Tracing::TraceContext` in all tracer drivers implementations. Check #16793 get more info about `Tracing::TraceContext`. After this PR, the main body of the entire general tracing system is completed. Next, we can try to use the new general tracing system in dubbo and thrift. Commit Message: replace RequestHeaderMap in tracers with general TraceContext Risk Level: Low. Testing: N/A. Docs Changes: N/A. Release Notes: N/A. Signed-off-by: wbpcode <comems@msn.com>

wbpcode · 2021-07-08T06:00:36Z

Check #16244 #16793 #17212
The main body of general tracing has been completed. At present, the implementation of general tracing still has some rough points, and some small PRs will be used to improve it in the future. Things to do next:

Improve the TraceContext API and code quality.
Extend dubbo/thrift proxy to verify the actual effect of general tracing.

**Finally, thank you @lizan for your patience and help. **

Signed-off-by: wbpcode <comems@msn.com> Sub-PR of envoyproxy#16049. Check envoyproxy#16049 get more background information. This PR designed a new generic abstraction `TraceContext` to replace `Http::RequestHeaderMap` to provide tracing context to the tracer driver. `Http::RequestHeaderMapImpl` already inherits from TraceContext and implements the relevant interfaces. Next, we just need simply replace `Http::RequestHeaderMap` with `TraceContext` in all tracer drivers implementations. After that, the main body of the entire general tracing system is completed. I'm not sure, whether the replacement step requires a separate PR. Because it does not involve any new logic, but there will be a lot of file changes. Risk Level: Low. Testing: Added. Docs Changes: N/A Release Notes: N/A Signed-off-by: wbpcode <comems@msn.com>

…roxy#17212) Sub-PR of envoyproxy#16049. Check envoyproxy#16049 get more background information. This PR is last sub-PR of envoyproxy#16049. It simply replaces `Http::RequestHeaderMap` with general `Tracing::TraceContext` in all tracer drivers implementations. Check envoyproxy#16793 get more info about `Tracing::TraceContext`. After this PR, the main body of the entire general tracing system is completed. Next, we can try to use the new general tracing system in dubbo and thrift. Commit Message: replace RequestHeaderMap in tracers with general TraceContext Risk Level: Low. Testing: N/A. Docs Changes: N/A. Release Notes: N/A. Signed-off-by: wbpcode <comems@msn.com>

wbpcode requested review from Shikugawa, alyssawilk, dio, lizan and mattklein123 as code owners April 18, 2021 05:53

repokitteh-read-only bot added the api label Apr 18, 2021

repokitteh-read-only bot assigned lizan Apr 18, 2021

wbpcode marked this pull request as draft April 18, 2021 06:02

first commit

d78258e

Signed-off-by: wbpcode <comems@msn.com>

wbpcode force-pushed the general-tracing branch from e328a9f to d78258e Compare April 19, 2021 09:38

wbpcode added 5 commits April 19, 2021 18:43

fix format

520cdc9

Signed-off-by: wbpcode <comems@msn.com>

fix proto format

43cc47e

Signed-off-by: wbpcode <comems@msn.com>

fix http connection manager test

8740d85

Signed-off-by: wbpcode <comems@msn.com>

clang-tidy

38a2e80

Signed-off-by: wbpcode <comems@msn.com>

add more test

53d0c58

Signed-off-by: wbpcode <comems@msn.com>

mattklein123 self-assigned this Apr 21, 2021

wbpcode changed the title ~~[WIP] tracing: general tracing support~~ tracing: general tracing support Apr 21, 2021

wbpcode marked this pull request as ready for review April 21, 2021 07:29

Merge branch 'main' of https://github.com/envoyproxy/envoy into gener…

b259a2a

…al-tracing

rojkov reviewed Apr 22, 2021

View reviewed changes

asraa assigned rojkov Apr 22, 2021

rojkov reviewed Apr 23, 2021

View reviewed changes

wbpcode added 3 commits April 26, 2021 13:51

fix some name etc.

bde8346

Signed-off-by: wbpcode <comems@msn.com>

rename TracingContext to TraceContext

f7d9ce9

Signed-off-by: wbpcode <comems@msn.com>

Merge branch 'main' of https://github.com/envoyproxy/envoy into gener…

dc57c3c

…al-tracing

wbpcode dismissed rojkov’s stale review via 38aba26 April 27, 2021 03:09

wbpcode added 4 commits April 28, 2021 15:12

Merge branch 'main' of https://github.com/envoyproxy/envoy into gener…

e8427f0

…al-tracing

Merge branch 'main' of https://github.com/envoyproxy/envoy into gener…

f535c8e

…al-tracing

move lower case string to common and add new interface to trace conte…

5f0c81a

…xt for case-insensitive key Signed-off-by: wbpcode <comems@msn.com>

update context key

d4d6910

Signed-off-by: wbpcode <comems@msn.com>

jmarantz reviewed Apr 29, 2021

View reviewed changes

wbpcode added 2 commits April 29, 2021 20:42

fix format

6341150

Signed-off-by: wbpcode <comems@msn.com>

make lower case string validator pluggable

1c5d1e0

Signed-off-by: wbpcode <comems@msn.com>

lizan reviewed Apr 29, 2021

View reviewed changes

lizan suggested changes Apr 29, 2021

View reviewed changes

wbpcode mentioned this pull request Apr 30, 2021

remove trace drivers' dependency on HttpTracerImpl #16244

Merged

wbpcode mentioned this pull request May 18, 2021

move LowerCaseString to common for common use #16539

Closed

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label May 30, 2021

github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Jun 1, 2021

mattklein123 removed their assignment Jun 1, 2021

wbpcode mentioned this pull request Jun 3, 2021

general tracing context interface #16793

Merged

alyssawilk added the waiting label Jun 8, 2021

wbpcode mentioned this pull request Jul 1, 2021

replace RequestHeaderMap in tracers with general TraceContext #17212

Merged

wbpcode closed this Jul 8, 2021

wbpcode mentioned this pull request Aug 5, 2021

dubbo tracing support #17601

Closed

Conversation

wbpcode commented Apr 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Apr 18, 2021

Uh oh!

wbpcode commented Apr 18, 2021

Uh oh!

wbpcode commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lizan commented Apr 21, 2021

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Apr 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

wbpcode commented Apr 18, 2021 •

edited

Loading

wbpcode commented Apr 21, 2021 •

edited

Loading

wbpcode Apr 23, 2021 •

edited

Loading

wbpcode Apr 26, 2021 •

edited

Loading

wbpcode Apr 30, 2021 •

edited

Loading

wbpcode commented Apr 30, 2021 •

edited

Loading