gRPC integration #12

acetechnologist · 2017-09-15T01:13:18Z

Added grpc handlers.
Added sumOfSquaredDeviation to aggregation distribution (to mimic stackdriver) and simplified its implementation to always include the oldest bucket.

…nly supportted in 1.8 and above

…e and aggregator. Also added equality as an interface to aggregationValue

…ains()... Also renamed some methods to be more succint and fixed some documentation issues

…ntually support conversions to other formats

…readme.md.

…strumentation-go Resync fork to upstream/master

menghanl · 2017-09-18T22:09:10Z

plugins/grpc/handlers.go

+package stats
+
+import (
+	"context"


gRPC-go still imports "golang.org/x/net/context" (grpc/grpc-go#711).
It will be a massive incompatible API change for all users using <=go1.8 if we change that.

go treats func("context".Context) and func("golang.org/x/net/context".Context) as two different types, which means the stats handler here won't work (it doesn't compile, with <=go1.8).

go1.9 introduced a new feature called type alias, which makes "golang.org/x/net/context" and "context" the same type. So, this actually works fine with >=go1.9.

The solution I can think of right now is to import "golang.org/x/net/context" in the stats package.

Or, if we decide that the stats package only supports users with >=go1.9, the current code will be fine.

Since this is our first release, I think we should avoid creating unnecessary confusion and say that we support >= go1.9 if using stats with grpc.
Wondering if @rakyll have a different opinion.

Thinking about it more. I think it is better to use "golang.org/x/net/context" so we can get more adoption and since once everybody moves beyond 1.8 it will be easy to move to "context".Context

menghanl · 2017-09-18T22:55:10Z

plugins/grpc/stats/client_handler.go

+
+// ClientHandler is the type implementing the "google.golang.org/grpc/stats.Handler"
+// interface to process lifecycle events from the GRPC client.
+type ClientHandler struct{}


Unexport this and add an exported function instead?

Not sure I understand your comment. You mean adding an exported "constructor" for it: eg func NewClientHandler() stats.Handler {...}

menghanl · 2017-09-18T23:20:19Z

plugins/grpc/stats/client_handler.go

+	ctx = metadata.NewOutgoingContext(ctx, metadata.Join(md, metadata.Pairs(tagsKey, string(encoded))))
+
+	tsb := tags.NewTagSetBuilder(ts)
+	tsb.UpsertString(keyService, serviceName)


Why does this function return a TagSetBuilder if it's called to update the builder?

To make it easier to use the shortcut version when modifying multiple tags. It returns the same TagSetBuilder. It is just a syntactic sugar to allow for the short form:
tags.NewTagSetBuilder(ts).UpsertString(keyService, serviceName).UpsertString(key2, value2).AddString(key3, value3)...

menghanl · 2017-09-18T23:35:38Z

plugins/grpc/handlers.go

@@ -0,0 +1,75 @@
+// Copyright 2017, OpenCensus Authors


Good catch. Thank you. Fixed now.

menghanl · 2017-09-18T23:36:26Z

plugins/grpc/stats/server_handler.go

+	"github.com/golang/glog"
+	"google.golang.org/grpc/metadata"
+	"google.golang.org/grpc/stats"
+	/*


Remove this

menghanl · 2017-09-18T23:46:56Z

plugins/grpc/stats/types.go

+// tagsKey is the metadata key used to identify both the stats tags in the GRPC
+// context metadata, as well as the RpcServerStats info sent back from the
+// server to the client in the GRPC context metadata.
+const tagsKey = "grpc-tags-bin"


Metadata keys start with grpc- are actually reserved, so we should avoid having a grpc- prefixed key explicitly in the code...

To make this work, we added functions to set/get trace info: https://github.com/grpc/grpc-go/blob/master/stats/stats.go#L269.

Please use the new functions instead.

menghanl · 2017-09-18T23:50:21Z

stats/aggregation_value.go

 	return &AggregationDistributionValue{
 		countPerBucket: countPerBucket,
 		bounds:         bounds,
 		count:          count,
 		min:            min,
 		max:            max,
-		sum:            sum,
+		mean:           mean,
+		sumOfSquaredDeviation: sumOfSquaredDeviation,


This is not formatted correctly.

menghanl · 2017-09-18T23:54:02Z

stats/aggregation_value.go

-		ret.sum = ret.min
-		ret.incrementBucketCount(ret.min)
-		return ret
+	ret := newAggregationDistributionValue(a.bounds)


Did you miss anything? Parameter fraction is not used in this function.

This method is part of the interface signature: "type AggregationValue interface".
We decided that for this specific aggregation we will not use the "fraction" for simplicity

…for Go1.9 to integrate with libraries still using golang.org/x/net/context.Context. Fixed the license headings to refer to opencensus instead of google.

…ructor for them.

…ontext by golang.org/x/net/context and reduced the requirement to 1.8

…ggregationDistributionValue

…tingAggregationDistributionValue in server_handler_test.go

menghanl

Two nits, otherwise LGTM.

menghanl · 2017-09-22T17:08:33Z

README.md

@@ -20,7 +20,8 @@ To install this package, you need to install Go and setup your Go workspace on y
 $ go get -u github.com/census-instrumentation/opencensus-go

 ## Prerequisites
-This requires Go 1.8 or later as it uses the convenience function sort.Slice(...) in few places.
+This requires Go 1.8 or later as it uses the convenience function sort.Slice(...) introduced in Go 1.8.
+It also uses "golang.org/x/net/context".Context introduced in Go 1.7 heavily. Therefore, in order to integrate with any library still using "golang.org/x/net/context".Context, Go 1.9 or higher is required to take advantage of type aliasing. 


Do we want to keep this line?

No we don't. Deleted it.

menghanl · 2017-09-22T17:09:47Z

plugins/grpc/stats/client_handler.go

+package stats
+
+import (
+	"golang.org/x/net/context"


goimports would complain about the order of the imported packages.

We do those checks on travis in gRPC: https://github.com/menghanl/grpc-go/blob/master/vet.sh#L51-L54

acetechnologist added 21 commits September 13, 2017 15:52

Add initial stats API.

4cecef5

fixed readme.md file

45542ec

fixed readme.md file

88f0d49

fixing yaml file

ce9353e

fixing yaml file

9714c63

remove go 1.7 and 1.6 from yaml file as we use 'sort.Slice' that is o…

0ee8a94

…nly supportted in 1.8 and above

Added to README.md a section about modifying the reporting interval.

f1d95ba

Using inheritence instead of type switch for creating aggregationValu…

de85e90

…e and aggregator. Also added equality as an interface to aggregationValue

Deleted not used directories

62d36a5

implemented automatic registration for measures on creation

412e8ae

implemented codec for tags to be used in GRPC

8496b0c

implemented codec for tags to be used in GRPC

8c1b0f5

implemented codec for tags to be used in GRPC

e2b9e39

Fixed readme.md typos. Changed API to create a tagSet builder

a086caa

added convenience methods to various structs: String(), Equal(), Cont…

2447f0b

…ains()... Also renamed some methods to be more succint and fixed some documentation issues

added getter methods to interfaces and made some fields public to eve…

dacbcce

…ntually support conversions to other formats

addressed commnets for the pull request. Fixed both method names and …

9a7ba3f

…readme.md.

added installation and prereq comments to readme.md

48af633

Merge branch 'master' of https://github.com/census-instrumentation/in…

da0f143

…strumentation-go Resync fork to upstream/master

Fixed aggregationDistirbution to include oldest bucket in interval stats

0a2a0d2

Added plugin interceptor for grpc stats

2f7f729

bogdandrutu requested a review from rakyll September 15, 2017 20:22

Added all defaults measures and views to be collected for grpc stats

c86ab26

acetechnologist requested a review from menghanl September 18, 2017 18:20

menghanl reviewed Sep 18, 2017

View reviewed changes

acetechnologist requested a review from jcd2 September 18, 2017 23:13

menghanl reviewed Sep 18, 2017

View reviewed changes

acetechnologist added 2 commits September 19, 2017 10:53

Addressed PR comments. Modified readme.md to mention the requirement …

85a5395

…for Go1.9 to integrate with libraries still using golang.org/x/net/context.Context. Fixed the license headings to refer to opencensus instead of google.

Made serverHandler and clientHandler private and added exported const…

174ba23

…ructor for them.

acetechnologist requested a review from bogdandrutu September 20, 2017 20:23

acetechnologist added 5 commits September 20, 2017 13:28

Requires Go 1.9 to take advantage of context type aliasing.

695335d

Requiring Go 1.9 seems quite limiting. So I went ahead and replaced c…

b5d7b91

…ontext by golang.org/x/net/context and reduced the requirement to 1.8

Reworded comments for NewTestingAggregationDistributionValue

fb0ec85

Renamed NewTestingAggregationDistributionValue to NewDoNotUseTestingA…

47e4a1b

…ggregationDistributionValue

Fixed naming NewTestingAggregationDistributionValue to NewDoNotUseTes…

827f7c2

…tingAggregationDistributionValue in server_handler_test.go

menghanl approved these changes Sep 22, 2017

View reviewed changes

Added gofmt to .travis script. Fixed formatting issues

58b58e5

acetechnologist merged commit 2d3c093 into census-instrumentation:master Sep 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gRPC integration #12

gRPC integration #12

acetechnologist commented Sep 15, 2017

menghanl Sep 18, 2017

acetechnologist Sep 19, 2017 •

edited

Loading

acetechnologist Sep 20, 2017

menghanl Sep 18, 2017

acetechnologist Sep 19, 2017 •

edited

Loading

menghanl Sep 20, 2017

acetechnologist Sep 20, 2017

menghanl Sep 18, 2017

acetechnologist Sep 19, 2017 •

edited

Loading

menghanl Sep 18, 2017

acetechnologist Sep 19, 2017

menghanl Sep 18, 2017

acetechnologist Sep 19, 2017

menghanl Sep 18, 2017

acetechnologist Sep 19, 2017

menghanl Sep 18, 2017

acetechnologist Sep 19, 2017

menghanl Sep 18, 2017

acetechnologist Sep 19, 2017

menghanl left a comment

menghanl Sep 22, 2017

acetechnologist Sep 22, 2017

menghanl Sep 22, 2017 •

edited

Loading

acetechnologist Sep 22, 2017

gRPC integration #12

gRPC integration #12

Conversation

acetechnologist commented Sep 15, 2017

Choose a reason for hiding this comment

acetechnologist Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acetechnologist Sep 19, 2017 • 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

acetechnologist Sep 19, 2017 • 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

menghanl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menghanl Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acetechnologist Sep 19, 2017 •

edited

Loading

acetechnologist Sep 19, 2017 •

edited

Loading

acetechnologist Sep 19, 2017 •

edited

Loading

menghanl Sep 22, 2017 •

edited

Loading