Skip to content

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

Merged
mattklein123 merged 16 commits intoenvoyproxy:masterfrom
wozz:opencensus_grpc_service
Feb 17, 2020
Merged

tracing: google grpc service support for ocagent/opencensus#9955
mattklein123 merged 16 commits intoenvoyproxy:masterfrom
wozz:opencensus_grpc_service

Conversation

@wozz
Copy link
Copy Markdown
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
Copy Markdown

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

🐱

Caused by: #9955 was opened by wozz.

see: more, trace.

@wozz wozz force-pushed the opencensus_grpc_service branch from 5ab34ef to 8d2c041 Compare February 7, 2020 00:29
Signed-off-by: michael.wozniak <[email protected]>
Signed-off-by: michael.wozniak <[email protected]>
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.

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

@wozz wozz requested a review from lizan February 11, 2020 17:08
@dio
Copy link
Copy Markdown
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
Copy Markdown
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
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 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
Copy Markdown
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
Copy Markdown
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