Update gRPC measures and views from spec #689

semistrict · 2018-04-08T07:28:06Z

Fixes: #687

odeke-em · 2018-04-09T17:54:35Z

plugin/ocgrpc/client_metrics.go

-	ClientRequestCount     = stats.Int64("grpc.io/client/request_count", "Number of client RPC request messages", stats.UnitNone)
-	ClientResponseCount    = stats.Int64("grpc.io/client/response_count", "Number of client RPC response messages", stats.UnitNone)
-	ClientRoundTripLatency = stats.Float64("grpc.io/client/roundtrip_latency", "RPC roundtrip latency in msecs", stats.UnitMilliseconds)
+	ClientSentMessagesPerRPC     = stats.Int64("grpc.io/client/sent_messages_per_rpc", "Number of messages sent in the RPC (always 1 for non-streaming RPCs).", stats.UnitNone)


This unit is wrong, please update it to "1" as the I believe the spec says in https://github.com/census-instrumentation/opencensus-specs/blob/master/stats/gRPC.md#units

Oh, I guess I hadn't seen the definition of "stats.UnitNone" which is a dimensionless unit, but it was confusing initially, "UnitNone" to me would be "" instead of "1" whereas perhaps we could use "stats.UnitDimensionless"

odeke-em · 2018-04-09T17:55:15Z

plugin/ocgrpc/client_metrics.go

-	ClientRoundTripLatency = stats.Float64("grpc.io/client/roundtrip_latency", "RPC roundtrip latency in msecs", stats.UnitMilliseconds)
+	ClientSentMessagesPerRPC     = stats.Int64("grpc.io/client/sent_messages_per_rpc", "Number of messages sent in the RPC (always 1 for non-streaming RPCs).", stats.UnitNone)
+	ClientSentBytesPerRPC        = stats.Int64("grpc.io/client/sent_bytes_per_rpc", "Total bytes sent across all request messages per RPC.", stats.UnitBytes)
+	ClientReceivedMessagesPerRPC = stats.Int64("grpc.io/client/received_messages_per_rpc", "Number of response messages received per RPC (always 1 for non-streaming RPCs).", stats.UnitNone)


Ditto about using "1" as the spec requires.

odeke-em · 2018-04-09T18:21:47Z

plugin/ocgrpc/client_spec_test.go

+		if got, want := m.Description(), defn.desc; got != want {
+			t.Errorf("%q: Description = %q; want %q", defn.name, got, want)
+		}
+	}


Great spec enforcement test!

odeke-em · 2018-04-09T18:22:18Z

plugin/ocgrpc/client_spec_test.go

