-
Notifications
You must be signed in to change notification settings - Fork 327
Clean up ocgrpc: remove NewXXXStatsHandler methods #539
Clean up ocgrpc: remove NewXXXStatsHandler methods #539
Conversation
@@ -44,7 +44,7 @@ func main() { | |||
|
|||
// Set up a connection to the server with the OpenCensus | |||
// stats handler to enable stats and tracing. | |||
conn, err := grpc.Dial(address, grpc.WithStatsHandler(ocgrpc.NewClientStatsHandler()), grpc.WithInsecure()) | |||
conn, err := grpc.Dial(address, grpc.WithStatsHandler(new(ocgrpc.ClientHandler)), grpc.WithInsecure()) |
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.
This is not a common style in Go for inline code.
It is somewhat encouraged with var for zero values:
var hander = new(ocgrpc.ClientHandler)
Use &ocgrpc.ClientHandler{}
instead.
@@ -59,7 +59,7 @@ func main() { | |||
|
|||
// Set up a new server with the OpenCensus | |||
// stats handler to enable stats and tracing. | |||
s := grpc.NewServer(grpc.StatsHandler(ocgrpc.NewServerStatsHandler())) | |||
s := grpc.NewServer(grpc.StatsHandler(new(ocgrpc.ServerHandler))) |
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.
ditto
plugin/ocgrpc/grpc_test.go
Outdated
te := &traceExporter{} | ||
trace.RegisterExporter(te) | ||
func TestClientHandler(t *testing.T) { | ||
var ( |
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.
Remove the var grouping here.
plugin/ocgrpc/grpc_test.go
Outdated
te := &traceExporter{} | ||
trace.RegisterExporter(te) | ||
func TestServerHandler(t *testing.T) { | ||
var ( |
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.
Ditto.
32c48a9
to
410fb67
Compare
@rakyll PTAL |
plugin/ocgrpc/grpc_test.go
Outdated
span := trace.NewSpan("/foo", nil, trace.StartOptions{ | ||
Sampler: trace.AlwaysSample(), | ||
}) | ||
ctx = trace.WithSpan(ctx, span) | ||
|
||
var handler ServerHandler |
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.
handler := &ServerHandler{}
plugin/ocgrpc/grpc_test.go
Outdated
te := &traceExporter{} | ||
trace.RegisterExporter(te) | ||
if err := ServerRequestCountView.Subscribe(); err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
// Ensure we start tracing |
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.
// Ensure we start tracing.
End with full stop. In Go, block comments are proper English sentences, side comments can be non-proper sentences. For example:
ctx = trace.WithSpan(ctx, span) // span in the context
Since we provide options on the ClientHandler and ServerHandler structs and the NewXXX methods were trivial, it is better to have users use a struct literal.
410fb67
to
e0b3e58
Compare
Since we provide options on the ClientHandler and ServerHandler structs
and the NewXXX methods were trivial, it is better to have users use a
struct literal.