Expose span context to filters by rshriram · Pull Request #894 · envoyproxy/envoy

rshriram · 2017-05-04T03:26:49Z

fixes #890

cc @goaway (I hope this doesn't conflict with what you are working on).
cc @fabolive @kyessenov

rshriram · 2017-05-04T03:57:41Z

@mattklein123 / @htuch . I am not sure if you want elaborate tests for this PR. If you do, any thoughts on where I could plug in a test for the function introduced in this PR, given that there are no consumers in the Envoy code base? [The filter we are using in Istio will be using this feature.]

mattklein123 · 2017-05-04T04:07:25Z

@rshriram we do need to talk about this. This is related to #888 and some other work that @goaway is doing. I'm in an all day meeting tomorrow and will be busy most of the day Friday also but perhaps you and @goaway can chat offline/VC (please find each other on Gitter).

@goaway I think this is a very reasonable change and a use case that I had not previously considered. It makes me somewhat rethink what you are working on. What if the span interface was changed so that you could create a child span from it (and the driver details were hidden)? If this were done, it would also work for what you are doing. Furthermore, if we passed the span inside the tracing context that is passed to gRPC AsyncClient calls, it could be fed into AsyncClient also fairly easily.

@rshriram I don't want to hold you up, and I think this change is reasonable, but I do want to consider the implications of this interface change in the face of other requirements that we are working on in parallel, so let's get a firm plan in place. @goaway should be able to drive.

rshriram · 2017-05-04T11:36:02Z

To be clear, our current use case is to modify the span that was already created, overwriting some metadata such as the calling service name (@fabolive would be introducing this interface into the base class as part of his second zipkin PR). I agree with you in general that it makes sense to unify the plumbing around creating child spans or modifying current span. I will sync with @goaway and spec out the plan.

On Thu, May 4, 2017 at 12:07 AM Matt Klein ***@***.***> wrote: @rshriram <https://github.com/rshriram> we do need to talk about this. This is related to #888 <#888> and some other work that @goaway <https://github.com/goaway> is doing. I'm in an all day meeting tomorrow and will be busy most of the day Friday also but perhaps you and @goaway <https://github.com/goaway> can chat offline/VC (please find each other on Gitter). @goaway <https://github.com/goaway> I think this is a very reasonable change and a use case that I had not previously considered. It makes me somewhat rethink what you are working on. What if the span interface was changed so that you could create a child span from it (and the driver details were hidden)? If this were done, it would also work for what you are doing. Furthermore, if we passed the span inside the tracing context that is passed to gRPC AsyncClient calls, it could be fed into AsyncClient also fairly easily. @rshriram <https://github.com/rshriram> I don't want to hold you up, and I think this change is reasonable, but I do want to consider the implications of this interface change in the face of other requirements that we are working on in parallel, so let's get a firm plan in place. @goaway <https://github.com/goaway> should be able to drive. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#894 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH0qd8HLmvXo5EbP_mD1c6Opsy00HRshks5r2U8AgaJpZM4NQMLp> .

-- ~shriram

mattklein123 · 2017-05-04T14:58:01Z

@rshriram yes I understand your use case and it makes sense, and my feeling is to go with this PR (since this is actually an approach I had originally discussed with @goaway), but I still want to make sure we understand how it fits in with the bigger picture.

goaway · 2017-05-04T18:25:22Z

source/common/http/async_client_impl.h

+
+    // Tracing::Span
+    void setTag(const std::string&, const std::string&) override { NOT_IMPLEMENTED; }
+    void finishSpan() override { NOT_IMPLEMENTED; }


I'd suggest moving NullSpan over into the Tracing namespace, and making these no-ops, rather than PANICs.

That way, a NullTracer and/or NullDriver can return a NullSpan instead of a nullptr, and this becomes somewhat safer to use.

goaway · 2017-05-04T18:28:02Z

Overall I think this is a pretty clean approach, and likely something I can hook into to allow the generation of child Spans off of an existing span. This would obviate the need for me to plumb
Tracers through everywhere: instead a function can be exposed on the Span interface to spawn a child, and the necessary internals can be embedded.

mattklein123 · 2017-05-04T18:44:34Z

@goaway +1, I think we should review and merge this PR, and build from there. I will add a few review comments later.

mattklein123 · 2017-05-04T22:21:41Z

@rshriram this does need a test. I would probably just add to here: https://github.com/lyft/envoy/blob/master/test/common/http/conn_manager_impl_test.cc#L241

If you actually EXPECT_CALL on the mock filter callbacks, from within a callback, you could verify that the span you get is what you expect. See other examples towards the end of the file.

…lters

ccaraman · 2017-05-06T00:02:10Z

include/envoy/http/filter.h

  virtual AccessLog::RequestInfo& requestInfo() PURE;

+  /**
+   * @return span context used for tracing purposes. Individual filters may add or modify


nit formatting: Have the word "information" start at the same space as "context" Ex:
https://github.com/lyft/envoy/blob/master/include/envoy/http/conn_pool.h#L58

ccaraman · 2017-05-06T00:05:37Z

test/mocks/http/mocks.h

  std::function<void()> reset_callback_;
  Event::MockDispatcher dispatcher_;
  testing::NiceMock<AccessLog::MockRequestInfo> request_info_;
+  std::shared_ptr<Tracing::MockSpan> active_span_;


nit: is this used in any of the tests?

mattklein123

LGTM modulo @ccaraman nits

…lters

rshriram · 2017-05-06T18:13:00Z

addressed the nits.

Patches need to be referenced using the workspace alias Signed-off-by: Alan Chiu <achiu@lyft.com> For an explanation of how to fill out the fields, please see the relevant section in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/master/PULL_REQUESTS.md) Description: bazel: fixup patches Risk Level: low Testing: ci Docs Changes: n/a Release Notes: n/a [Optional Fixes #Issue] [Optional Deprecated:] Signed-off-by: JP Simard <jp@jpsim.com>

Like _free_base, _free_dbg is called by CRT internal functions or operator delete in Debug mode. This closes envoyproxy#719 and closes envoyproxy#894. [alkondratenko@gmail.com: trivial formatting fixes] [alkondratenko@gmail.com: build free_dbg even in release builds]

**Description** This PR adds clearroutecache to dataplane flow. clearroutecache is an important functionality that envoy ai gw relies on. Signed-off-by: bitliu <bitliu@tencent.com>

Shriram Rajagopalan added 2 commits May 3, 2017 23:20

Expose span context to filters

e051acf

clang format fix

a5ad968

goaway reviewed May 4, 2017

View reviewed changes

Shriram Rajagopalan added 6 commits May 4, 2017 19:23

move NullSpan into Tracing namespace

2d87e05

test activeSpan interface

4c545ae

Merge remote-tracking branch 'upstream/master' into expose_span_to_fi…

17ffe9d

…lters

bug fix

22fe3bc

format fix

eb261d6

Merge remote-tracking branch 'upstream/master' into expose_span_to_fi…

7b7b388

…lters

ccaraman reviewed May 6, 2017

View reviewed changes

mattklein123 reviewed May 6, 2017

View reviewed changes

Shriram Rajagopalan added 2 commits May 6, 2017 01:22

nits

3b8dda1

Merge remote-tracking branch 'upstream/master' into expose_span_to_fi…

f59d15f

…lters

mattklein123 approved these changes May 6, 2017

View reviewed changes

mattklein123 merged commit 7c0d640 into envoyproxy:master May 6, 2017

rshriram deleted the expose_span_to_filters branch August 14, 2017 18:11

rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018

Add check.error_code and error_message (envoyproxy#894)

1518c68

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose span context to filters#894

Expose span context to filters#894
mattklein123 merged 10 commits intoenvoyproxy:masterfrom
amalgam8:expose_span_to_filters

rshriram commented May 4, 2017 •

edited

Loading

Uh oh!

rshriram commented May 4, 2017

Uh oh!

mattklein123 commented May 4, 2017

Uh oh!

rshriram commented May 4, 2017 via email

Uh oh!

mattklein123 commented May 4, 2017

Uh oh!

goaway May 4, 2017

Uh oh!

goaway commented May 4, 2017

Uh oh!

mattklein123 commented May 4, 2017

Uh oh!

mattklein123 commented May 4, 2017

Uh oh!

ccaraman May 6, 2017

Uh oh!

ccaraman May 6, 2017

Uh oh!

mattklein123 left a comment

Uh oh!

rshriram commented May 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

rshriram commented May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rshriram commented May 4, 2017

Uh oh!

mattklein123 commented May 4, 2017

Uh oh!

rshriram commented May 4, 2017 via email

Uh oh!

mattklein123 commented May 4, 2017

Uh oh!

goaway May 4, 2017

Choose a reason for hiding this comment

Uh oh!

goaway commented May 4, 2017

Uh oh!

mattklein123 commented May 4, 2017

Uh oh!

mattklein123 commented May 4, 2017

Uh oh!

ccaraman May 6, 2017

Choose a reason for hiding this comment

Uh oh!

ccaraman May 6, 2017

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

rshriram commented May 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rshriram commented May 4, 2017 •

edited

Loading