+		t.Fatalf("len(gotMeasures) = %d; want %d", got, want)
+	}
+
+	for i, v := range views {


This is great, me likey!

odeke-em · 2018-04-09T18:43:23Z

plugin/ocgrpc/stats_common.go

+	case codes.OK:
+		return "OK"
+	case codes.Canceled:
+		return "CANCELLED"


Aha, it's funny we have "Canceled" as the enumeration but "CANCELLED" as the string as defined in https://godoc.org/google.golang.org/grpc/codes vs https://github.com/grpc/grpc/blob/master/doc/statuscodes.md

The enum type stringifies them differently btw in gRPC Go: https://github.com/grpc/grpc-go/blob/master/codes/code_string.go#L23.

Baby Jesus, perhaps let's follow what grpc-go is doing? Thanks for posting that @rakyll.

Go codes look like just the names of the vars, highly unlikely to be consistent across languages. The examples in the OpenCensus spec are consistent with the gRPC doc (all upper case, CANCELLED with two "l", UPPER_SNAKE_CASE) which is the most official source I could find. Also explicitly added to the spec to avoid future confusion: census-instrumentation/opencensus-specs#97

odeke-em · 2018-04-09T18:43:50Z

stats/record.go

 func init() {
+	debug = os.Getenv("OC_STATS_DEBUG") != ""


Let's document this perhaps?

Don't think it should be a documented feature of the library so I removed it instead.

odeke-em · 2018-04-09T19:03:05Z

zpages/rpcz.go

+			}
+			for _, tag := range row.Tags {
+				if tag.Key == ocgrpc.KeyClientStatus && tag.Value != "OK" {
+					s.ErrorsTotal += int(count)


Although we removed the measure and view for viewing the total number of gRPC errors.

Yes, so this is why I had to compute it from the CompletedRPCs measures

I have a naive question: how are we then going to view and track the number of errors?

odeke-em · 2018-04-09T19:04:04Z

Thank you for this PR @Ramonza, it's great work, I like the rigor with the spec adherence. It is quite a large PR though, so I'll need to do a couple of takes on reviewing it.

odeke-em · 2018-04-09T21:37:19Z

examples/grpc/helloworld_client/main.go

@@ -37,8 +37,8 @@ func main() {
 	// the collected data.
 	view.RegisterExporter(&exporter.PrintExporter{})

-	// Register the view to collect client request count.
-	if err := view.Register(ocgrpc.ClientErrorCountView); err != nil {


I have a naive question, how are now going to be able to track the number of errors in either the client or server?

We now break down the requests by status, so you can get the total errors by adding up all the non-success statuses like we do in rpcz.

TestCumulativenessFromHistograms failed on AppVeyor. Looks likely to be timing related. Without rewriting the test to avoid timing issues, I am just increasing the wait times in the hopes that this will resolve the issue.

Fixes: census-instrumentation#687

semistrict · 2018-04-10T21:15:42Z

@odeke-em @rakyll PTAL

bogdandrutu · 2018-04-10T21:36:19Z

plugin/ocgrpc/stats_common.go

+
+func statusCodeToString(s *status.Status) string {
+	// see https://github.com/grpc/grpc/blob/master/doc/statuscodes.md
+	switch c := s.Code(); c {


Usually switch statements are very slow. Probably better to define a static map<Code, String>

Curious what the intuition behind that is? Did a benchmark and turns out that the switch is actually significantly faster, at least on my laptop:

name time/op StatusCodeToString_OK-8 2.66ns ± 0% StatusCodeToString_Unauthenticated-8 2.88ns ± 0% MapAlternativeImpl_OK-8 7.56ns ± 1%

Then keep the switch statement. the map search seems to me very slow, maybe is a Go thing with maps. The way how the switch statements are now implemented is using a 2-level jump table (https://www.codeproject.com/Articles/100473/Something-You-May-Not-Know-About-the-Switch-Statem) so it should be one extra jump, but probably the way how the map is implemented in Go is also 2-level jump (does not store the values directly in the map, instead it uses pointers).

rakyll · 2018-04-18T20:50:08Z

plugin/ocgrpc/benchmark_test.go

@@ -0,0 +1,52 @@
+package ocgrpc


Missing license header.

rakyll · 2018-04-20T18:47:41Z

plugin/ocgrpc/benchmark_test.go

+func BenchmarkMapAlternativeImpl_OK(b *testing.B) {
+	st := status.New(codes.OK, "OK")
+	for i := 0; i < b.N; i++ {
+		s := codeToStringMap[st.Code()]


codeToStringMap[st.Code()]

OpenCensus gRPC views are being updated to reflect the latest spec. Remove a reference to an old view that will not longer exist after the update, and instead subscribe to the default set of client-side views. OpenCensus PR: census-instrumentation/opencensus-go#689 Change-Id: I3308ec22b71c8548e3c7fd39cfad597cab65d241 Reviewed-on: https://code-review.googlesource.com/26370 Reviewed-by: Jonathan Amsterdam <[email protected]>

codefromthecrypt added the in progress label Apr 8, 2018

semistrict force-pushed the update-grpc-stats branch 4 times, most recently from c531a42 to d07bc02 Compare April 9, 2018 08:40

semistrict requested review from rakyll and odeke-em and removed request for rakyll April 9, 2018 08:51

semistrict removed the in progress label Apr 9, 2018

odeke-em reviewed Apr 9, 2018

View reviewed changes

semistrict force-pushed the update-grpc-stats branch from 6a0e34b to 50c3735 Compare April 9, 2018 21:28

odeke-em reviewed Apr 9, 2018

View reviewed changes

songy23 mentioned this pull request Apr 10, 2018

Add new RPC measure and view constants, deprecate old ones. census-instrumentation/opencensus-java#1115

Merged

Ramon Nogueira added 3 commits April 10, 2018 10:19

Increase wait time after AppVeyor suprious failure

dc65daf

TestCumulativenessFromHistograms failed on AppVeyor. Looks likely to be timing related. Without rewriting the test to avoid timing issues, I am just increasing the wait times in the hopes that this will resolve the issue.

Update gRPC measures and views from spec

94ca4c7

Fixes: census-instrumentation#687

Replace stats.UnitNone with stats.UnitDimensionless.

b66f142

semistrict force-pushed the update-grpc-stats branch from 50c3735 to b66f142 Compare April 10, 2018 08:19

bogdandrutu reviewed Apr 10, 2018

View reviewed changes

Add benchmark to test if map-based statusCodeToString is better

a0fc081

semistrict force-pushed the update-grpc-stats branch from 93f04e2 to a0fc081 Compare April 10, 2018 22:04

semistrict added the in progress label Apr 11, 2018

semistrict mentioned this pull request Apr 13, 2018

Reflect spec changes to the gRPC plugin #702

Closed

semistrict added api-breaking and removed in progress api-breaking labels Apr 13, 2018

rakyll approved these changes Apr 20, 2018

View reviewed changes

Add back ClientRequestCountView, it is used by cloud.google.com/go

b90813a

semistrict force-pushed the update-grpc-stats branch from f0341f8 to b90813a Compare April 20, 2018 20:22

semistrict merged commit 85464ad into census-instrumentation:master Apr 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update gRPC measures and views from spec #689

Update gRPC measures and views from spec #689

semistrict commented Apr 8, 2018 •

edited

Loading

odeke-em Apr 9, 2018

odeke-em Apr 9, 2018

odeke-em Apr 9, 2018

odeke-em Apr 9, 2018

odeke-em Apr 9, 2018

odeke-em Apr 9, 2018

rakyll Apr 10, 2018

odeke-em Apr 10, 2018

semistrict Apr 10, 2018

odeke-em Apr 9, 2018

semistrict Apr 9, 2018

odeke-em Apr 9, 2018

semistrict Apr 9, 2018

odeke-em Apr 9, 2018

odeke-em commented Apr 9, 2018

odeke-em Apr 9, 2018

semistrict Apr 10, 2018

semistrict commented Apr 10, 2018

bogdandrutu Apr 10, 2018

semistrict Apr 10, 2018

bogdandrutu Apr 10, 2018

rakyll Apr 18, 2018

rakyll Apr 20, 2018

Update gRPC measures and views from spec #689

Update gRPC measures and views from spec #689

Conversation

semistrict commented Apr 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odeke-em commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

semistrict commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

semistrict commented Apr 8, 2018 •

edited

Loading