router: add more upstream request metadata to tracing spans by nareddyt · Pull Request #8289 · envoyproxy/envoy

nareddyt · 2019-09-18T22:36:55Z

Description: Adds more metadata to spans created by the router for upstream requests. Additional metadata added includes response codes, reset reasons, retries, etc.

Risk Level: Low

Testing: Modified existing tests to check for this span metadata. Added new tests for all branches.

Docs Changes: None

Release Notes: None. Please let me know if this change should have release notes.

nareddyt · 2019-09-18T22:51:28Z

Hi @lizan, I'm having trouble with check_format.py. I ran the following command and was able to push (all the commit hooks passed, including format check) but CircleCI is complaining.

./tools/check_format.py fix && ./tools/format_python_tools.sh fix

nareddyt · 2019-09-18T23:56:11Z

I don't understand this build error either. CLion did not complain. I tried looking at my files but I don't see anything obvious:

d.lld: error: undefined symbol: Envoy::Grpc::Common::responseToGrpcStatus(Envoy::StreamInfo::StreamInfo const&, Envoy::Http::HeaderMap const&, Envoy::Http::HeaderMap const&)
>>> referenced by router.cc
>>>               bazel-out/k8-fastbuild/bin/source/common/router/_objs/router_lib/router.pic.o:(Envoy::Router::Filter::UpstreamRequest::~UpstreamRequest())

ld.lld: error: undefined symbol: Envoy::Grpc::Common::responseToGrpcStatus(Envoy::StreamInfo::StreamInfo const&, Envoy::Http::HeaderMap const&, Envoy::Http::HeaderMap const&)
>>> referenced by access_log_impl.cc
>>>               bazel-out/k8-fastbuild/bin/source/common/access_log/_objs/access_log_lib/access_log_impl.pic.o:(Envoy::AccessLog::GrpcStatusFilter::evaluate(Envoy::StreamInfo::StreamInfo const&, Envoy::Http::HeaderMap const&, Envoy::Http::HeaderMap const&, Envoy::Http::HeaderMap const&))
clang: error: linker command failed with exit code 1 (use -v to see invocation)

source/common/router/router.cc

nareddyt · 2019-09-19T17:49:00Z

Was able to fix format issues by running the formatter via the docker image and then pushing with --no-verify.

./ci/run_envoy_docker.sh './ci/do_ci.sh fix_format'

nareddyt · 2019-09-19T21:45:24Z

/retest

repokitteh-read-only · 2019-09-19T21:45:26Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #8289 (comment) was created by @nareddyt.

see: more, trace.

source/common/access_log/access_log_impl.cc

nareddyt · 2019-09-19T23:34:33Z

/retest

repokitteh-read-only · 2019-09-19T23:34:36Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #8289 (comment) was created by @nareddyt.

see: more, trace.

mattklein123

Generally LGTM with some small comments.

I'm a bit concerned in general about the overall perf of tracing and whether these extra tags should be added in all cases or potentially opt-in. @lizan @jmarantz any thoughts?

Also, you will need full coverage of all of the extra tags you added. Thank you!

/wait

source/common/access_log/access_log_impl.cc

source/common/router/router.cc

nareddyt · 2019-09-20T17:27:23Z

I actually found a static function that adds all these tags and takes care of nullptr values for the response headers and trailers. I think I should refactor my changes to just use this function...

https://github.com/envoyproxy/envoy/blob/master/source/common/tracing/http_tracer_impl.cc#L141

objectiser · 2019-09-20T18:26:39Z

@nareddyt +1 sounds like a good idea - sorry I had forgotten about the tracing_config.verbose(), which serves the purpose of conditionally adding more detailed information.

nareddyt · 2019-09-20T19:58:14Z

Can you take a look at the logic now? Specifically for source/common/tracing/http_tracer_impl.cc. I did not understand why finalizeSpan was structured the way it was with all the if (request_headers), so I just made another method and extracted the common tags out.

I still need to add tests. Will add if this structure looks good.

source/common/tracing/http_tracer_impl.h

source/common/router/router.cc

source/common/tracing/http_tracer_impl.h

nareddyt · 2019-09-25T01:07:34Z

Working on adding tests, will let you know when ready :)

mattklein123

Thanks, this looks really good. Nice consolidation and cleanup. One test request.

/wait

source/common/router/router.cc

Signed-off-by: Teju Nareddy <nareddyt@google.com>

nareddyt · 2019-09-26T02:54:45Z

Ready for review! Sorry about the force push, I was trying to rebase with master

nareddyt · 2019-09-26T05:46:39Z

/retest

repokitteh-read-only · 2019-09-26T05:46:42Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #8289 (comment) was created by @nareddyt.

see: more, trace.

test/common/router/router_test.cc

Signed-off-by: Teju Nareddy <nareddyt@google.com>

mattklein123

Awesome, thanks!

objectiser

Thanks!

