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

Make opencensus Stackdriver exporter respects initial_metadata option #11831

Merged
merged 8 commits into from
Jul 17, 2020

Conversation

bianpengyuan
Copy link
Contributor

@bianpengyuan bianpengyuan commented Jul 1, 2020

Currently Opencensus tracer uses grpcService proto to configure its tracing client stub. It uses GoogleGrpcUtil to construct a channel and create client stub with that. However, the channel created there does not take care of initial_metadata from grpcService configure. This change fills in OCprepare_client_context option in export to make it respect initial metadata.

Signed-off-by: Pengyuan Bian [email protected]

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thanks! Could you please add unit tests to cover this and for skipping the interceptor due to no initial metadata configured first?

@bianpengyuan
Copy link
Contributor Author

@asraa I am having a hard time to find an easy way to test this. All current gRPC client tests are based on Envoy's async client/stream interface and implementation, but for this change we'd just want the test to verify the channel created could have initial metadata injected. Current options that I've thought about:

  1. create a GoogleAsyncClientImpl with the initial metadata interceptor enabled channel, but leave initial_metadata in the configuration param, and verify the stream could still have initial metadata injected.
  2. Enable mock interface generation for Greeter server and test the generated channel with mock grpc client and server.

Neither seems ideal to me.. any better ideas?

@asraa asraa assigned htuch and unassigned asraa Jul 8, 2020
@asraa
Copy link
Contributor

asraa commented Jul 8, 2020

re-assigning to @htuch,who may be able to help a little more with testing.
I think maybe we should reduce the scope down to testing the written interceptor, but I'm uncertain how to do that right now.

@htuch
Copy link
Member

htuch commented Jul 9, 2020

@bianpengyuan a couple of things:

  1. Can you explain in the commit message a bit more on the "what" vs. the motivation. I.e. we had some initial metadata plumbing before, what is the interceptor doing that provides value here? Ideally include links back to the Google gRPC docs of interest and how this would apply to end users.
  2. We need some tests as discussed above. I think a good candidate place to put these would be https://github.com/envoyproxy/envoy/blob/master/test/common/grpc/google_async_client_impl_test.cc.

@bianpengyuan
Copy link
Contributor Author

@htuch I updated the PR description. Yeah I have read through related tests. They are all based on AsyncClient / AsyncStream implementation and I don't find an easy way to apply them to this use case, which leads me to two options listed in #11831 (comment).

@htuch
Copy link
Member

htuch commented Jul 10, 2020

@bianpengyuan looking at the description, this makes me think that we're doing something pretty weird in OpenCensus. We already have another thread about OpenCensus doing its own thing with HTTP and curl (#11816), this is another place that it would be better for OpenCensus to provide a generic gRPC interface rather than relying on using Google gRPC libraries directly; when you do this you basically break Envoy's siloed threading model and lose observability via Envoy's normal mechanisms.

^^ is not actionable in this PR, but it would be good to make some progress here asynchronously, CC @g-easy @markdroth

For tests, I think you're going to need to do an integration test with OpenCensus and gRPC tracing enabled with a fake upstream (it can be Envoy's inbuilt FakeUpstream) that can validate the initial metadata received.

Signed-off-by: Pengyuan Bian <[email protected]>
Signed-off-by: Pengyuan Bian <[email protected]>
@bianpengyuan bianpengyuan changed the title Add initial metadata interceptor into google gRPC channel creation Make opencensus Stackdriver exporter respects initial_metadata option Jul 12, 2020
@bianpengyuan
Copy link
Contributor Author

@htuch find an easier way to achieve this with the lastest opencensus. ptal, thanks!

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@htuch htuch merged commit bb53b8a into envoyproxy:master Jul 17, 2020
bianpengyuan added a commit to bianpengyuan/envoy that referenced this pull request Jul 18, 2020
…envoyproxy#11831)

Currently Opencensus tracer uses grpcService proto to configure its tracing client stub. It uses GoogleGrpcUtil to construct a channel and create client stub with that. However, the channel created there does not take care of initial_metadata from grpcService configure. This change fills in OCprepare_client_context option in export to make it respect initial metadata.

Risk level: Low

Signed-off-by: Pengyuan Bian <[email protected]>
istio-testing pushed a commit to istio/envoy that referenced this pull request Jul 22, 2020
…envoyproxy#11831) (#234)

Currently Opencensus tracer uses grpcService proto to configure its tracing client stub. It uses GoogleGrpcUtil to construct a channel and create client stub with that. However, the channel created there does not take care of initial_metadata from grpcService configure. This change fills in OCprepare_client_context option in export to make it respect initial metadata.

Risk level: Low

Signed-off-by: Pengyuan Bian <[email protected]>
KBaichoo pushed a commit to KBaichoo/envoy that referenced this pull request Jul 30, 2020
…envoyproxy#11831)

Currently Opencensus tracer uses grpcService proto to configure its tracing client stub. It uses GoogleGrpcUtil to construct a channel and create client stub with that. However, the channel created there does not take care of initial_metadata from grpcService configure. This change fills in OCprepare_client_context option in export to make it respect initial metadata.

Risk level: Low

Signed-off-by: Pengyuan Bian <[email protected]>
Signed-off-by: Kevin Baichoo <[email protected]>
scheler pushed a commit to scheler/envoy that referenced this pull request Aug 4, 2020
…envoyproxy#11831)

Currently Opencensus tracer uses grpcService proto to configure its tracing client stub. It uses GoogleGrpcUtil to construct a channel and create client stub with that. However, the channel created there does not take care of initial_metadata from grpcService configure. This change fills in OCprepare_client_context option in export to make it respect initial metadata.

Risk level: Low

Signed-off-by: Pengyuan Bian <[email protected]>
Signed-off-by: scheler <[email protected]>
bianpengyuan added a commit to bianpengyuan/envoy that referenced this pull request Aug 24, 2020
…envoyproxy#11831)

Currently Opencensus tracer uses grpcService proto to configure its tracing client stub. It uses GoogleGrpcUtil to construct a channel and create client stub with that. However, the channel created there does not take care of initial_metadata from grpcService configure. This change fills in OCprepare_client_context option in export to make it respect initial metadata.

Risk level: Low

Signed-off-by: Pengyuan Bian <[email protected]>
istio-testing pushed a commit to istio/envoy that referenced this pull request Aug 24, 2020
…envoyproxy#11831) (#261)

Currently Opencensus tracer uses grpcService proto to configure its tracing client stub. It uses GoogleGrpcUtil to construct a channel and create client stub with that. However, the channel created there does not take care of initial_metadata from grpcService configure. This change fills in OCprepare_client_context option in export to make it respect initial metadata.

Risk level: Low

Signed-off-by: Pengyuan Bian <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants