-
Notifications
You must be signed in to change notification settings - Fork 259
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
Add open census spans for ncproxy + go mod vendor #966
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
We should make sure we have the |
@kevpar I think I have the ttrpc ones handled, but I don't for grpc |
86eb619
to
bb6e20f
Compare
cmd/ncproxy/main.go
Outdated
@@ -50,7 +67,12 @@ func main() { | |||
if conf.NodeNetSvcAddr != "" { | |||
log.G(ctx).Debugf("connecting to NodeNetworkService at address %s", conf.NodeNetSvcAddr) | |||
|
|||
opts := []grpc.DialOption{grpc.WithInsecure()} | |||
// Register views to collect data. | |||
if err := view.Register(ocgrpc.DefaultClientViews...); err != nil { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
e50feb3
to
eaa6acb
Compare
* 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>
Ok last rename I swear 😆 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR updates our internal ADO repo to the github commit [d9474d2](microsoft@d9474d2). Related work items: microsoft#964, microsoft#965, microsoft#966, microsoft#967, microsoft#968, microsoft#969, microsoft#970, microsoft#971, microsoft#972, microsoft#974, microsoft#975, microsoft#976, microsoft#977, microsoft#978, microsoft#979, microsoft#980, microsoft#981, microsoft#982, microsoft#983, microsoft#984, microsoft#987, microsoft#990, microsoft#991, microsoft#992, microsoft#993, microsoft#995, microsoft#996, microsoft#997, microsoft#1000
Signed-off-by: Daniel Canter dcanter@microsoft.com