…xy#8289) Signed-off-by: Teju Nareddy <nareddyt@google.com>

nareddyt requested a review from lizan as a code owner September 18, 2019 22:40

nareddyt force-pushed the router-spans-tags branch from 0aa3d98 to 5dd146b Compare September 18, 2019 22:50

mattklein123 self-assigned this Sep 19, 2019

objectiser reviewed Sep 19, 2019

View reviewed changes

source/common/router/router.cc Outdated Show resolved Hide resolved

source/common/router/router.cc Outdated Show resolved Hide resolved

lizan reviewed Sep 19, 2019

View reviewed changes

source/common/access_log/access_log_impl.cc Outdated Show resolved Hide resolved

mattklein123 requested changes Sep 19, 2019

View reviewed changes

source/common/access_log/access_log_impl.cc Outdated Show resolved Hide resolved

source/common/router/router.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Sep 19, 2019

jmarantz reviewed Sep 20, 2019

View reviewed changes

source/common/router/router.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot removed the waiting label Sep 20, 2019

nareddyt commented Sep 20, 2019

View reviewed changes

source/common/tracing/http_tracer_impl.h Outdated Show resolved Hide resolved

mattklein123 added the waiting label Sep 20, 2019

objectiser reviewed Sep 24, 2019

View reviewed changes

source/common/router/router.cc Outdated Show resolved Hide resolved

source/common/tracing/http_tracer_impl.h Outdated Show resolved Hide resolved

repokitteh-read-only bot removed the waiting label Sep 24, 2019

mattklein123 requested changes Sep 25, 2019

View reviewed changes

source/common/router/router.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Sep 25, 2019

nareddyt added 4 commits September 25, 2019 18:28

router: add more metadata about upstream request to tracing spans

ad4137a

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Fix format

7c5bf4b

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Fix build files

5d85d32

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Fix build files format

40b3a14

Signed-off-by: Teju Nareddy <nareddyt@google.com>

nareddyt added 10 commits September 25, 2019 18:28

Review comments

6ad13d8

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Fix build errors

b4f9d7d

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Fix tests

9822727

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Resolve comments

6e123a8

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Re-use existing function

bda69d0

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Revert un-needed changes

22f2ea0

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Reuse existing function, fix tests

16e6f4e

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Fix format

d85ec82

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Review comments

2439ed0

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Add test for reset and cancellation cases

107bdbb

Signed-off-by: Teju Nareddy <nareddyt@google.com>

nareddyt force-pushed the router-spans-tags branch from f912fee to 107bdbb Compare September 26, 2019 02:39

repokitteh-read-only bot removed the waiting label Sep 26, 2019

nareddyt requested review from mattklein123 and objectiser September 26, 2019 02:54

objectiser reviewed Sep 26, 2019

View reviewed changes

test/common/router/router_test.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Sep 26, 2019

Add test for retry due to reset

c86672e

Signed-off-by: Teju Nareddy <nareddyt@google.com>

repokitteh-read-only bot removed the waiting label Sep 26, 2019

nareddyt requested a review from objectiser September 26, 2019 20:03

mattklein123 added the waiting label Sep 26, 2019

Add test for cancellation

aa38056

Signed-off-by: Teju Nareddy <nareddyt@google.com>

repokitteh-read-only bot removed the waiting label Sep 26, 2019

mattklein123 approved these changes Sep 26, 2019

View reviewed changes

objectiser approved these changes Sep 27, 2019

View reviewed changes

mattklein123 merged commit 4606bc1 into envoyproxy:master Sep 27, 2019

danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019

router: add more upstream request metadata to tracing spans (envoypro…

2b63e91

…xy#8289) Signed-off-by: Teju Nareddy <nareddyt@google.com>

matthewgrossman mentioned this pull request Apr 14, 2022

tracing: Move custom-tag application after common tags #20645

Merged

Conversation

nareddyt commented Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nareddyt commented Sep 18, 2019

Uh oh!

nareddyt commented Sep 18, 2019

Uh oh!

Uh oh!

Uh oh!

nareddyt commented Sep 19, 2019

Uh oh!

nareddyt commented Sep 19, 2019

Uh oh!

repokitteh-read-only bot commented Sep 19, 2019

Uh oh!

Uh oh!

nareddyt commented Sep 19, 2019

Uh oh!

repokitteh-read-only bot commented Sep 19, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nareddyt commented Sep 20, 2019

Uh oh!

objectiser commented Sep 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nareddyt commented Sep 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nareddyt commented Sep 25, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nareddyt commented Sep 26, 2019

Uh oh!

nareddyt commented Sep 26, 2019

Uh oh!

repokitteh-read-only bot commented Sep 26, 2019

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

objectiser left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nareddyt commented Sep 18, 2019 •

edited

Loading

objectiser commented Sep 20, 2019 •

edited

Loading

nareddyt commented Sep 20, 2019 •

edited

Loading