stackdriver exporter implemented by lychung83 · Pull Request #2 · easwars/cloudsql-proxy

lychung83 · 2018-09-01T00:32:03Z

Let's hope this sync is clean.

easwars

More comments on the way ...

easwars · 2018-09-04T22:13:36Z

monitoring/exporter/exporter.go

+// 3.2. RowData has correcponding GCP projects, and we can determine its project ID.
+// 3.3. After trimming labels and tags, configuration of all view data matches that of corresponding
+//      stackdriver metric
+package exporter


Please rename the directory and the associated package as "stackdriver", so that we can add support for other exporter backends in the future.

easwars · 2018-09-04T22:15:14Z

monitoring/exporter/exporter.go

+	"go.opencensus.io/stats/view"
+	"google.golang.org/api/option"
+	"google.golang.org/api/support/bundler"
+	monitoredrespb "google.golang.org/genproto/googleapis/api/monitoredres"


If you are taking the trouble to alias the import, I would rather that you use a smaller alias name. Something like:
mrpb "google.golang.org/genproto/googleapis/api/monitoredres"
mpb "google.golang.org/genproto/googleapis/monitoring/v3"
etc.
Here and in all other places. Thanks.

easwars · 2018-09-04T22:16:06Z

monitoring/exporter/exporter.go

+
+// StatsExporter is the exporter that can be registered to opencensus. A StatsExporter object must
+// be created by NewStatsExporter().
+type StatsExporter struct {


This could be renamed as just Exported, so that usages would be stackdriver.Exporter.

easwars · 2018-09-04T22:18:37Z

monitoring/exporter/exporter.go

+// StatsExporter is the exporter that can be registered to opencensus. A StatsExporter object must
+// be created by NewStatsExporter().
+type StatsExporter struct {
+	ctx    context.Context


I'm not a big fan of storing a context within a struct. I see that the context object is only used for creating the metric client in NewStatsExporter(), which is already being passed the context. So, why do you need to store the context object?

Sorry, I see that you are using the context in other places. Would it possible to actually pass a context from the caller instead of storing it in this exporter object. Something to think about. If it is too much of a hassle, at least add a TODO to have that cleaned up later on.

https://groups.google.com/forum/#!topic/golang-nuts/xRbzq8yzKWI

I generally agree that context should not be stored in a struct. But MetricClient.CreateTimeSeries() is not directly called by customer, so I think it's ugly compromise for now. My lame excuse is that current stackdriver exporter is doing the same thing. (sigh...) But I added a TODO as you suggested.

easwars · 2018-09-04T22:35:39Z

monitoring/exporter/exporter.go

+	BundleDelayThreshold time.Duration
+	BundleCountThreshold int
+
+	// callback functions provided by user.


Please start comments with upper case. Here and in other places.

easwars · 2018-09-04T23:35:02Z

monitoring/exporter/exporter.go

+
+	// We don't want to modify user-supplied options, so save default options directly in
+	// exporter.
+	if opts.GetProjectID != nil {


You can replace all these if-else blocks with an initialization followed by an if-block.

Example:
e.onError = defaultOnError
if opts.OnError != nil {
e.onError = opts.OnError
}

easwars · 2018-09-04T23:41:28Z

monitoring/exporter/exporter.go

+}
+
+// We wrap monitoring.MetricClient and it's maker for testing.
+type metricClient interface {


You shouldn't have to define an interface for this. Instead the client member in the exporter should just be a monitoring.MetricClient object, and in your test you should make a fake MetricClient by mocking the MetricServiceClient in the below definition of MetricClient.

// MetricClient is a client for interacting with Stackdriver Monitoring API.
type MetricClient struct {
// The connection to the service.
conn *grpc.ClientConn

// The gRPC API client. metricClient monitoringpb.MetricServiceClient // The call options for this service. CallOptions *MetricCallOptions // The metadata to be sent with each request. metadata metadata.MD

}

ok. I removed that interface but mocked monitoring.NewMetricClient and monitoring.MetricClient.CreateTimesSeries

easwars · 2018-09-04T23:45:20Z

monitoring/exporter/exporter.go

+}
+
+// default values for options
+func defaultGetProjectID(rd *RowData) (string, error) {


Would it better if the default function handled the checking of Tags in the RowData to see if there is a project_id tag and returning the corresponding ID?

ok. I did and wrote a test for it

easwars · 2018-09-04T23:51:19Z

monitoring/exporter/project_data.go

+}
+
+// We wrap bundler and its maker for testing purpose.
+type expBundler interface {


Again, I feel this is not the correct use of interface. You should return concrete types wherever possible, and interfaces should be defined by the users at the place of use.
https://medium.com/@cep21/preemptive-interface-anti-pattern-in-go-54c18ac0668a

ok. Similar to monitroing.MetricClient, I mocked some function defined in the blunder package.

lychung83 · 2018-09-07T18:05:21Z

And general comments (most of them about tests) on commit "fix suggested by easwars" 3f25a33

While removing all interfaces only for testing, I created some global variables for testing. These are used per test case, so I moved all tests under one big test TestAll().
More data are defined in data_test.go. Most of them are used for testing behavior of defaultGetProjectID(), but adding value7 and value8 are simply for avoiding any possible conflict in testMakeLabel() with existing values of labels.

easwars

I haven't yet looked at the tests. Will wait till the code comments are resolved. Thanks.

easwars · 2018-09-07T17:51:32Z

monitoring/exporter/stackdriver/stackdriver.go

+	// RPC calls.
+	ClientOptions []option.ClientOption
+
+	// options for bundles amortizing export requests. Note that a bundle is created for each


Please add a separate comment for each exported variable, in Go syntax.

easwars · 2018-09-07T17:56:46Z

monitoring/exporter/stackdriver/stackdriver.go

+	return &mrpb.MonitoredResource{Type: "global"}, nil
+}
+
+// Following functions are wrapper of functions that may show non-deterministic behavior. Only tests


What non-deterministic behavior are you talking about?

I think that comment is unnecessary. I simply removed those parts.

easwars · 2018-09-07T18:03:59Z

monitoring/exporter/stackdriver/stackdriver.go

+		projDataMap: make(map[string]*projectData),
+	}
+
+	// We don't want to modify user-supplied options, so save default options directly in


How about accepting an Options instead of *Options in this function, and you would need to make a copy here.

easwars · 2018-09-07T18:05:27Z

monitoring/exporter/stackdriver/project_data.go

+	mpb "google.golang.org/genproto/googleapis/monitoring/v3"
+)
+
+// MaxTimeSeriePerUpload is the maximum number of time series that stackdriver accepts. Only test


