Skip to content

Add open census spans for ncproxy + go mod vendor#966

Merged
dcantah merged 1 commit intomicrosoft:masterfrom
dcantah:ncproxy-oc
Mar 13, 2021
Merged

Add open census spans for ncproxy + go mod vendor#966
dcantah merged 1 commit intomicrosoft:masterfrom
dcantah:ncproxy-oc

Conversation

@dcantah
Copy link
Contributor

@dcantah dcantah commented Mar 11, 2021

  • Give ncproxy its own etw provider
  • Add open census spans around all of the ncproxy calls
  • Go mod vendor + tidy to bring in go.opencensus.io/plugin and go.opencensus.io/stats

Signed-off-by: Daniel Canter dcanter@microsoft.com

@dcantah dcantah requested a review from a team as a code owner March 11, 2021 22:00
Copy link
Contributor

@anmaxvl anmaxvl left a comment

Choose a reason for hiding this comment

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

LGTM.

@kevpar
Copy link
Member

kevpar commented Mar 11, 2021

We should make sure we have the octtrpc and ocgrpc server and client pieces registered wherever we have GRPC or TTRPC servers or clients.

@dcantah
Copy link
Contributor Author

dcantah commented Mar 11, 2021

@kevpar I think I have the ttrpc ones handled, but I don't for grpc

@dcantah dcantah force-pushed the ncproxy-oc branch 2 times, most recently from 86eb619 to bb6e20f Compare March 12, 2021 00:23

opts := []grpc.DialOption{grpc.WithInsecure()}
// Register views to collect data.
if err := view.Register(ocgrpc.DefaultClientViews...); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this, don't use any stats stuff anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

KK wasn't sure. Just followed the example they gave

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait, do we just not want the views registered or the interceptor at all?

Copy link
Member

Choose a reason for hiding this comment

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

We need the GRPC handler registered, just not the view.Register thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dcantah dcantah force-pushed the ncproxy-oc branch 2 times, most recently from e50feb3 to eaa6acb Compare March 12, 2021 01:11
@dcantah dcantah changed the title Add open census spans for ncproxy Add open census spans for ncproxy + go mod vendor Mar 12, 2021
@dcantah dcantah changed the title Add open census spans for ncproxy + go mod vendor Add open census spans for ncproxy Mar 12, 2021
* Give ncproxy its own etw provider
* Add open census spans around all of the ncproxy calls
* Go mod vendor + tidy to bring in go.opencensus.io/plugin and go.opencensus.io/stats

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
@dcantah dcantah changed the title Add open census spans for ncproxy Add open census spans for ncproxy + go mod vendor Mar 12, 2021
@dcantah
Copy link
Contributor Author

dcantah commented Mar 12, 2021

Ok last rename I swear 😆

Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants