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

tracing: google grpc service support for ocagent/opencensus #9955

Merged
merged 16 commits into from
Feb 17, 2020

Conversation

wozz
Copy link
Contributor

@wozz wozz commented Feb 7, 2020

Description: support a grpc service for opencensus tracing ocagent backend. this allows more fine grained control over the grpc connection to the ocagent and exposes more settings for the connection.
Risk Level: low
Testing: there don't appear to be tests around this part of the code at the moment. would be open to suggestions about how to add tests for this.
Docs Changes: updated docs for new proto field
Release Notes: added note to existing release note

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9955 was opened by wozz.

see: more, trace.

Signed-off-by: michael.wozniak <[email protected]>
Signed-off-by: michael.wozniak <[email protected]>
Copy link
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.

Flushing a high-level comment. Seems like you need to add a test as well?

api/envoy/config/trace/v2/trace.proto Outdated Show resolved Hide resolved
@wozz wozz requested a review from lizan February 11, 2020 17:08
@dio
Copy link
Member

dio commented Feb 16, 2020

Looks good. Should we have a test for this?

Signed-off-by: michael.wozniak <[email protected]>
@wozz
Copy link
Contributor Author

wozz commented Feb 16, 2020

I added a test for the new config option, however I'm not sure if you mean to set up a mock service and actually connect to it. I don't see tests currently that do that for the existing exporters, so I'm not sure how I would do that.

Copy link
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 it is good. Thanks! I’ll let @lizan to do the API changes approval. Thanks again!

Signed-off-by: michael.wozniak <[email protected]>
Signed-off-by: michael.wozniak <[email protected]>
Signed-off-by: michael.wozniak <[email protected]>
@wozz
Copy link
Contributor Author

wozz commented Feb 16, 2020

the test failure doesn't seem to be related to these changes as far as I can tell

Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

API LGTM

Signed-off-by: michael.wozniak <[email protected]>
@mattklein123 mattklein123 merged commit 4423dcd into envoyproxy:master Feb 17, 2020
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.

5 participants