Skip to content

added tracing capability to ext-authz http client#8142

Merged
zuercher merged 8 commits intoenvoyproxy:masterfrom
gsagula:ext_authz-http-tracing
Sep 17, 2019
Merged

added tracing capability to ext-authz http client#8142
zuercher merged 8 commits intoenvoyproxy:masterfrom
gsagula:ext_authz-http-tracing

Conversation

@gsagula
Copy link
Copy Markdown
Member

@gsagula gsagula commented Sep 4, 2019

Description:
This PR adds tracing support to the HTTP client in the external authorization filter. So far, only gRPC client was able to trace requests from the filter to an authorization service.

Risk Level: Low
Testing: yes
Docs Changes: N/A
Release Notes: Added
Fixes #6520

Signed-off-by: Gabriel Linden Sagula gsagula@gmail.com

added tracing capability to ext-authz http client
ca5cadc 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
@gsagula
Copy link
Copy Markdown
Member Author

gsagula commented Sep 4, 2019 •
edited
Loading

@dio Sorry for the delay on this one. I think I addressed all your comments from #7267. Thanks!

@dio
Copy link
Copy Markdown
Member

dio commented Sep 4, 2019

Thanks. will take a look.

@dio dio self-assigned this Sep 4, 2019
@dio dio mentioned this pull request Sep 4, 2019
Closed
@gsagula gsagula mentioned this pull request Sep 4, 2019
Merged
changed set tag to happen before the member function returns 5xx
d0d8ab0 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
dio
dio previously approved these changes Sep 5, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Thanks @gsagula! I think the improvement on showing service unavailable for 503 is great (as mentioned in #7267 (review))!

@dio dio dismissed their stale review September 5, 2019 10:47

Sorry, we missed the version_history changes here?

dio
dio suggested changes Sep 5, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

I think yes, we need an entry in version history. Thanks!

updated version history
5614ca3 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
@gsagula
Copy link
Copy Markdown
Member Author

gsagula commented Sep 5, 2019

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Sep 5, 2019

🔨 rebuilding ci/circleci: release (failed build)

🐱

Caused by: a #8142 (comment) was created by @gsagula.

see: more, trace.

Merge remote-tracking branch 'upstream/master' into ext_authz-http-tr…
065826a 
…acing

Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
@gsagula
Copy link
Copy Markdown
Member Author

gsagula commented Sep 5, 2019

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Sep 5, 2019

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #8142 (comment) was created by @gsagula.

see: more, trace.

@dio
Copy link
Copy Markdown
Member

dio commented Sep 5, 2019

/azp run envoy-linux

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Sep 5, 2019

Azure Pipelines successfully started running 1 pipeline(s).

dio
dio previously approved these changes Sep 5, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Thanks!

zuercher
zuercher reviewed Sep 6, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Looks good. For consistency, I think we should handle cleaning up the span_ in the same way as callbacks_.

@zuercher zuercher self-assigned this Sep 6, 2019
Gabriel Linden Sagula added 2 commits September 6, 2019 22:34
cleaned up the pointer to span
bb9bf66 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
Merge remote-tracking branch 'upstream/master' into ext_authz-http-tr…
8f890bf 
…acing

Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
@gsagula
Copy link
Copy Markdown
Member Author

gsagula commented Sep 7, 2019

@zuercher @dio Thank you for reviewing it.

@gsagula
Copy link
Copy Markdown
Member Author

gsagula commented Sep 7, 2019

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Sep 7, 2019

🔨 rebuilding ci/circleci: coverage (failed build)
🔨 rebuilding ci/circleci: asan (failed build)
🔨 rebuilding ci/circleci: release (failed build)
🔨 rebuilding ci/circleci: clang_tidy (failed build)

🐱

Caused by: a #8142 (comment) was created by @gsagula.

see: more, trace.

Merge remote-tracking branch 'upstream/master' into ext_authz-http-tr…
602d32f 
…acing

Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
@gsagula
Copy link
Copy Markdown
Member Author

gsagula commented Sep 8, 2019

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Sep 8, 2019

🔨 rebuilding ci/circleci: asan (failed build)

🐱

Caused by: a #8142 (comment) was created by @gsagula.

see: more, trace.

zuercher
zuercher previously approved these changes Sep 9, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Sep 10, 2019

Needs master merge.

merged master and added tracing to the no cluster error
e499abf 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
@gsagula
Copy link
Copy Markdown
Member Author

gsagula commented Sep 10, 2019

Done.

@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Sep 17, 2019

@zuercher I think this just needs a re-approve but I haven't been tracking carefully.

zuercher
zuercher approved these changes Sep 17, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Looks good. My apologies for losing track of this one.

@zuercher zuercher merged commit 2f5f947 into envoyproxy:master Sep 17, 2019
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Sep 24, 2019
@danzh1989
added tracing capability to ext-authz http client (envoyproxy#8142)
8b5a9cf 
This PR adds tracing support to the HTTP client in the external authorization filter. So far, only gRPC client was able to trace requests from the filter to an authorization service.

Risk Level: Low
Testing: yes
Docs Changes: N/A
Release Notes: Added
Fixes envoyproxy#6520

Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019
added tracing capability to ext-authz http client (envoyproxy#8142)
c0405f8 
This PR adds tracing support to the HTTP client in the external authorization filter. So far, only gRPC client was able to trace requests from the filter to an authorization service.

Risk Level: Low
Testing: yes
Docs Changes: N/A
Release Notes: Added
Fixes envoyproxy#6520

Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019
added tracing capability to ext-authz http client (envoyproxy#8142)
12996ef 
This PR adds tracing support to the HTTP client in the external authorization filter. So far, only gRPC client was able to trace requests from the filter to an authorization service.

Risk Level: Low
Testing: yes
Docs Changes: N/A
Release Notes: Added
Fixes envoyproxy#6520

Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zuercher zuercher zuercher approved these changes

+1 more reviewer

@dio dio dio left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@zuercher zuercher

@dio dio

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

B3 HTTP headers not propagated to ext_authz server in 1.10.0

4 participants

@gsagula @dio @mattklein123 @zuercher