Is this some hard limit imposed by Stackdriver? If not, why should it be not configurable?

Replied separately.

As you suggested I reduced the number to 100, and make it configurable but put a warning there.

easwars · 2018-09-07T18:07:09Z

monitoring/exporter/stackdriver/project_data.go

+	bndler *bundler.Bundler
+}
+
+func (e *Exporter) newProjectData(projectID string) *projectData {


This function should ideally reside with other functions of the Exporter type.

easwars · 2018-09-07T18:12:14Z

monitoring/exporter/stackdriver/project_data.go

+}
+
+// makeLables constructs label that's ready for being uploaded to stackdriver.
+func (e *Exporter) makeLabels(tags []tag.Tag) map[string]string {


Same for this function.

Sorry, I changed the file name from project_data.go to upload.go and collects all functions/methods that works for uploading row data after bundler request for an upload operation. (It seems better for me. If you don't like, I can split by structs again, or merge everything to one file except row_data_to_point.go )

I followed your suggestion. One file per struct now.

easwars · 2018-09-07T18:13:34Z

monitoring/exporter/stackdriver/project_data.go

+		pt := newPoint(rd.View, rd.Row, rd.Start, rd.End)
+		if pt.Value == nil {
+			err := fmt.Errorf("inconsistent data found in view %s", rd.View.Name)
+			pd.parent.onError(err, rd)


How about calling these onError functions in goroutines to not block here on what happens in the error handlers?

Can we just add requirement to onError() that it should be non-blocking? If I call "go { ...onError() }", then, I guess it's better to create go routine for all places that onError() is called.

easwars · 2018-09-07T19:14:14Z

monitoring/exporter/stackdriver/project_data.go

+	// reqRds contains RowData objects those are uploaded to stackdriver at given iteration.
+	// It's main usage is for error reporting. For actual uploading operation, we use req.
+	// remainingRds are RowData that has not been processed at all.
+	var reqRds, remainingRds []*RowData


This whole flow seems a little hard to read for me. How about restructuring this code as follows:

func uploadRowData(bundle) {

get the slice of RowData from the bundle

iterate through the slice, and for each RowData {

call a func named makeTS() which returns one &mpb.TimeSeries

maintain a slice of TimeSeries objects here, and append the one returned in the previous line to that slice

if the slice has reached the required length, call another function (or just inline it here) to actually make the RPC
}

if you have items still in the timeSeries slice, make another call to send an RPC
}

What do you think?

I generally followed your suggestion.

lychung83 · 2018-09-08T05:43:44Z

I merged all changes to one commit: 'review round 2 fix" 8fb9793 . I hope it's easy for you to review.

easwars · 2018-09-10T17:38:20Z

monitoring/exporter/stackdriver/project_data.go

+	mpb "google.golang.org/genproto/googleapis/monitoring/v3"
+)
+
+// MaxTimeSeriePerUpload is the maximum number of time series that's uploaded to the stackdriver


Would you mind framing this as follows:

MaxTimeSeriesPerUpload is the maximum number of timeseries objects that will be uploaded to Stackdriver in one API call. Users are not encouraged to change this value as Stackdriver may reject an upload request if it contains too many timeseries objects.

The behavior of MaxTimeSeriesPerUpload is somewhat changed, so I also changed the comment.

easwars · 2018-09-10T17:39:18Z

monitoring/exporter/stackdriver/project_data.go

+// MaxTimeSeriePerUpload is the maximum number of time series that's uploaded to the stackdriver
+// at once. Consumer may change this value, but note that stackdriver may reject upload request if
+// the number of time series is too large.
+var MaxTimeSeriesPerUpload = 100


Does this have to an exported variable?

Did separate discussion to resolve issue.

easwars · 2018-09-10T17:39:54Z

monitoring/exporter/stackdriver/project_data.go

+type projectData struct {
+	parent    *Exporter
+	projectID string
+	// We make bundler for each project because call to monitoring RPC can be grouped only in


s/monitoring RPC/monitoring API

easwars · 2018-09-10T17:41:55Z

monitoring/exporter/stackdriver/project_data.go

+		TimeSeries: ts,
+	}
+	if err := createTimeSeries(exp.client, exp.ctx, req); err != nil {
+		newErr := fmt.Errorf("RPC call to create time series failed for project %s: %v", pd.projectID, err)


Please refer to these as API calls and not RPC calls, here and everywhere else. Thanks.

easwars · 2018-09-10T18:05:41Z

monitoring/exporter/stackdriver/mock_check_test.go

+
+// newTestExp creates an exporter which saves error to errStorage. Caller should not set
+// opts.OnError.
+func newTestExp(t *testing.T, opts Options) *Exporter {


Please make this file consistent by not accepting testing.T as a parameter to these helper functions.

These helper functions should just return an error, and it is up to the caller to call t.Error() or t.Fatal().

I also implemented a tiny helper type to aggregate multiple errors.

lychung83 · 2018-09-11T01:04:50Z

Changes not mentioned in above.

As behavior of MaxTimeSeriePerUpload is changed, I check all bundle related option also in NewExporter. To me it's more consistent.
Nit in the beginning of checkProjData(). We don't need separate wantProj.

stackdriver exporter implemented

277f032

easwars reviewed Sep 4, 2018

View reviewed changes

move and rename files

fcf87af

lychung83 force-pushed the stackdriver-exporter-review branch from 865798a to 277f032 Compare September 7, 2018 17:39

fix suggested by easwars

3f25a33

easwars reviewed Sep 7, 2018

View reviewed changes

lychung83 force-pushed the stackdriver-exporter-review branch from 151f1a0 to 51b24c1 Compare September 7, 2018 23:58

review round 2 fix

8fb9793

lychung83 force-pushed the stackdriver-exporter-review branch from 0b16819 to 8fb9793 Compare September 8, 2018 05:37

easwars reviewed Sep 10, 2018

View reviewed changes

review round 3 response

1e12b16

lychung83 force-pushed the stackdriver-exporter-review branch from bf4e62e to 1e12b16 Compare September 11, 2018 01:09

easwars approved these changes Sep 11, 2018

View reviewed changes

minor correction with fake

3641a89

Conversation

lychung83 commented Sep 1, 2018

Uh oh!

easwars left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lychung83 commented Sep 7, 2018

Uh oh!

easwars left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment