-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
stats/opencensus: OpenCensus instrumentation api #5919
Conversation
5150d97
to
282b87c
Compare
stats/opencensus/opencensus.go
Outdated
"context" | ||
"go.opencensus.io/trace" |
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.
Nit: please organize your imports; something like:
import (
"std"
"lib"
"imports"
"other"
"imports"
renamed "imports"
)
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 always see a space between std lib imports and others. However, I sometimes see non grpc imports separated from grpc imports, and renamed imports never separate, only thing I've seen after gRPC imports are something like proto imports. I put spaces in between std lib, other, and grpc.
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.
Yeah, there are no hard-set rules everyone follows, and without a linter for it, we'll never enforce any particular rule in our repo. It's definitely a widely used convention to separate stdlib imports from others, so as long as we do that I'm fine with whatever other preferences people have.
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.
Yeah that seems like the most important/always followed distinction.
stats/opencensus/opencensus.go
Outdated
// DialOption returns a dial option which enables OpenCensus instrumentation | ||
// code for a grpc.ClientConn. | ||
// | ||
// Client Applications interested in instrumenting their grpc.ClientConn should |
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.
Nit: Applications->applications
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.
Whoops, switched.
stats/opencensus/opencensus.go
Outdated
// | ||
// Using this option will always lead to instrumentation, however in order to | ||
// use the data an exporter must be registered with the OpenCensus trace package | ||
// for traces and the OpenCensus view package for metrics. Client Side has |
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.
Nit: "Client-side" or "Client side"
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.
Chose latter. Changed Server Side later in file as well.
stats/opencensus/opencensus.go
Outdated
// | ||
// Using this option will always lead to instrumentation, however in order to | ||
// use the data an exporter must be registered with the OpenCensus trace package | ||
// for traces and the OpenCensus view package for metrics. Server Side does not |
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.
Server-side / Server side
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.
Switched to latter.
stats/opencensus/opencensus.go
Outdated
// have retries, so a registered Stats Handler is the only component you need | ||
// for RPC traces/metrics. |
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.
"... so a registered Stats Handler is the only option that is returned"?
Also it is probably important to call out that (at least for now) it's not possible to use this in conjunction with another stats handler dial/server option.
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.
Fair enough, will add, but this was never possible right? I added binary logger option in this PR: #5675, which added a second possible binary logger on top of the global at each binary log recording point. Also not visible to external users, but fair point to document.
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.
Correct, it was never possible. But this is the first time we've ever provided a function that returns a grpc.WithStatsHandler
DialOption
(/ ServerOption
), as opposed to the OC package which provides a grpc/stats.Handler
, so it's important to mention that this sets a stats.Handler
and no other one should be used on the same client/server (for now). It seems like we will need to support multiple in the not-too-distant future for migration to OpenTelemetry, though.
5f1b41b
to
1793f1d
Compare
3f5e591
to
17217b8
Compare
This PR shows the API for OpenCensus instrumentation in grpc-go. On the client side, an interceptor and stats handler will be part of the dial option, and on the server side, only a stats handler will be registered. The data passed in is the Sampler, to give users a knob on the trace sampling rate, and also a bool to disable traces selectively, needed for observability GA.
RELEASE NOTES: N/A